Report keyboard shortcut when entering a list #15826

CyrilleB79 · 2023-11-24T18:15:37Z

Link to issue number:

Summary of the issue:

When the focus enters in a list, the list's keyboard shortcut is not reported. It's because a list item takes the focus, whereas the shortcut key is on the whole list; logically, individual list items have no shortcut. We usually do not report shortcut keys when entering container objects. However, in the case of list, it would be useful so that people know that the list has a shortcut key.

Description of user facing changes

Reports shortcut when the focus enters a list.

Description of development approach

When speaking an object for FOCUS_ENTERED reason, if the object is a list, we do not discard keyboard shortcut anymore.

While at it, removed the accelerator of path selection in the "Create portable NVDA" dialog, since I have realized during the investigation for this PR that it was not working. A better solution would be to have a working key accelerator for this field but I do not know how to do this.

Testing strategy:

Manually tested the lists in #15816 (comment).

Known issues with pull request:

None

Code Review Checklist:

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

burmancomp · 2023-11-24T19:03:12Z

cc: @LeonarddeR, @Emil-18
Should it be displayed on braille line as well?

Emil-18 · 2023-11-24T19:10:25Z

I think it should be displayed in braille as well, in the same way it is displayed for the focus/navigator object

burmancomp · 2023-11-24T19:47:28Z

Related to braille.getPropertiesBraille?

LeonarddeR · 2023-11-24T20:13:16Z

Have you considered leaving out the list restriction. i.e. always report the shortcut key on focus entered? Can you think of any drawbacks that might cause?

Emil-18 · 2023-11-24T20:21:33Z

The only drawback I can think of is consumtion of time, but if it is that big of an issue for someone, they could just turn off the reporting of shortcut keys in settings anyway

CyrilleB79 · 2023-11-24T22:46:14Z

Regarding braille, I will not be able to test and work on it before next week. Is the full list also reported when the list item is selected?

Also, @LeonarddeR wrote:

Have you considered leaving out the list restriction. i.e. always report the shortcut key on focus entered? Can you think of any drawbacks that might cause?

I have restricted to list for two reasons:

I do not know how many types of container objects could trigger the focus entered event and I do not know if reporting the shortcut key is relevant for them. Said otherwise, I have almost no other example of container with a shortcut key.
The only other example I have is the path text control in a grouping, in the "Create portable" dialog. The grouping's label ("Portable directory") has the "D" accelerator and it would be reported if I did not filter on list. Unfortunately, this accelerator key actually does not work. The best solutions would be to make it work, but I do not know how. The second best solution is maybe to remove the accelerator if I cannot find a way to implement it. However, in other situations (e.g. add-ons), we cannot guarantee that grouping labels will not contain accelerator.

@LeonarddeR what do you think of that? Would you recommend something else?

To all:
Note: this PR is still in draft state and thus may be subject to modifications. Anyway, I appreciate your feedback even at this stage.

burmancomp · 2023-11-25T08:19:36Z

Regarding braille, I will not be able to test and work on it before next week. Is the full list also reported when the list item is selected?

List item is shown not list title. To see shortcut you must navigate to parent object.

LeonarddeR · 2023-11-25T15:18:39Z

2. The only other example I have is the path text control in a grouping, in the "Create portable" dialog. The grouping's label ("Portable directory") has the "D" accelerator and it would be reported if I did not filter on list. Unfortunately, this accelerator key actually does not work. The best solutions would be to make it work, but I do not know how. The second best solution is maybe to remove the accelerator if I cannot find a way to implement it. However, in other situations (e.g. add-ons), we cannot guarantee that grouping labels will not contain accelerator.

This doesn't only apply to cases in the NVDA interface, but also in other applications.
I figured that we could limit shortcut reporting to focusable ancestors, but then I noticed that the grouping in the portable directory dialog is reported as focusable, yet it isn't.
Makes sense to leave this as is.

CyrilleB79 · 2023-11-25T18:48:46Z

@burmancomp wrote:

Should it be displayed on braille line as well?

@Emil-18 wrote:

I think it should be displayed in braille as well, in the same way it is displayed for the focus/navigator object

In the end, I have been able to test with the braille viewer. It seems to me that in braille, the shortcut is already present. If you cannot see it, you will probably have to pane left to see the context.

burmancomp · 2023-11-25T18:57:13Z

In the end, I have been able to test with the braille viewer. It seems to me that in braille, the shortcut is already present. If you cannot see it, you will probably have to pane left to see the context.

Shortcut can be found using object navigation but when review mode is object review, it cannot be found by panning left.

CyrilleB79 · 2023-11-25T21:10:50Z

In the end, I have been able to test with the braille viewer. It seems to me that in braille, the shortcut is already present. If you cannot see it, you will probably have to pane left to see the context.

Shortcut can be found using object navigation but when review mode is object review, it cannot be found by panning left.

I am not speaking about object navigation. I am speaking about focus context. When I tab to the NVDA modifier list in the keyboard settings, I have the following line in the log:
Braille regions text: ['NVDA Settings: Keyboard (normal configuration) dlg ', 'Keyboard property page ', 'Select NVDA Modifier Keys lst Alt+s ', 'caps lock ⣏⣿⣹ chk']
We can see that the focus context indicates the list with its shortcut key ('Select NVDA Modifier Keys lst Alt+s ').

Of course, when the braille follows review, there is no focus context.

Could you clarify what you mean when you write "when review mode is object review"? Are you speaking of a specific option?

Or did I miss something else? In this case, please indicates the steps to reproduce, what you get and what you expect. Thanks.

AppVeyorBot · 2023-11-26T00:19:06Z

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/yqbjf4tyh8v55bgk/artifacts/output/nvda_snapshot_pr15826-30055,1b8aea4a.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
INSTALL_END 1.0,
BUILD_START 0.0,
BUILD_END 25.5,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 16.5,
FINISH_END 0.1

See test results for failed build of commit 1b8aea4afc

burmancomp · 2023-11-26T08:38:28Z

Of course, when the braille follows review, there is no focus context.

You are right. When braille is tethered to focus scrolling back shows shortcut. I use braille tethered to review. When braille is tethered to review, shortcut is not shown.

There may be some advantages to tether braille to review (and moving system caret when routing review cursor):

when in terminal windows (command prompt, putty) content can be reviewed without extra steps
when editing text you can scroll right/left/up/down with braille display without affecting caret, and if you want to move caret from its current location, you can do this with routing key.

I suppose this has been possible from ms-dos times, and currently with nvda as well.

It would be nice if shortcut were shown in braille also when braille is tethered to review.

CyrilleB79 · 2023-11-26T19:55:37Z

Of course, when the braille follows review, there is no focus context.

You are right. When braille is tethered to focus scrolling back shows shortcut. I use braille tethered to review. When braille is tethered to review, shortcut is not shown.

There may be some advantages to tether braille to review (and moving system caret when routing review cursor):
* when in terminal windows (command prompt, putty) content can be reviewed without extra steps

* when editing text you can scroll right/left/up/down with braille display without affecting caret, and if you want to move caret from its current location, you can do this with routing key.
I suppose this has been possible from ms-dos times, and currently with nvda as well.

It would be nice if shortcut were shown in braille also when braille is tethered to review.

@burmancomp, shortcut keys of the current navigator object are shown when braille is tethered to review, e.g. checkbox, combo-box. In the case of list items, they have no shortcut keys. And since there is no "Nav object context" similar to focus context, the list and its shortcut is not shown in review mode except when the nav object is actually the whole list.

To summarize, your request is not linked to this PR and I'd suggest that you open a specific new issue to clarify the need, what you get today and what you would expect.

michaelDCurran · 2023-11-26T21:10:01Z

source/speech/speech.py

@@ -774,7 +774,8 @@ def _objectSpeech_calculateAllowedProps(reason, shouldReportTextContent):
 	}
 	if reason in (OutputReason.FOCUSENTERED, OutputReason.MOUSE):
 		allowProperties["value"] = False
-		allowProperties["keyboardShortcut"] = False
+		if not objRole == controlTypes.Role.LIST:


I think we should speak shortcuts for any object that focus enters. I cannot remember specifically why we never originally did. If you can make an argument for lists, I think you can for any other container. Most likely this was simply that early on it was only done for focus and not focusEntered, and then when later abstracted, it was deliberately removed from focusEntered to match existing behaviour.

I think we should speak shortcuts for any object that focus enters. I cannot remember specifically why we never originally did. If you can make an argument for lists, I think you can for any other container. Most likely this was simply that early on it was only done for focus and not focusEntered, and then when later abstracted, it was deliberately removed from focusEntered to match existing behaviour.

@michaelDCurran, this has just been discussed with @LeonarddeR in #15826 (comment), #15826 (comment) and #15826 (comment).

The group container can have a shortcut key defined but actually not working. To avoid to get other such not working case, it seems safer to restrict this PR to the only known, frequent and useful case of lists. If in the future we can find other containers for which the shortcut key should be announced, we may include their role too.

Or should I instead exclude explicitly the case of grouping which is not working?

A third possibility can be to remove this filtering and consider that putting a shortcut key in grouping where it does not work should not be done in first place by the developer of the application.

@michaelDCurran, let me know which solution you would prefer.

Whether authors place a shortcut on groupings, or whether these shortcuts actually work on groupings in applications really is separate to this pr I feel. However, your current solution solves your immediate problem, and does not introduce other known problems. Therefore, could you please at least add a comment above this line stating why we only do it for list at this stage.

OK. Done in 43c500e.

seanbudd · 2023-11-26T22:50:40Z

source/speech/speech.py

@@ -745,7 +745,7 @@ def getObjectSpeech(
 	return sequence


-def _objectSpeech_calculateAllowedProps(reason, shouldReportTextContent):
+def _objectSpeech_calculateAllowedProps(reason, shouldReportTextContent, objRole):


please add type hints here

Done in acaeeee.

burmancomp · 2023-11-27T08:15:04Z

I'd suggest that you open a specific new issue to clarify the need, what you get today and what you would expect.

You are right. After thinking this, I agree. Showing list shortcut in braille when braille is tethered to review, requires considerationn (at minimum what and when should be displayed), and I have no opinion about this at the moment. So I also prefer separatehandling of showing list shortcut when braille is tethered to review. If on the whole it seems to be useful feature, I suppose that then I or somebody else will likely open issue or discussion.

source/speech/speech.py

Report keyboard shortcut when entering a list

aadac80

CyrilleB79 added 2 commits November 26, 2023 00:05

Remove unuseful accelerator

57df466

Update change log

667c0ca

bump CI

2a1d18b

CyrilleB79 marked this pull request as ready for review November 26, 2023 19:55

CyrilleB79 requested a review from a team as a code owner November 26, 2023 19:55

CyrilleB79 requested a review from michaelDCurran November 26, 2023 19:55

michaelDCurran requested changes Nov 26, 2023

View reviewed changes

Comment to explain why we only filter lists

43c500e

michaelDCurran approved these changes Nov 26, 2023

View reviewed changes

seanbudd reviewed Nov 26, 2023

View reviewed changes

Add type hints

acaeeee

seanbudd reviewed Nov 27, 2023

View reviewed changes

source/speech/speech.py Outdated Show resolved Hide resolved

add return type

9a05a44

seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Nov 27, 2023

seanbudd merged commit fb375e2 into nvaccess:master Nov 27, 2023
1 check passed

nvaccessAuto added this to the 2024.1 milestone Nov 27, 2023

CyrilleB79 deleted the accKeyList branch November 29, 2023 10:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report keyboard shortcut when entering a list #15826

Report keyboard shortcut when entering a list #15826

CyrilleB79 commented Nov 24, 2023 •

edited

burmancomp commented Nov 24, 2023

Emil-18 commented Nov 24, 2023

burmancomp commented Nov 24, 2023

LeonarddeR commented Nov 24, 2023

Emil-18 commented Nov 24, 2023

CyrilleB79 commented Nov 24, 2023

burmancomp commented Nov 25, 2023

LeonarddeR commented Nov 25, 2023

CyrilleB79 commented Nov 25, 2023

burmancomp commented Nov 25, 2023

CyrilleB79 commented Nov 25, 2023

AppVeyorBot commented Nov 26, 2023

burmancomp commented Nov 26, 2023

CyrilleB79 commented Nov 26, 2023

michaelDCurran Nov 26, 2023

CyrilleB79 Nov 26, 2023

michaelDCurran Nov 26, 2023

CyrilleB79 Nov 26, 2023

seanbudd Nov 26, 2023

CyrilleB79 Nov 27, 2023

burmancomp commented Nov 27, 2023

Report keyboard shortcut when entering a list #15826

Report keyboard shortcut when entering a list #15826

Conversation

CyrilleB79 commented Nov 24, 2023 • edited

Link to issue number:

Summary of the issue:

Description of user facing changes

Description of development approach

Testing strategy:

Known issues with pull request:

Code Review Checklist:

burmancomp commented Nov 24, 2023

Emil-18 commented Nov 24, 2023

burmancomp commented Nov 24, 2023

LeonarddeR commented Nov 24, 2023

Emil-18 commented Nov 24, 2023

CyrilleB79 commented Nov 24, 2023

burmancomp commented Nov 25, 2023

LeonarddeR commented Nov 25, 2023

CyrilleB79 commented Nov 25, 2023

burmancomp commented Nov 25, 2023

CyrilleB79 commented Nov 25, 2023

AppVeyorBot commented Nov 26, 2023

burmancomp commented Nov 26, 2023

CyrilleB79 commented Nov 26, 2023

michaelDCurran Nov 26, 2023

Choose a reason for hiding this comment

CyrilleB79 Nov 26, 2023

Choose a reason for hiding this comment

michaelDCurran Nov 26, 2023

Choose a reason for hiding this comment

CyrilleB79 Nov 26, 2023

Choose a reason for hiding this comment

seanbudd Nov 26, 2023

Choose a reason for hiding this comment

CyrilleB79 Nov 27, 2023

Choose a reason for hiding this comment

burmancomp commented Nov 27, 2023

CyrilleB79 commented Nov 24, 2023 •

edited