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

Detect caret movement using events, except for IA2Web #9933

Merged
merged 6 commits into from
Jul 24, 2019
Merged

Detect caret movement using events, except for IA2Web #9933

merged 6 commits into from
Jul 24, 2019

Conversation

LeonarddeR
Copy link
Collaborator

@LeonarddeR LeonarddeR commented Jul 16, 2019

Link to issue number:

Fixes #9928
Follow up of #9773

Summary of the issue:

In text fields, when moving the caret with caret movement commands, such as the arrows, NVDA relies on bookmarks to find out whether the caret has moved. In short:

  1. You press left arrow
  2. NVDA creates a bookmark of the current caret position
  3. NVDA executes left arrow
  4. For up to the caret movement timeout, NVDA tries to find out whether the caret has moved by creating new bookmarks and comparing them against the old.

However, for UIA, this comparison fails, as creating a bookmark at the end of a range and then removing one character from the end of the range results in a new bookmark that is equal to the former.

Description of how this pull request fixes the issue:

This pr introduces a different approach based on code by @codeofdusk in #9773. In first instance, this code caused #9786 to occur (i.e. editors in Chrome reporting the wrong caret position).

Compared to #9773, flow is now as follows:

  1. In a loop in EditableText._hasCaretMoved, NVDA processes pedning events
  2. If there is a focus event pending, the loop is exited
  3. NVDA tries to find out whether the caret position has changed by creating a textInfo at the caret position
  4. new code: only when this succeeds, NVDA checks for pending caret and textChange events. If the object that contains the caret has caretMovementDetectionUsesEvents set to False, it still ignores the caret events. This is what we do in IA2Web objects.

Testing performed:

  1. Tested that deleted characters/words are again reported in UIA controls, such as MS Word
  2. Tested Notepad, Wordpad, Word Without UIA, Thunderbird, LibreOffice, start menu edit field, legacy command consoles and Skype Electron. Made sure that the caret was still correctly reported in all of them when moving with arrow keys or deleting.

Known issues with pull request:

We can't set the caretMovementDetectionUsesEvents attribute to True on editableText.EditableText and then override it in other places, such as IA2Web. This is because EditableText precedes the IA2Web class as well as other classes in the mro Setting it on EditableText directly therefore makes it impossible for other classes to override the attribute. I solved this by making it a magic property on EditableText that first tries to call super before returning the default.

Change log entry:

None

@LeonarddeR LeonarddeR requested a review from feerrenrut July 16, 2019 06:34
@codeofdusk
Copy link
Contributor

  • Re name, I think useCaretEvents is probably descriptive enough if documented.
  • In the Windows 10 start menu search field, backspacing characters no longer seems to announce anything.
  • In UIA consoles, backspacing quickly causes the last character of the prompt (i.e. ">" for cmd) to be occasionally reported.

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Jul 16, 2019 via email

@LeonarddeR
Copy link
Collaborator Author

Apart from the start menu issue, all seems fixed now. I need to investigate that on Master builds though, I'm pretty sure it has never worked very well.

@LeonarddeR
Copy link
Collaborator Author

The code now also listens for textChange events, as they help in the case where text is removed. This fixes the start menu issue for me.

@LeonarddeR
Copy link
Collaborator Author

@feerrenrut: I think it is pretty hard to make sure we have everything right in this pr. THerefore, I propose reviewing and merging it as soon as possible, then patching it afterwards for cases that have problems with this new behavior.

@codeofdusk
Copy link
Contributor

For a change log entry, how about something like:

== Bug Fixes ==

@codeofdusk
Copy link
Contributor

UIA consoles are indeed fixed. Start menu says "blank" once when backspacing the last character, then nothing. Ideally it would function like the Windows run dialog and console (i.e. say nothing when all characters have been deleted) but this is still an improvement.

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Jul 17, 2019 via email

@codeofdusk
Copy link
Contributor

I don't think anything has improved. It regressed with the use of python 3 and had to be implemented differently. Unless you know a particular improvement I missed?

Current master says "blank" repeatedly when deleting characters in the Windows 10 start search field. This new approach only says it once, and maybe could be made to say it not at all?

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Jul 17, 2019 via email

@codeofdusk
Copy link
Contributor

Hmm that improves things indeed. The fact that this fails in master makes exactly clear why using bookmarks with UIA is not the reliable way to go. I'm afraid that NVDA receives a textChange event when pressing backspace the first time. after the field is blank. I will look at this briefly.

Once it's fixed, maybe a similar approach can be used for UIA consoles as well? (so we don't have to disable the new approach)

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Jul 17, 2019 via email

@LeonarddeR
Copy link
Collaborator Author

The problem in consoles where the last character of the prompt is reported can also be seen in legacy consoles. This really has to do with events coming in pretty late, i.e. pressing backspace results in an event that is received by NVDA around half a second later.

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Jul 17, 2019

After the most recent commit, I could no longer reproduce the issue in consoles and therefore I restored the behavior for UIA consoles. In the start menu search field, it still can be observed, though you'd have to remove text pretty quicly. I don't think there's anything we can use to improve this, as said, events are coming pretty late.

Copy link
Contributor

@feerrenrut feerrenrut left a comment

Choose a reason for hiding this comment

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

Thanks @LeonarddeR and @codeofdusk. Overall this looks good, just pending some clarification about the timeout comment.

else:
# Caret events are unreliable in some controls.
# Only use them if we consider them safe to rely on for a particular control,
# and only if they arrive within 20 mili seconds after causing the event to occur.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment says 20 ms but useEvents_maxTimeoutMs is 10

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I'm sorry. I first hard coded the 20 ms, but then decided to put it into a constant. Just forgot to change it :)

@LeonarddeR LeonarddeR requested a review from feerrenrut July 24, 2019 17:24
@feerrenrut feerrenrut merged commit 75c8d00 into nvaccess:threshold Jul 24, 2019
@codeofdusk
Copy link
Contributor

As of latest master, I can still reproduce the issues with UIA consoles, although they occur much less frequently.

@LeonarddeR
Copy link
Collaborator Author

How about increasing the _useEvents_maxTimeoutMs timeout in editabletext?

@codeofdusk
Copy link
Contributor

How about increasing the _useEvents_maxTimeoutMs timeout in editabletext?

Tried 20, 30, and 40 ms with no success.

feerrenrut pushed a commit that referenced this pull request Jul 31, 2019
A regression caused during Python 3 migration.
NVDA was announcing the final character of the prompt after backspacing text quickly.
This can result in NVDA announcing "greater" after backspacing a line of text.

Disable caret events in the UIA console and legacy consoles.
This PR disables the caret movement event detection from #9933
feerrenrut pushed a commit that referenced this pull request Aug 22, 2019
In Mintty, the last character of the prompt was being read when quickly deleting text, just as in Windows Console (see this comment and the "summary of the issue" section in #9986).

This change disables caret events for all Terminal objects by overriding NVDAObjects.behaviors.Terminal._get_caretMovementDetectionUsesEvents, instead of only Windows Console like in #9986.

Closes #1348.
Related to #9933, #9986.
@LeonarddeR LeonarddeR added the BabbageWork Pull requests filed on behalf of Babbage B.V. label Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BabbageWork Pull requests filed on behalf of Babbage B.V.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants