Make add-on store accelerator keys label translatable and discoverable by CyrilleB79 · Pull Request #15103 · nvaccess/nvda

CyrilleB79 · 2023-07-05T14:56:50Z

Link to issue number:

Closes #15010
Closes #15011

Summary of the issue:

In add-on store, some accelerator keys are not discoverable nor translatable:

alt+L for the add-on list is hard-coded (thus not modifiable by translators) and is not discoverable, neither visually (no visible label), nor via object navigation on the whole list
alt+O for "Other details" field is not discoverable visually nor with shift+numpad2.

Description of user facing changes

The labels for the list and for "other details" field are made visible and the accelerator key is underlined.
shift+numpad2 allows to report the accelerator key of the field.

Description of development approach

Do not hide anymore the labels of the list and "other details" field
used the same strings for the label of the list and of the tabs; the only difference is that the "&" making a letter an accelerator key is removed when the string is used for the tabs (and for the title).
Used "alt+c" for the "Actions" button to free up "alt+a" for the add-on list label.

Testing strategy:

For "Other details" and for the 4 possible labels of the add-on list:

Visual check
Check that alt+a/alt+o are working, i.e. moving the focus
When tabbing to "Other details" or to empty add-on list, checked that alt+O/alt+A is reported
When in "Other details" or in empty add-on list, shift+numpad2 reports the shortcut key
When moving nav object to the add-on list (not specifically empty), the shortcut key is reported.

Known issues with pull request:

The shortcut key is not reported for non-empty list with shift+numpad2 or when it gains focus. This is expected and like in other lists since a child item is focused, not the whole list.

Still to be discussed

I have updated the shortcut key of the list in the user doc; but I wonder if it is still worth mentioning it at all since it is now discoverable. The user doc usually does not mention standard shortcut keys.
Maybe showing these labels make the add-on store visually less nice; feel free to suggest a better visual experience if you have a better idea.
On the opposite, I have kept the "Description" label hidden since there is no shortcut key. Do you think that this label should be shown as well to follow the convention used for the other controls? Does this description field deserve a shortcut key as well?

Change log entries:

Not needed; add-on store not yet released.

Code Review Checklist:

Pull Request description:
- description is up to date
- change log entries
Testing:
- Unit tests
- System (end to end) tests
- Manual testing
API is compatible with existing add-ons.
Documentation:
- User Documentation
- Developer / Technical Documentation
- Context sensitive help for GUI changes
UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers
- Localization in other languages / culture than English
Security precautions taken.

AppVeyorBot · 2023-07-05T16:06:06Z

Build (for testing PR): https://ci.appveyor.com/api/buildjobs/s2j9gj5y6fanbqy8/artifacts/output/nvda_snapshot_pr15103-28599,8b9d9a5c.exe
PASS: Translation comments check.
PASS: Unit tests.
CI timing (mins):
INIT 0.0,
INSTALL_START 0.8,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 23.5,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 21.3,
FINISH_END 0.1
PASS: Lint check.
FAIL: System tests (tags: installer NVDA). See test results for more information.

See test results for failed build of commit 8b9d9a5c88

AppVeyorBot · 2023-07-05T17:03:09Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/4dsty4qho3ibb7tp/artifacts/output/nvda_snapshot_pr15103-28600,cb8368cf.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.8,
INSTALL_END 0.8,
BUILD_START 0.0,
BUILD_END 24.3,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 21.4,
FINISH_END 0.2

See test results for failed build of commit cb8368cf1f

CyrilleB79 · 2023-07-05T21:24:08Z

I have tried to re-run the CI test but one test is still failing.

Passing the PR in ready anyway since the test seems unrelated.

seanbudd

Maybe showing these labels make the add-on store visually less nice; feel free to suggest a better visual experience if you have a better idea.

I believe this is why they were hidden in the first place, but after testing this code, it's not visually jarring at all.

On the opposite, I have kept the "Description" label hidden since there is no shortcut key. Do you think that this label should be shown as well to follow the convention used for the other controls? Does this description field deserve a shortcut key as well?

I personally don't see a need for a shortcut key, and I think adding a visual label in this case would be visually jarring. As a result, I think this PR can close #15011 as well.

seanbudd · 2023-07-06T04:13:10Z


 	def _setListLabels(self):
-		self.listLabel.SetLabelText(self._listLabelText)
+		self.listLabel.SetLabel(self._listLabelText)


is there a reason why this was changed? We use SetLabelText elsewhere so perhaps those instances should be changed too

SetLabelText seems to display the raw text, i.e. the "&" character appears in the label's text (e.g. "Installed &add-ons") and the accelerator key is not created. On the contrary, SetLabel converts "&" character to an accelerator key and is removed from the displayed text (e.g. "Installed add-ons").

Ah - it doesn't look this is will be an issue for the other usages.

seanbudd · 2023-07-06T04:13:33Z

 ++ Browsing add-ons ++[AddonStoreBrowsing]
 When opened, the Add-on Store displays a list of add-ons.
-You can jump back to the list with ``alt+l`` from anywhere else within the store.
+You can jump back to the add-ons list with ``alt+a`` from anywhere else within the store.


I agree, this line can be removed. There's also a reference to alt+a for actions which should be removed or updated. Additionally, you may need to update references in tests\manual\addonStore.md

Suggested change

You can jump back to the add-ons list with ``alt+a`` from anywhere else within the store.

CyrilleB79 · 2023-07-06T06:56:41Z

@seanbudd, I have updated the manual test plan based on the test section of this PR. Let me know if this update is OK for you or if I should write it differently (less details).

Regarding #15011, I will put a comment there to ask about the description field which is not part of this PR.

seanbudd · 2023-07-06T07:27:04Z

 Add-ons have associated actions, such as install, help, disable, and remove.
 The actions menu can be accessed for an add-on in the add-on list by pressing the ``applications`` key, ``enter``, right clicking or double clicking the add-on.
-There is also an Actions button in the selected add-on's details, which can be activated normally or by pressing ``alt+a``.
+There is also an Actions button in the selected add-on's details, which can be activated normally or by pressing ``alt+c``.


If we are trying to remove accelerator keys:

Suggested change

There is also an Actions button in the selected add-on's details, which can be activated normally or by pressing ``alt+c``.

There is also an Actions button in the selected add-on's details.

maybe this should be merged with the previous sentence to be a list e.g.:

There is an actions button in the add-on details which opens a menu of actions. The actions menu can also be accessed for an add-on in the add-on list by: - pressing the ``applications`` key - pressing ``enter`` - right clicking - double clicking

I have committed a minimal modification of the User Guide in 9b0f52f, just removing the accelerator key, making the assumption that users should know how to activate a button normally in Windows or discover shortcut keys through navigation.

I have then committed a wider rewording of this paragraph (a5ce3ec) that reads better IMO. Since it's less related to this PR, feel free to accept it (keeping a5ce3ec) or reject it (I will revert a5ce3ec in this case).

In both cases, I have kept the context menu solution first since it should be the more straight forward and preferred solution.

Regarding the conversion to list of various way to access the menu, with a5ce3ec, it does not seem necessary to me.

seanbudd · 2023-07-06T07:28:37Z

I have updated the manual test plan based on the test section of this PR. Let me know if this update is OK for you or if I should write it differently (less details).

The test plan looks fine. I don't think it was necessary for inclusion, however it is appreciated. For background in #15103 (comment) I was more concerned about outdated accelerator keys needing to be updated after this change like in the user guide.

seanbudd

Thanks @CyrilleB79

CyrilleB79 added 2 commits July 5, 2023 16:12

Make add-on store accelerator keys label translatable and discoverable.

3ca9d17

Update user doc

a76b5b6

Bump CI

0acf213

CyrilleB79 marked this pull request as ready for review July 5, 2023 21:24

CyrilleB79 requested review from a team as code owners July 5, 2023 21:24

CyrilleB79 requested review from Qchristensen and michaelDCurran July 5, 2023 21:24

seanbudd self-requested a review July 5, 2023 23:28

seanbudd reviewed Jul 6, 2023

View reviewed changes

seanbudd marked this pull request as draft July 6, 2023 04:22

CyrilleB79 added 2 commits July 6, 2023 08:24

Addressing review comments

9679fa1

Update add-on store manual test plan

aa61514

CyrilleB79 marked this pull request as ready for review July 6, 2023 06:56

CyrilleB79 mentioned this pull request Jul 6, 2023

NVDA add-ons store: UI: "Description" and "other details" fields do not have visual labels #15011

Closed

seanbudd reviewed Jul 6, 2023

View reviewed changes

CyrilleB79 added 2 commits July 6, 2023 10:01

Remove accelerator indication from user guide

9b0f52f

More rephrasing

a5ce3ec

seanbudd reviewed Jul 6, 2023

View reviewed changes

Comment thread user_docs/en/userGuide.t2t Outdated

fix up userguide

6f10d59

seanbudd approved these changes Jul 6, 2023

View reviewed changes

seanbudd merged commit c484878 into nvaccess:master Jul 6, 2023

nvaccessAuto added this to the 2023.2 milestone Jul 6, 2023

CyrilleB79 mentioned this pull request Aug 4, 2023

Add-on store list shares translatable strings with tabs causing accelerator keys to appear on tab names #15258

Closed

CyrilleB79 deleted the storeAccKeys branch August 29, 2023 06:46

	There is also an Actions button in the selected add-on's details, which can be activated normally or by pressing ``alt+c``.
	There is also an Actions button in the selected add-on's details.

Uh oh!

Conversation

CyrilleB79 commented Jul 5, 2023 • edited by seanbudd Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of user facing changes

Description of development approach

Testing strategy:

Known issues with pull request:

Still to be discussed

Change log entries:

Code Review Checklist:

Uh oh!

AppVeyorBot commented Jul 5, 2023

Uh oh!

AppVeyorBot commented Jul 5, 2023

Uh oh!

CyrilleB79 commented Jul 5, 2023

Uh oh!

seanbudd left a comment

Choose a reason for hiding this comment

Uh oh!

seanbudd Jul 6, 2023

Choose a reason for hiding this comment

Uh oh!

CyrilleB79 Jul 6, 2023

Choose a reason for hiding this comment

Uh oh!

seanbudd Jul 6, 2023

Choose a reason for hiding this comment

Uh oh!

seanbudd Jul 6, 2023

Choose a reason for hiding this comment

Uh oh!

CyrilleB79 commented Jul 6, 2023

Uh oh!

seanbudd Jul 6, 2023

Choose a reason for hiding this comment

Uh oh!

seanbudd Jul 6, 2023

Choose a reason for hiding this comment

Uh oh!

CyrilleB79 Jul 6, 2023

Choose a reason for hiding this comment

Uh oh!

seanbudd commented Jul 6, 2023

Uh oh!

Uh oh!

seanbudd left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CyrilleB79 commented Jul 5, 2023 •

edited by seanbudd

Loading