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

Cancellable speech #10885

Merged
merged 4 commits into from May 8, 2020
Merged

Cancellable speech #10885

merged 4 commits into from May 8, 2020

Conversation

feerrenrut
Copy link
Member

@feerrenrut feerrenrut commented Mar 19, 2020

Depends on refactoring in PR #11049, this diff will become easier to follow once that is merged.
Note: this fix is an experimental approach that must be turned on via the 'advanced settings panel' in the NVDA settings dialog.

Link to issue number:

Summary of the issue:

In some web applications (such as Gmail) where user interaction causes rapid focus events NVDA may speak outdated focus information. This can be reproduced in Gmail by quickly moving through email (in focus mode) using the down arrow. Sometimes two or three rows are reported from Gmail.

This issue happens more often in Chrome than in Firefox, but it can be reproduced in both browsers.

Description of how this pull request fixes the issue:

This PR introduces a cancellable speech command, which can be inserted into a speech sequence for speech that may be invalidated (such as due to a new focus event)

Testing performed:

Tested moving through emails (and across table boundaries) in Gmail.
Tested smoke tested web browsing.

Known issues with pull request:

Change log entry:

Changes:

 - Web applications (EG Gmail) no longer speak outdated information when moving focus rapidly

@josephsl
Copy link
Collaborator

josephsl commented Mar 19, 2020

Copy link
Collaborator

@leonardder leonardder left a comment

I did a small walk through this. At first glance, the cancelable speech is indeed pretty related to focus, whereas it could be more generic may be.

source/NVDAObjects/IAccessible/ia2Web.py Outdated Show resolved Hide resolved
source/NVDAObjects/IAccessible/ia2Web.py Outdated Show resolved Hide resolved
source/speech/__init__.py Outdated Show resolved Hide resolved
source/speech/__init__.py Outdated Show resolved Hide resolved
@feerrenrut
Copy link
Member Author

feerrenrut commented Mar 26, 2020

Just added a note to the description of this PR:

When reviewing this, several of the first commits are refactoring. It will be much easier to review if you limit the commit range.

Copy link
Member

@michaelDCurran michaelDCurran left a comment

Firstly this diff was very hard to read with all the refactoring going on. True I could look at commits individually, but I do feel this sort of suggests that refactoring would have been better in a separate pr first if it were truly necessary.
As for the actual changes needed in this pr: if I understand correctly, in simple terms, if a speech utterance was produced due to a gainFocus event, it will contain a cancellableSpeechCommand. This cancellableSpeechCommand will show itself as 'canceled' if the object the event was for was definitly queued as a focus event at some point, and now is not the currently queued focus event. Is my understanding correct?
There are some extra changes to do with focus on this pr, such as the addition of focusInfo and isGainFocusValid. But I cannot see where these are actually ever called... unless NVDA / github are not playing nicely for me...
If they are not called, then they should be obviously removed.
I can confirm that this approach does solve the issue in gmail.

source/NVDAObjects/IAccessible/ia2Web.py Outdated Show resolved Hide resolved
source/NVDAObjects/IAccessible/ia2Web.py Outdated Show resolved Hide resolved
@feerrenrut
Copy link
Member Author

feerrenrut commented Apr 14, 2020

@michaelDCurran Your summary sounds correct to me. I'll clean up those unused functions and split the PR into a refactor + feature. Thanks for reviewing it.

@AppVeyorBot

This comment has been minimized.

@feerrenrut feerrenrut marked this pull request as ready for review Apr 23, 2020
feerrenrut added a commit that referenced this pull request Apr 28, 2020
Merge remote-tracking branch 'origin/pr/11049'

PR #10885 introduces a way to cancel speech due to focus events that are no
longer valid. However, the refactoring commits made the diff hard to view.

Some static analysis tools did not understand several classes due to their order
in the file. This change re-orders those classes so they are defined before
they are used. It also makes that code pass the flake8 checks. The move
and restyling are done in separate commits to make revisiting the
changes easier.
Turning off white space in the diff will also make this easier.

- Speech manager changes:
  - Explicit imports
  - More typing information added to function definitions.
- synthDriverHandler changes:
  - The order of classes. Now defined before they are used.
feerrenrut added 2 commits Apr 28, 2020
This experimental feature can be controlled with a flag in the advanced
settings panel.

In eventHandler.py keep track of objects that have had focus (on the object, using
attribute 'wasGainFocusObj').
This expands on the 'lastQueuedFocusObject' concept.

A cancellable speech command is added to speech sequences that result
from the focus change event.
This command is able to check if the object once had focus
('wasGainFocusObj') and if it still has focus.
Speech for objects that no longer have focus can be discarded,
or cancelled if already with the synth.

Then checking for cancellations is done both early (eventHandler) and late
(speech.manager.speak)

This requires some careful tracking and processing in speech manager.
When nothing is with the synth, try pushing more.
Otherwise, there are items in the queue but nothing being spoken.
When cancelling synth clear tracking of "with synth"

Use _removeCompletedFromQueue rather than _handleIndex
_handleIndex may not actually call _removeCompleted. Cancelled speech does
need it's callbacks called.

For now CancellableSpeechCommand should be considered a private API
@feerrenrut
Copy link
Member Author

feerrenrut commented Apr 28, 2020

Refactor for cancellable speech #11049 has now been merged and this PR rebased. It is now ready to be reviewed again.

@AppVeyorBot

This comment has been minimized.

@Adriani90
Copy link
Collaborator

Adriani90 commented May 3, 2020

I think this will probably fix #6421

michaelDCurran
michaelDCurran previously requested changes May 7, 2020
Copy link
Member

@michaelDCurran michaelDCurran left a comment

I really don't think changes to NVDAObjects\IAccessible\__init__.py and NVDAObjects\IAccessible\Ia2web.py are necessary in this pr.
However, I am happy with the rest of it.

@feerrenrut feerrenrut dismissed michaelDCurran’s stale review May 8, 2020

All actions are addressed.

@feerrenrut feerrenrut merged commit 9967903 into master May 8, 2020
1 check was pending
@feerrenrut feerrenrut deleted the cancellableSpeech branch May 8, 2020
@nvaccessAuto nvaccessAuto added this to the 2020.2 milestone May 8, 2020
@codeofdusk

This comment has been minimized.

@feerrenrut

This comment has been minimized.

@codeofdusk

This comment has been minimized.

@codeofdusk

This comment has been minimized.

@feerrenrut

This comment has been minimized.

@feerrenrut

This comment has been minimized.

@feerrenrut
Copy link
Member Author

feerrenrut commented May 8, 2020

Created issue: #11132

I'll look at fixing this early next week. If we can't fix this issue before branching for beta the PR will have to be reverted.

@Adriani90

This comment has been minimized.

@feerrenrut

This comment has been minimized.

@Adriani90
Copy link
Collaborator

Adriani90 commented May 8, 2020

@feerrenrut before creating a new issue, I just wanted to share that enabling this option causes NVDA to skip reporting of window title or tab title when switching with alt+tab or with ctrl+tab. Maybe it is possible to limit the cancelable speech to the elements of the window except for address bars, status bars, notification bars and window titles.
If this does not get reverted next week, I will create a new issue for this specific problem.

@feerrenrut
Copy link
Member Author

feerrenrut commented May 11, 2020

Note: #11144

@ramchaitanyap
Copy link

ramchaitanyap commented Nov 16, 2020

Lets say, I change focus by some timeout, NVDA is still not cancelling expired focus speech.

function getFocus() {
document.getElementById("myAnchor").focus();
setTimeout(() => document.getElementById("myAnchor2").focus(), 500);
}

Its still reading #myAnchor text and then #myAnchor2 text

@seanbudd seanbudd added the deprecated/2021.1 Label used to track deprecations due for removal in the 2021.1 release label Mar 5, 2021
feerrenrut added a commit that referenced this pull request Mar 17, 2021
First introduced with "Cancellable speech #10885"
Several issues fixed with "Fix several issues in speech manager #11245"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecated/2021.1 Label used to track deprecations due for removal in the 2021.1 release feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants