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

Emoji panel and clipboard history: locate actual items when traversing the elements for element selected event #11485

Merged
merged 29 commits into from Jul 6, 2021

Conversation

josephsl
Copy link
Collaborator

@josephsl josephsl commented Aug 11, 2020

Hi,

A reboot of #10378 with updated assumptions and lessons learned:

Link to issue number:

Fixes #10377

Summary of the issue:

In Version 1903 and later, on some systems, when emoji panel and clipboard history opens, wrong items are announced.

Description of how this pull request fixes the issue:

Refined element traversal strategies when emoji panel and clipboard history opens. Also performed lints and changed cachedAutomationID to UIAAutomationId, along with using dedicated input panel and input panel Automation Id variables to react to window open event.

Testing performed:

Tested via Windows 10 App Essentials for several months:

  1. Opening emoji panel multiple times.
  2. Opening clipboard history.
  3. Toggling between emoji panel and clipboard history without closing them.
  4. Searching for emoji items.
  5. Reopening emoji panel after selecting People emoji group in Version 1903 and later.

Known issues with pull request:

There is no way of telling NVDA that modern input elements are shown on screen (that's reserved for a future pull request; see #11454 for details).

Change log entry:

Same as #10378.

Thanks.

… and input panel Automation Id to react to various parts of modern input facility.

A dedicated input panel element will be used as a starting point for traversing modern keyboard interface. Along with this, rename childAutomationID to input panel Automation Id to better communicate the fact that window open event will be using Automation Id to detect different modern input features and react accordingly.
…clipboard history to account for changes in Version 1903 on some systems. Re nvaccess#10377.

Refined element traversal in window open event:
* Emoji panel: in Version 1903 (and on some systems), something other than emoji items are selected, particularly when People emoji group opens and skin tone modifier is somehow selected.
* Clipboard history: if clipboard is empty, clipboard status text isn't announced due to moving to a different element.
…elves rather than parent lists. Re nvaccess#10377.

Rather than calling element selected event on emojis list or clipboard history list, descend one more level (or in case of People emoji group, actual emojis list) so the event can deal with items themselves. This also means element selected event won't have to worry about traversing the elements tree one more time.
…executeEvent.

Rather than executing UIA element selected event directly, use event handler's execute event function to schedule this event. This allows event handling features from NVDA to respond to this event.
…separate list, along with using isWin10 function.

Rather than using a build number, use winVersion.isWin10 function to check Windows 10 releases. Also, separate 1709 and 1803 emoji panel Automation Id's to a separate list.
@josephsl
Copy link
Collaborator Author

Blocked by #11562 due to edits to window open event handler.

Thanks.

@LeonarddeR
Copy link
Collaborator

Does this also fix the emoji panel on 21H1 builds?

@josephsl
Copy link
Collaborator Author

josephsl commented Oct 7, 2020 via email

… UIA IME candidate panel check. Re nvaccess#10377.

IME candidate panel check already assigns a dedicated variable to obj.firstChild, making input panel variable assignment redundant. Also, rename input panel Automation Id back to childAutomationId.
@josephsl
Copy link
Collaborator Author

Hi,
Ready for review again now that #11562 is here.

Thanks.

@feerrenrut
Copy link
Contributor

I'm asking @LeonarddeR for a review because he was also reviewing #10378

Copy link
Collaborator

@LeonarddeR LeonarddeR left a comment

Choose a reason for hiding this comment

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

Could you please especially consider cases where you're traversing objects without checking whether the objects exist?

try:
self.event_UIA_elementSelected(obj.firstChild.firstChild.firstChild, nextHandler)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I rather see checking every child's boolean value rather than doing a try/except here. That's just my personal preference.

@josephsl
Copy link
Collaborator Author

josephsl commented Oct 13, 2020 via email

…omment clarification. Re nvaccess#10377.

Reviewed by Leonard de Ruijter: split lines with multiple statements, clarify that 'some systems' refer to non-English builds of Version 1903 (May 2019 Update) and later.
…esent. Re nvaccess#10377.

Suggested by Leonard de Ruijter: when assigning emoji item, check each emoji panel level (emoji list header, grouping, emoji items/skin tone modifier list) to make sure child objects are present (not None). In case skin tone modifiers list is selected, before moving to the next object (emoji item), make sure it is even present. This avoids a try/except statement and is used to force NVDA to announce something rather than staying silent of attribute error is raised in the future due to modern keyboard UI changes.
@josephsl
Copy link
Collaborator Author

Hi,

It was pointed out a few days ago that the emoji panel was redesigned in a dev channel WIP build. I'm sorry to inform everyone that the redesigned emoji panel cannot be used with NVDA at all - no announcements with both Narrator and NVDA when the panel opens, so for the purposes of this PR, the redesigned emoji panel is out of scope.

Thanks.

@LeonarddeR
Copy link
Collaborator

@josephsl I'm on the most recent insider preview build, and the panel seems to work correct again. Does this pr needs to be revised for this? At least, I think I'd prefer it if you check the current changes against an insider build before continuing with this.

@josephsl
Copy link
Collaborator Author

josephsl commented Nov 20, 2020 via email

@josephsl
Copy link
Collaborator Author

josephsl commented Dec 9, 2020

Hi all,

Ready for another review if needed. Thanks.

@josephsl
Copy link
Collaborator Author

Hi,

Update: build 21277 (rs_prerelease, released December 10) brought back modern emoji panel. As for my assessment on it, see the above comment (brought screen readers to their knees again). Therefore for the purposes of this PR, the new emoji panel will not be supported in this PR.

Thanks.

@josephsl
Copy link
Collaborator Author

josephsl commented May 7, 2021

Hi,

One more change: to prepare for Python 3.10's pattern matching syntax and readability, when checking for 1709 and 1803 emoji panel, Automation Id will be checked before build range; no changes to semantics whatsoever.

Thanks.

@josephsl josephsl requested a review from a team as a code owner May 7, 2021 02:47
@josephsl josephsl requested a review from seanbudd May 7, 2021 02:47
@seanbudd seanbudd self-assigned this May 14, 2021
@seanbudd
Copy link
Member

closing/reopening to trigger a build

@seanbudd seanbudd closed this May 14, 2021
@seanbudd seanbudd reopened this May 14, 2021
@seanbudd
Copy link
Member

seanbudd commented May 14, 2021

Thanks @josephsl,

Would you be able to add system tests to smoke test the emoji panel and clipboard history? I understand your capacity is changing right now (congratulations!) so let us know if the work needs to be passed over. Some simple tests as mentioned in the manual testing would be good to start with and we can add more later as needed.

@josephsl
Copy link
Collaborator Author

Hi,

I might not be able to add system tests to smoke this. The biggest concern is the fact that emoji panel UI is changing in Cobalt releases (build 21300 and later) which will cause tests to fail unless winVersion.getWinVer().build is checked first.

The test procedure should be:

  1. Check if emoji panel is present (winVersion.GetWinVer() >= winVersion.WIN10_1809; Version 1709 is out of support and emoji panel UI has changed in Version 1809, that's why). This will need to be refined if adding smoke tests for Cobalt.
  2. Press Windows+Period to open emoji panel.
  3. Use the object navigation path as specified in this PR and see if NVDA announces emoji of any kind (you can't really rely on the exact emoji being announced because the emoji panel may learn to give you something else; I expect it won't be a big concern as Appveyor spawns a fresh virtual machine every time a commit is made). A possible check is making sure the resulting navigator object is indeed a UIA object with a specific Automation Id and/or class name and/or is part of a specific app module (you can check for api.getNavigatorObject().appName == "textinputhost" provided that the VM runs Version 2004 (build 19041) or later; if it runs Server 2019 (Version 1809) or 190x (1903/1909), then it becomes a bit complicated as the executable name is rather long).

NOtes on Cobalt emoji panel:

  1. Emoji panel and clipboard history have been combined under a web document interface.
  2. UIA notification event, not element selected event, is used to announce emojis. Therefore object navigation strategy won't work.

Making matters even more complicated is a possibility that Version 21H2 client and server code might be different - currently latest Server 2022 preview (Version 21H2) code is based on work done last year (iron/fe_release/build 203xx), whereas client is based on work being done this year (cobalt/co_release/build 213xx). If it turns out Version 21H2 code between client and server are different, and if Server 2022 will be based on Iron (build 203xx), then it might be a bit easier to smoke test emoji panel (although this means dealing with an extra UIA element selected event being fired somehow); all this is thrown out the window if Server 2022 will be based on Cobalt and Appveyor decides to use Server 2022 VM, in which case smoke test cannot be added.

I know how complicated the picture is (this is one of the personal gripes with what's happening at Microsoft build labs, but such things are expected; this is one of those perks and stresses of being a Windows Insider and trying to guess what Microsoft will do in the short term).

Hope this helps. Thanks.

@seanbudd
Copy link
Member

Hi @josephsl,

Thanks for the detailed notes. I think for now we can just write a system that passes for Iron (assuming that it would currently work for AppVeyor and personal machines). If the system test starts to fail we can exclude it from builds so that AppVeyor continues to pass, and prioritise work to support Cobalt if it hasn't been done yet.

@josephsl
Copy link
Collaborator Author

josephsl commented May 17, 2021 via email

@seanbudd seanbudd added this to the 2021.2 milestone May 17, 2021
@josephsl
Copy link
Collaborator Author

josephsl commented Jun 1, 2021

Blocked by #12487. Thanks.

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.

This seems to be somewhat unnecessarily blocked by things that may take a while to resolve. It would be good to still merge the system tests in #12487, but I have tested this PR with the tests and it seems okay.

@josephsl
Copy link
Collaborator Author

josephsl commented Jul 6, 2021 via email

@seanbudd seanbudd merged commit cc0d506 into nvaccess:master Jul 6, 2021
@josephsl josephsl deleted the i10377 branch July 6, 2021 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants