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.

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

Makes the timeout for multiple keypress configurable #12049

Merged
merged 14 commits into from
Jul 30, 2024

Conversation

CyrilleB79
Copy link
Collaborator

@CyrilleB79 CyrilleB79 commented Feb 8, 2021

Link to issue number:

Fixes #11929.

Summary of the issue:

Some NVDA keystrokes can perform various commands according to the number of times the keystrokes is executed. E.g. NVDA+Numpad2 reports the current character under the review cursor with one key press. But a double key press reports the character description. And a tripe key press reports the character code.
Currently, double or triple key press should executed quite quickly to be considered a multiple key press: a maximum of 0.5 s is required between two key presses.
This may be difficult or even impossible for some people having physical disabilities and/or using sticky keys.

Description of user facing changes

This PR adds an option in the keyboard settings panel to allow to configure the timeout for multiple keypress.

  • The default value is the same as the current timeout, i.e. 500 ms.
  • The max value is 20000 s (= 20 s). I have taken a value that seemed quite exaggerated to me on purpose in order to be sure not to penalize anyone. I imagine that someone who is not able to type two keys in less than 20 seconds may not use a standard keyboard at all. But in any case, a real-world feedback is welcome.

Description of development approach

Added a value in ms in the GUI and in the config spec.

In this PR, a first attempt was made with a value in seconds, but it was blocked by an external issue wxWidgets/wxWidgets#19222

Testing strategy:

Manual tests:

  • Tested that the option accepts only values between 100 and 20000
  • Tested various values and tried to perform double press commands to check that the configuration was applied.

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.

Summary by CodeRabbit

  • New Features

    • Users can now configure the maximum delay between repeated keypresses, enhancing accessibility for individuals with dexterity impairments.
    • Improvements to keyboard commands in Microsoft Word and Excel, allowing double-press actions to display comments and notes in a browsable format.
  • Documentation

    • Updated user guide with a new section detailing the configurable maximum delay feature, emphasizing its benefits for users with physical disabilities.
  • Bug Fixes

    • Enhanced logic for script execution timing, providing better control over keypress actions.

@CyrilleB79
Copy link
Collaborator Author

Cc @josephsl, maybe you would have any advice on how to investigate the wx.SpinCtrlDouble issue given #8517 (comment) ?

@AppVeyorBot
Copy link

See test results for failed build of commit 4ca7f331f9

@CyrilleB79
Copy link
Collaborator Author

Hi @seanbudd
First of all, welcome to your new job!
I have seen in #12115 (comment) that you raised ticket against WxPython/Phoenix. May you also have a look to the issue with wx.SpinCtrlDouble described in the current PR?
It would be interesting to open an issue with WxPython regarding this component, but I do not know how to indicate to WxPython people what precisely is not working.

Also @josephsl, please comment here, especially if you have already tested this component as you write in #8517 (comment); if you just did not test it, please also let us know.

@seanbudd
Copy link
Member

Hi @CyrilleB79

Thanks for the welcome! I can do an investigation - it might take sometime. There's been a fair few WxPython issues raised in response to the latest release (which we recently incorporated into master with python3.8).

Just to get started I found this ancient ticket on WxWidgets that seems related http://trac.wxwidgets.org/ticket/12004. The bug might be fixed by merging the new master with python3.8 into your branch - but this might also introduce some other Wx issues.

@seanbudd
Copy link
Member

seanbudd commented Mar 16, 2021

Confirmed that this is still broken on the latest WxPython and I've opened a ticket here, http://trac.wxwidgets.org/ticket/19102

@lukaszgo1
Copy link
Contributor

I don't think http://trac.wxwidgets.org/ticket/19102 describes the actual problem that @CyrilleB79 mentioned in the PR description. Labels are by design not reachable in the tab order so this is expected. In this particular case text is not associated with the text control because AFAIK MSAA considers only text which is directly to the left of the control as its label. In case of WX's spin control this assumption does not hold as it is contained in the additional, unnamed, window. This can be easily confirmed by turning 'simple review mode' off an inspecting hierarchy with the object nav. I've tested the example and it works as expected in WXPython 2.9.1 which I'm using for a different project so this appears to be a regression on the WX Vidgets side. As to how to fix this I propose to annotate the control using accPropServer with the name and then remove NVDAObjects.behaviors.Dialog from the MRO of the settingsPanel, to ensure that the label which according to NVDA isn't associate with any control is not reported when tabbing from the categories list.

@seanbudd
Copy link
Member

Ah that makes a lot more sense, I've closed that ticket then.

@lukaszgo1
Copy link
Contributor

@CyrilleB79 Have you had time to investigate the approach that I've proposed in this comment? There is no rush obviously I'm just curious if my idea works :-)

@CyrilleB79
Copy link
Collaborator Author

Thanks @lukaszgo1 for your first comment and your reminder. I do not have a good knowledge of accessibility APIs (specifically MSAA here) and thus, I left a bit this PR aside.

Today I have upmerged from master branch. I have also removed the code for spin control value selection when it is focused since this seems to be automatic now. And I have made some tries to have the component named, but without success.

Let me add my reactions to what you wrote:

In this particular case text is not associated with the text control because AFAIK MSAA considers only text which is directly to the left of the control as its label. In case of WX's spin control this assumption does not hold as it is contained in the additional, unnamed, window. This can be easily confirmed by turning 'simple review mode' off an inspecting hierarchy with the object nav.

Strangely, inspecting the hierarchy with object nav, wx.SpinCtrlDouble has this container window containing edit field and spin buttons, whereas wx.SpinCtrl creates 2 sibling windows, the one containing the edit field and the one containing the spin button (= the up and down buttons). Why this difference.

I've tested the example and it works as expected in WXPython 2.9.1 which I'm using for a different project so this appears to be a regression on the WX Vidgets side.

Is there already a ticket for this issue? If not, would you accept to open it? You have the ideas much clearer than I on this topic and on the accessibility API more generally.

As to how to fix this I propose to annotate the control using accPropServer with the name and then remove NVDAObjects.behaviors.Dialog from the MRO of the settingsPanel, to ensure that the label which according to NVDA isn't associate with any control is not reported when tabbing from the categories list.

Regarding NVDAObjects.behaviors.Dialog's removal from MRO, it seems that it is not required anymore, probably due to wxPython upgrade or one of the various related PRs that have followed. Indeed the fact that the label was reported when tabbing from the category list to the keyboard settings panel seems to have disappeared after the upmerge.

Regarding the control annotation, I have understood that we should now use wx.Accessible to annotate controls rather than accPropServer following PR #12215.

In either case I do not know how to implement the solution, either the one proposed by you @lukaszgo1 or the one indicated in PR #12215 change log:

the gui.accPropServer module as well as the AccPropertyOverride and ListCtrlAccPropServer classes from the gui.nvdaControls module have been removed in favor of WX' native support for overriding accessibility properties. When enhancing accessibility of WX controls, implement wx.Accessible instead

I have tried to implement wx.Accessible but I can only make it apply to the container window, not to the edit field.
The non-working code is as follows:

class SpinCtrlDoubleAccessible(wx.Accessible):
	"""WX Accessible implementation for SpinCtrlDouble which aren't fully accessible."""

	def GetName(self, childId):
		if childId == winUser.CHILDID_SELF:
			return (wx.ACC_OK, self.Window.GetName())
		return super().GetName(childId)


class CustomSpinCtrlDouble(wx.SpinCtrlDouble):
	"""Custom spin control double to fix a11y bugs in the standard wx spin control double."""

	def __init__(self, *args, **kwargs):
		super(CustomSpinCtrlDouble, self).__init__(*args, **kwargs)
		# Register a custom wx.Accessible implementation to fix accessibility incompleties
		self.SetAccessible(SpinCtrlDoubleAccessible(self))



@LeonarddeR do you have any advice?

@CyrilleB79 CyrilleB79 closed this Apr 30, 2021
@CyrilleB79 CyrilleB79 reopened this Apr 30, 2021
@@ -68,22 +68,6 @@ def sendListItemFocusedEvent(self, index):
evt.Index = index
self.ProcessEvent(evt)

class SelectOnFocusSpinCtrl(wx.SpinCtrl):
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless you expect this to be merged before 2021.1 this class should stay since some add-ons may rely on it (certainly one of mine does). Since we don't know if selecting content of spin controls by default was a deliberate decision on the WX Vidgets side it would be good to have a system test ensuring that this behaviour persists when we update wxPython. My preference would be for both removal of this class and eventual automated test to happen in a separate pr.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unless you expect this to be merged before 2021.1 this class should stay since some add-ons may rely on it (certainly one of mine does). Since we don't know if selecting content of spin controls by default was a deliberate decision on the WX Vidgets side it would be good to have a system test ensuring that this behaviour persists when we update wxPython. My preference would be for both removal of this class and eventual automated test to happen in a separate pr.

Good point. Thanks. When the remaining issue of this PR will be solved, I will:

  • either add an additional note about deprecation in the change log if this PR can still be included in 2021.1
  • or revert the commit that removed the auto-select component if this PR may not be merged in 2021.1 anymore

@lukaszgo1
Copy link
Contributor

I've tested the example and it works as expected in WXPython 2.9.1 which I'm using for a different project so this appears to be a regression on the WX Vidgets side.

Is there already a ticket for this issue? If not, would you accept to open it? You have the ideas much clearer than I on this topic and on the accessibility API more generally.

I'll need to check if this also occurs in the wx Vidgets itself and probably create a C++ sample demonstrating the problem;. I'll try to set some time aside for this in the coming days.

As to how to fix this I propose to annotate the control using accPropServer with the name and then remove NVDAObjects.behaviors.Dialog from the MRO of the settingsPanel, to ensure that the label which according to NVDA isn't associate with any control is not reported when tabbing from the categories list.

Regarding NVDAObjects.behaviors.Dialog's removal from MRO, it seems that it is not required anymore, probably due to wxPython upgrade or one of the various related PRs that have followed. Indeed the fact that the label was reported when tabbing from the category list to the keyboard settings panel seems to have disappeared after the upmerge.

Regarding the control annotation, I have understood that we should now use wx.Accessible to annotate controls rather than accPropServer following PR #12215.

In either case I do not know how to implement the solution, either the one proposed by you @lukaszgo1 or the one indicated in PR #12215 change log:

the gui.accPropServer module as well as the AccPropertyOverride and ListCtrlAccPropServer classes from the gui.nvdaControls module have been removed in favor of WX' native support for overriding accessibility properties. When enhancing accessibility of WX controls, implement wx.Accessible instead

I have tried to implement wx.Accessible but I can only make it apply to the container window, not to the edit field.

I've attempted to associate wx.Accessible with the first children of this window i.e. the edit field containing the numeric value and while the Accessible seems to be registered properly correct name is still not returned. I'll keep digging.

@CyrilleB79
Copy link
Collaborator Author

Thanks @lukaszgo1 for your comments, your help and your investigations.

I'll need to check if this also occurs in the wx Vidgets itself and probably create a C++ sample demonstrating the problem;. I'll try to set some time aside for this in the coming days.

Thanks a lot!

As to how to fix this I propose to annotate the control using accPropServer with the name and then remove NVDAObjects.behaviors.Dialog from the MRO of the settingsPanel, to ensure that the label which according to NVDA isn't associate with any control is not reported when tabbing from the categories list.

I've attempted to associate wx.Accessible with the first children of this window i.e. the edit field containing the numeric value and while the Accessible seems to be registered properly correct name is still not returned. I'll keep digging.

How do you get the first child of this window and how do you know it is the edit field? (just for my information)

@CyrilleB79 CyrilleB79 closed this May 3, 2021
@CyrilleB79 CyrilleB79 reopened this May 3, 2021
@lukaszgo1
Copy link
Contributor

I now understand why the correct name is not returned even if the wx.Accessible is associated with the edit control. In short edit controls don't know their name when accessed via MSAA (see here and they asks they parent about the label of the static text which precedes them in the window. I've no idea how to fix this with wx.Accessible so unless someone more knowledgeable has an idea I'm afraid it needs to await a resolution in the wx Vidgets itself.

How do you get the first child of this window and how do you know it is the edit field? (just for my information)

I've just printed self.mySpinCtrlDouble.GetChildren() to the console.

@lukaszgo1
Copy link
Contributor

lukaszgo1 commented Jul 8, 2021

I've reported this against wx at: wxWidgets/wxWidgets#19222
providing a wxPython sample for now.

@CyrilleB79
Copy link
Collaborator Author

@seanbudd, you have just added the blocked/needs-external-fix label.
Thinking again to it and since wxWidgets/wxWidgets#19222 does not seem to go forward, I find that it's a pity to block people who need to change this timeout just for a GUI issue.

Rather than waiting for wx to find a solution to make wxSpinCtrlDouble fully accessible, we may think to other less perfect solutions in order to close #11929 anyway. I can think of the following alternatives (ordered according to my own preference):

  1. Use wxSpinCtrl instead of wxSpinCtrlDouble as done in the braille panel for blinking rate option. And use in the GUI as well as in the config a value in millisecond.
  2. Use a combo-box with a subset of possible values.
  3. Get rid of the GUI modifications in this PR and let people modify the configuration via the console (or via a custom ad-on to be created)

What do you and all think of these suggestions?
Thanks.

@XLTechie
Copy link
Collaborator

XLTechie commented Jul 16, 2022 via email

@CyrilleB79
Copy link
Collaborator Author

Can you prompt a try build for this PR? From the description alone, it is hard to understand why the issue is so significant as to be blocking. If it is just the absence of a label, could not a separate static text control be provided first to explain the data entry? But I suspect I'm misunderstanding it, which is why I'd like to observe it.

@XLTechie, I have pushed an empty commit to re-trigger an appVeyor build.

@CyrilleB79
Copy link
Collaborator Author

Can you prompt a try build for this PR? From the description alone, it is hard to understand why the issue is so significant as to be blocking. If it is just the absence of a label, could not a separate static text control be provided first to explain the data entry? But I suspect I'm misunderstanding it, which is why I'd like to observe it.

@XLTechie, I have pushed an empty commit to re-trigger an appVeyor build.

@XLTechie, here is a second attempt to generate a new build. I have merged with updated master branch. Hope that the build can be generated now.

@seanbudd
Copy link
Member

seanbudd commented Jul 18, 2022

@CyrilleB79 - Now that I have had a chance to write this issue up, I would consider this feature blocked by #13915 rather than wxWidgets/wxWidgets#19222.

@seanbudd
Copy link
Member

seanbudd commented Aug 8, 2022

Upon later reflection, perhaps this shouldn't be blocked by #13915. Use-cases which require a separate option may not have been fully considered as there is a difference between this feature, and the general concept of "idle speech". I would still consider it a "nice to have" to get the option in #13915 first, so we can test if it is appropriate to use the same setting for this PR.

@seanbudd seanbudd added the merge-early Merge Early in a developer cycle label Oct 10, 2022
@@ -1977,6 +1977,24 @@ def makeSettings(self, settingsSizer):
self.bindHelpEvent("KeyboardSettingsHandleKeys", self.handleInjectedKeysCheckBox)
self.handleInjectedKeysCheckBox.SetValue(config.conf["keyboard"]["handleInjectedKeys"])

minDelay = int(
config.conf.getConfigValidation(("keyboard", "maxRepeatedKeyPressDelay")).kwargs["min"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same thing here (and in other places). Even if the rest of it stays the same, I would recommend "timeout" over "delay".

user_docs/en/userGuide.md Outdated Show resolved Hide resolved
user_docs/en/userGuide.md Outdated Show resolved Hide resolved
user_docs/en/userGuide.md Outdated Show resolved Hide resolved
Co-authored-by: Luke Davis <8139760+XLTechie@users.noreply.github.com>
@CyrilleB79 CyrilleB79 changed the title Makes the maximum delay for key repetition configurable Makes the timeout for multiple keypress configurable Jul 24, 2024
@CyrilleB79
Copy link
Collaborator Author

@XLTechie your review is totally relevant and very appreciated. Many thanks!
Especially regarding English wording, do not refrain from suggesting rewording even it sometimes seems a bit picky. Well worded GUI and documentation is really a plus for end users as well as for translators.

For example regarding "delay" vs "timeout": the initial issue was mentioning "delay" and I have taken this word as is. In my native language (French), there is no single word for "timeout", and "delay" and "timeout" are translated more or less the same. Though, the meaning of these two words is clearly different and after your suggestion, it was clear for me that the most (or even only) suitable word was "timeout".

@AppVeyorBot
Copy link

See test results for failed build of commit 6ff923382b

@XLTechie
Copy link
Collaborator

Thanks @CyrilleB79. I'll review again, if you're done making changes.

@CyrilleB79
Copy link
Collaborator Author

Yes, you can review again, I'm done.

source/gui/settingsDialogs.py Outdated Show resolved Hide resolved
source/scriptHandler.py Outdated Show resolved Hide resolved
source/scriptHandler.py Outdated Show resolved Hide resolved
source/scriptHandler.py Outdated Show resolved Hide resolved
CyrilleB79 and others added 2 commits July 25, 2024 09:53
Co-authored-by: Luke Davis <8139760+XLTechie@users.noreply.github.com>
] and scriptFunc == lastScriptRef:
if (
(scriptTime - _lastScriptTime) * 1000 <= config.conf["keyboard"]["multiPressTimeout"]
and scriptFunc == lastScriptRef
Copy link
Collaborator

Choose a reason for hiding this comment

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

@CyrilleB79 you asked what I meant about "preferred linting" or some such phrase I used. I meant, NVDA's preferred coding style.
Specifically, we used to have a rule, that conditionals should start new and/or clauses on new lines.

However, I checked codingStandards.md, and this rule is no longer extant.
Don't know why it was removed, unless it's in some other file, but it probably happened in the recent docs re-org. @seanbudd did I miss that we stopped caring about this, or is it somewhere else?

@AppVeyorBot
Copy link

See test results for failed build of commit d009a72d1d

source/scriptHandler.py Outdated Show resolved Hide resolved
Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

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

Some thoughts on documentation and linting

source/gui/settingsDialogs.py Outdated Show resolved Hide resolved
user_docs/en/userGuide.md Outdated Show resolved Hide resolved
user_docs/en/userGuide.md Outdated Show resolved Hide resolved
user_docs/en/userGuide.md Outdated Show resolved Hide resolved
Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

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

Some thoughts on documentation and linting

user_docs/en/userGuide.md Outdated Show resolved Hide resolved
CyrilleB79 and others added 2 commits July 29, 2024 10:18
Also taking this suggestion (oversight).

Co-authored-by: Sean Budd <seanbudd123@gmail.com>
Copy link
Member

@Qchristensen Qchristensen left a comment

Choose a reason for hiding this comment

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

Docs read well, this will be a good chance, thank you

@seanbudd seanbudd merged commit 5d04b49 into nvaccess:master Jul 30, 2024
1 check was pending
@CyrilleB79 CyrilleB79 deleted the doubleKeyPressDelay branch July 30, 2024 10:40
SaschaCowley pushed a commit that referenced this pull request Oct 1, 2024
Fix-up of #12049 

Summary of the issue:
When implementing multi keypress timeout in #12049, the timeout for double press of NVDA key (to execute native function) was not honoured.

Description of user facing changes
Multiple key press timeout will now be honoured also for double press of NVDA key.

Description of development approach
Replace the hard-coded value 0.5 with the config value.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. merge-early Merge Early in a developer cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove time limit for "Press twice quickly" or "Press three times quickly" commands
7 participants