Emoji panel and clipboard history: locate actual items when traversing the elements for element selected event by josephsl · Pull Request #11485 · nvaccess/nvda

josephsl · 2020-08-11T23:29:25Z

Hi,

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

Link to issue number:

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:

Opening emoji panel multiple times.
Opening clipboard history.
Toggling between emoji panel and clipboard history without closing them.
Searching for emoji items.
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.

… for consistency with other files)

… 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 · 2020-09-10T16:48:43Z

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

Thanks.

LeonarddeR · 2020-10-07T17:16:39Z

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

josephsl · 2020-10-07T17:28:26Z

Hi, the new design? Can’t verify that, as I didn’t get the new design yet (it is rolling out to a subset of dev Insiders). Thanks.

…0377

… 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 · 2020-10-12T04:28:25Z

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

Thanks.

feerrenrut · 2020-10-13T16:51:25Z

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

LeonarddeR

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

LeonarddeR · 2020-10-13T17:03:00Z

+			and config.conf["inputComposition"]["autoReportAllCandidates"]
+		):
 			try:
-				self.event_UIA_elementSelected(obj.firstChild.firstChild.firstChild, nextHandler)


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

josephsl · 2020-10-13T17:13:27Z

Hi, try/except was done for compactness and for readability. I admit that the code under review came from my add-on, so I’ll do something about these from both sides. Thanks.

…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.

…ncy with UIA IME candidates window.

josephsl · 2020-10-16T22:06:41Z

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 · 2020-11-20T18:34:33Z

@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 · 2020-11-20T20:48:18Z

Hi, nope. There is an issue with dev channel build that has to do with categories firing element selected event but that can be handed by Windows 10 App Essentials add-on and is beyond the PR scope. Thanks.

josephsl · 2020-12-09T19:58:46Z

Hi all,

Ready for another review if needed. Thanks.

josephsl · 2020-12-12T17:40:28Z

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 · 2021-05-07T02:32:27Z

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.

…ore build range for consistency with the rest of UIA window open event.

seanbudd · 2021-05-14T06:13:13Z

closing/reopening to trigger a build

seanbudd · 2021-05-14T06:17:07Z

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 · 2021-05-14T06:44:38Z

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:

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.
Press Windows+Period to open emoji panel.
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:

Emoji panel and clipboard history have been combined under a web document interface.
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 · 2021-05-17T06:00:39Z

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 · 2021-05-17T06:06:12Z

Hi, I expect Cobalt won’t be finalized until mid-June, so perhaps it would be best to defer support for Cobalt emoji panel until fall (2021.2) at the earliest, with system tests planned for summer (I think I might not be able to write tests, but would be able to help plan things a bit before starting graduate studies). Thanks.

josephsl · 2021-06-01T05:32:02Z

Blocked by #12487. Thanks.

seanbudd

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 · 2021-07-06T00:54:47Z

Hi, agreed about system tests – let’s get the testing party started with alpha builds!!! Thanks (this will lessen the burden on Windows App Essentials add-on significantly).

josephsl added 7 commits August 11, 2020 08:17

Modern keyboard: update copyright header (year and removed intro text…

781a9e8

… for consistency with other files)

Modern keyboard: Flake8 lint

4de9b8e

feerrenrut added the blocked label Sep 25, 2020

josephsl added 3 commits October 11, 2020 21:12

Merged master

b53734b

Modern keyboard: remove duplicate event handler import. Re nvaccess#1…

faecbb3

…0377

feerrenrut removed the blocked label Oct 13, 2020

feerrenrut requested a review from LeonarddeR October 13, 2020 16:50

LeonarddeR suggested changes Oct 13, 2020

View reviewed changes

josephsl added 3 commits October 14, 2020 12:51

Modern keyboard: eventHandler.executeEvent -> queueEvent for consiste…

9435bce

…ncy with UIA IME candidates window.

feerrenrut requested a review from LeonarddeR November 20, 2020 07:47

Merge branch 'master' into i10377

99b69a0

josephsl added 2 commits May 6, 2021 19:33

Merge branch 'master' into i10377

d0386d4

Modern keyboard/emoji panel in 1709 and 1803: check Automation Id bef…

5a42ae3

…ore build range for consistency with the rest of UIA window open event.

josephsl requested a review from a team as a code owner May 7, 2021 02:47

josephsl requested a review from seanbudd May 7, 2021 02:47

seanbudd self-assigned this May 14, 2021

seanbudd closed this May 14, 2021

seanbudd reopened this May 14, 2021

seanbudd added this to the 2021.2 milestone May 17, 2021

seanbudd added 3 commits May 31, 2021 17:16

emoji panel and clipboard system tests

784f085

fix teardown

07b943e

exclude panel search from appveyor build

d93967b

seanbudd mentioned this pull request May 31, 2021

Emoji panel and clipboard history system tests #12487

Closed

8 tasks

seanbudd added 2 commits June 1, 2021 12:47

fix behaviour to be more expected

c5c353b

exclude emoji test from build

7dd9fa6

seanbudd added 3 commits June 3, 2021 10:45

more explicit tags for exclusion

faf01ff

use run dialog instead of search bar

af7b38e

Merge branch 'master' into i10377

cc9fee4

seanbudd approved these changes Jul 6, 2021

View reviewed changes

update changes

24daf89

Merge branch 'master' into i10377

d9e30c3

seanbudd merged commit cc0d506 into nvaccess:master Jul 6, 2021

josephsl deleted the i10377 branch July 6, 2021 01:20

Uh oh!

Conversation

josephsl commented Aug 11, 2020 • edited by seanbudd Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing performed:

Known issues with pull request:

Change log entry:

Uh oh!

josephsl commented Sep 10, 2020

Uh oh!

LeonarddeR commented Oct 7, 2020

Uh oh!

josephsl commented Oct 7, 2020 via email • edited by feerrenrut Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josephsl commented Oct 12, 2020

Uh oh!

feerrenrut commented Oct 13, 2020

Uh oh!

LeonarddeR left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LeonarddeR Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

josephsl commented Oct 13, 2020 via email • edited by feerrenrut Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josephsl commented Oct 16, 2020

Uh oh!

LeonarddeR commented Nov 20, 2020

Uh oh!

josephsl commented Nov 20, 2020 via email

Uh oh!

josephsl commented Dec 9, 2020

Uh oh!

josephsl commented Dec 12, 2020

Uh oh!

josephsl commented May 7, 2021

Uh oh!

seanbudd commented May 14, 2021

Uh oh!

seanbudd commented May 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josephsl commented May 14, 2021

Uh oh!

seanbudd commented May 17, 2021

Uh oh!

josephsl commented May 17, 2021 via email

Uh oh!

josephsl commented Jun 1, 2021

Uh oh!

seanbudd left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josephsl commented Jul 6, 2021 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

josephsl commented Aug 11, 2020 •

edited by seanbudd

Loading

josephsl commented Oct 7, 2020 via email •

edited by feerrenrut

Loading

josephsl commented Oct 13, 2020 via email •

edited by feerrenrut

Loading

seanbudd commented May 14, 2021 •

edited

Loading

seanbudd left a comment •

edited

Loading