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

Use auto select detection for UIA when listening for TextChanged and TextSelectionChanged events #9660

Merged
merged 12 commits into from
Sep 10, 2019

Conversation

LeonarddeR
Copy link
Collaborator

@LeonarddeR LeonarddeR commented Jun 1, 2019

Link to issue number:

Fixes #9749
Fixes #7554

Summary of the issue:

  1. When editing in Excel, braille doesn't always follow correctly. This also happens in other UIA text fields, such as the search field in Win 10 settings.

  2. Suggestions aren't read in the non-chromium Microsoft Edge address bar.

As of pr #9614, on Windows 10, UIA TextChanged events are once again received by NVDA. They were once disabled in 9025c04. As part of this commit, we switched from the NVDAObjects.behaviors.EditableTextWithAutoSelectDetection ovrlay to the EditableTextWithoutAutoSelectDetection overlay for UIA. This was a necessary step, as text change and selection change events were no longer received from UIA.

Description of how this pull request fixes the issue:

We are already listening for TextChange events. This pr also requests TextSelectionChange events as before 9025c04. A, but only for Windows 10. When on Windows 10, UIA text controls now use the EditableTextWithAutoSelectDetection overlay.

Testing performed:

  1. Tested that in UIA text controls, such as in the Windows start menu and settings app, selection is reported as expected. Value changes are ignored correctly.
  2. Tested selection support in Word with UIA enabled.3. Tested that braille follows much more accurately when editing Excel cell contents.
  3. According to @josephsl, suggestions in the Edge address bar are now handle correctly.

Known issues with pull request:

None

Change log entry:

It is probably a bit difficult to come up with an entry that is understandable by end users.

Copy link
Collaborator

@josephsl josephsl left a comment

Choose a reason for hiding this comment

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

Hi,

One additional bug fix: address omnibar in EdgeHTML-based Edge is announced correctly if there are suggestions.

Two things observed when testing it with Edge:

  1. In GitHub edit fields, typed text is announced despite my setting to turn typed characters/words off.
  2. On some websites (such as Google), selection isn't announced if I use focus mode inside an edit field and attempt to select text.

Tested with PR-9660 binary build from Appveyor.

Thanks.

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Jun 1, 2019 via email

@josephsl
Copy link
Collaborator

josephsl commented Jun 1, 2019 via email

@josephsl
Copy link
Collaborator

josephsl commented Jun 1, 2019

Hi,

EdgeHTML-based Edge issues are fixed. It breaks in Excel, however:

  1. Open Excel.
  2. When a new sheet opens, press F2 to edit a cell.
  3. Locate the just edited cell and press F2 to edit it again.
  4. Try selecting cell contents by using Shift+arrow keys.

Expected: selected character/word/line, etc. are announced.
Actual: no announcement.

Tested on Excel 365.
No problem with Outlook at the moment.

Thanks.

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Jun 1, 2019 via email

@codeofdusk
Copy link
Contributor

Test notes for UIA consoles:

Windows 10 1803:
No noticeable changes.

Windows 10 1903:

  • Performance of selection change reporting has been significantly improved.
  • When editing text, NVDA repeatedly announces "selected" and "unselected". If it's the console's UIA implementation that's misbehaving here (rather than a bug in this PR) I think I can work around it.

@LeonarddeR
Copy link
Collaborator Author

@josephsl: I couldn't reproduce your issue with Excel with Excel 2016. I tested from source, commenting out the version check from _UIAHandler. Made sure that WithAutoSelectDetection is on the dynamic class of the edit box. Both the formulla bar and the in cell edit field work as expected.

@LeonarddeR
Copy link
Collaborator Author

@codeofdusk: I've only looked quickly at this, but it looks like the autoselect detection code triggers on this because even though nothing is selected and the text range seems to be collapsed, isCollapsed on the selection returns False.

@LeonarddeR
Copy link
Collaborator Author

@codeofdusk: No matter what I do, collapsing a text info for UIA consoles on Windows 10 1903 keeps returning False for isCollapsed. Really looks like there's something severely broken in there.

@LeonarddeR
Copy link
Collaborator Author

@leonardder commented on 3 Jun 2019, 07:20 CEST:

@josephsl: I couldn't reproduce your issue with Excel with Excel 2016. I tested from source, commenting out the version check from _UIAHandler. Made sure that WithAutoSelectDetection is on the dynamic class of the edit box. Both the formulla bar and the in cell edit field work as expected.

Could reproduce it in Excel 365. The reason is that the edit window is of window class Excel7, and that class is in badUIAWindowClassNames, so events for that class are forcefully ignored.

@josephsl
Copy link
Collaborator

josephsl commented Jun 3, 2019

Hi,

Excel: works. Thanks.

@LeonarddeR
Copy link
Collaborator Author

@michaelDCurran: I really would like to know what you think about this.
Note that issues regarding command consoles are still there, but that UIA implementation in 1903 realy has its brokenness.

@michaelDCurran
Copy link
Member

There are definitely issues with the way Microsoft has implemented UIA textRanges for consoles. Specifically:

  • In many cases, fetching the range for the caret results in a textRange one character previous to what it should be. Bill has worked around this bug I think.
  • When fetching the selection from a UIA text pattern when there is no selection (only a caret), the resulting textRange is not collapsed. I think it is expanded to one character. This is a bug which we really can't work around. I am in the process of getting this reported to Microsoft.
    I think all we can do for now is for consoles, we should still use editableTextWithoutAutoSelectDetection. I.e. check for autoSelectDetection class in findOverlayCasses and remove it, and make UIAWinConsole inherit from editableTextWithoutAutoSelectDetection.
    I do understand the advantages of using autoSelectDetection, but could you clearly define from a users' point of view how this pr is going to be bennoficial?

@LeonarddeR
Copy link
Collaborator Author

  I do understand the advantages of using autoSelectDetection, but could you clearly define from a users' point of view how this pr is going to be bennoficial?

I don't think that there are much benefits, apart from selection being reported when performed with the mouse There is also #9660 (comment) where @codeofdusk noted massive performance changes, but it looks like we have to disregard these.

@LeonarddeR
Copy link
Collaborator Author

@feerrenrut: Just wanted to point out that from a user's perspective, I think this one is important to have in 2019.3. It will also benefit from a long testing period.

@LeonarddeR
Copy link
Collaborator Author

@codeofdusk: how is this now behaving with regard to your console work?

Copy link
Collaborator

@josephsl josephsl left a comment

Choose a reason for hiding this comment

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

I think it'd be best to hold off until 2019.3 due to vast changes needed. I did run into editable field issues in Start menu and such, but no need to worry, as these problems are seen with Windows 10 App Essentials enabled (I'll write a fix for it from my add-on once this PR comes to NVDA Core proper). Thanks.

@codeofdusk
Copy link
Contributor

@codeofdusk: how is this now behaving with regard to your console work?

Initial testing looks quite promising (no longer says "selected" repeatedly when typing on Windows 10 1903)!

@LeonarddeR
Copy link
Collaborator Author

@feerrenrut; This pr fixes issues related to braille and UIA. It also needs some wider testing before going into an official release. Therefore if you could give this some attention in the near future that would be awesome!

@AppVeyorBot
Copy link

PR introduces Flake8 errors 😲

See test results for Failed build of commit dd9ed2dd7c

@LeonarddeR
Copy link
Collaborator Author

@feerrenrut: The linter raises the following warning for _UIAHandler:

2     F405 'UIA_Text_TextChangedEventId' may be undefined, or defined from star imports: comtypes, comtypes.gen.UIAutomationClient, ctypes, ctypes.wintypes

I can explicitly import it, but I'm afraid this will make _UIAHandler pretty messy in the end. Therefore I have the feeling that we should exclude this warning. For new modules, warning F403 will still trigger.

@LeonarddeR
Copy link
Collaborator Author

There is also the following issue for the following code:

	UIAEventIdsToNVDAEventNames.update({
		UIA_Text_TextChangedEventId: "textChange",
		UIA_Text_TextSelectionChangedEventId: "caret",
		})

If the line has 2 tabs at the start, the warning is: 1 ET126 (flake8-tabs) unexpected number of tabs at start of expression line (expected 1, got 2). However, if I remove one tab, we get the under indented for hanging indent warning. I have the feeling that flake8-tabs fails here.

@AppVeyorBot
Copy link

PR introduces Flake8 errors 😲

See test results for Failed build of commit 855139b269

@feerrenrut
Copy link
Contributor

Error F405 I believe is quite valuable.

  • F403: If we wish to import * so that clients of this module / package are exposed to everything in some other module then we can leave that and put a #noqa comment to explain why. Ideally this is rare.
  • F405: The functions from an import * that we use in the current module should be imported by name. This can be in addition to those the import * line.

I'm looking into the indentation error.

@feerrenrut
Copy link
Contributor

I'm not sure what is causing E121, note that this should be emitted, E121 is supposed to be replaced by ET121 from Flake8-tabs. I can't reproduce it with a minimal test file. So that the closing brace does not need to be moved when adding a new item, to make it easier to see that the brackets match, and indentation levels are correct, I would prefer the following style:

	UIAEventIdsToNVDAEventNames.update({
		UIA_Text_TextChangedEventId: "textChange",
		UIA_Text_TextSelectionChangedEventId: "caret",
	})

@feerrenrut
Copy link
Contributor

Ah, I think I made a mistake with the config. I don't think we want use-pycodestyle-indent = True

@feerrenrut
Copy link
Contributor

Or maybe we do, but need to ignore E121 and any others similar....

@LeonarddeR
Copy link
Collaborator Author

LeonarddeR commented Aug 2, 2019 via email

@feerrenrut
Copy link
Contributor

flake8-tabs replaces many, but not all of pycodestyle indent checks. When you enable flake8-tabs it disables the pycodestyle indent checks, this allows you to re-enable them. However, then some of them are contradictory to the flake8-tabs checks and should be manually disabled. I think we want some of them, but not all. I am looking at them now, and will update #10020 when I work out a good combination.

@josephsl
Copy link
Collaborator

josephsl commented Sep 8, 2019

To @LeonarddeR: any plans for dealing with linting?

Thanks.

@codeofdusk
Copy link
Contributor

To @LeonarddeR: any plans for dealing with linting?

Thanks.

You might also wish to test how the latest changes affect UIA consoles and if selection is reported more consistently.

@LeonarddeR
Copy link
Collaborator Author

I somehow thought that this was already merged. Should have been a dream.

@LeonarddeR
Copy link
Collaborator Author

I fixed linting issues by applying a suggestion from @michaelDCurran from #10110 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants