Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Report keyboard shortcut when entering a list #15826

Merged
merged 7 commits into from
Nov 27, 2023
Merged

Report keyboard shortcut when entering a list #15826

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
aadac80
Report keyboard shortcut when entering a list
CyrilleB79 Nov 24, 2023
57df466
Remove unuseful accelerator
CyrilleB79 Nov 25, 2023
667c0ca
Update change log
CyrilleB79 Nov 25, 2023
2a1d18b
bump CI
CyrilleB79 Nov 26, 2023
43c500e
Comment to explain why we only filter lists
CyrilleB79 Nov 26, 2023
acaeeee
Add type hints
CyrilleB79 Nov 27, 2023
9a05a44
add return type
seanbudd Nov 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion source/gui/installerGui.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ def __init__(self, parent):

# Translators: The label of a grouping containing controls to select the destination directory
# in the Create Portable NVDA dialog.
directoryGroupText = _("Portable &directory:")
directoryGroupText = _("Portable directory:")
groupSizer = wx.StaticBoxSizer(wx.VERTICAL, self, label=directoryGroupText)
groupHelper = sHelper.addItem(guiHelper.BoxSizerHelper(self, sizer=groupSizer))
groupBox = groupSizer.GetStaticBox()
Expand Down
14 changes: 11 additions & 3 deletions source/speech/speech.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -699,7 +699,7 @@ def getObjectSpeech(
or not obj._hasNavigableText
)

allowProperties = _objectSpeech_calculateAllowedProps(reason, shouldReportTextContent)
allowProperties = _objectSpeech_calculateAllowedProps(reason, shouldReportTextContent, obj.role)

if reason == OutputReason.FOCUSENTERED:
# Aside from excluding some properties, focus entered should be spoken like focus.
Expand Down Expand Up @@ -745,7 +745,11 @@ def getObjectSpeech(
return sequence


def _objectSpeech_calculateAllowedProps(reason, shouldReportTextContent):
def _objectSpeech_calculateAllowedProps(
reason: OutputReason,
shouldReportTextContent: bool,
objRole: controlTypes.Role,
) -> dict[str, bool]:
allowProperties = {
'name': True,
'role': True,
Expand Down Expand Up @@ -774,7 +778,11 @@ def _objectSpeech_calculateAllowedProps(reason, shouldReportTextContent):
}
if reason in (OutputReason.FOCUSENTERED, OutputReason.MOUSE):
allowProperties["value"] = False
allowProperties["keyboardShortcut"] = False
# #15826: For containers, there are cases where the shortcut key can be defined but not working (e.g.
# GROUPING). The safest strategy is then to remove the shortcut keys of containers except in the known
# cases where it is working and useful. The only such known case is the one of LIST.
if not objRole == controlTypes.Role.LIST:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Done in 43c500e.

allowProperties["keyboardShortcut"] = False
allowProperties["positionInfo_level"] = False
if reason == OutputReason.MOUSE:
# Name is often part of the text content when mouse tracking.
Expand Down
2 changes: 1 addition & 1 deletion user_docs/en/changes.t2t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ Users of Poedit 1 are encouraged to update to Poedit 3 if they want to rely on e
-

== Bug Fixes ==
- Reporting of object shortcut keys has been improved. (#10807, @CyrilleB79)
- Reporting of object shortcut keys has been improved. (#10807, #15816, @CyrilleB79)
- The SAPI4 synthesizer now properly supports volume, rate and pitch changes embedded in speech. (#15271, @LeonarddeR)
- Multi line state is now correctly reported in applications using Java Access Bridge. (#14609)
- In LibreOffice, words deleted using the ``control+backspace`` keyboard shortcut are now also properly announced when the deleted word is followed by whitespace (like spaces and tabs). (#15436, @michaelweghorn)
Expand Down