1573337 - History and Bookmarks menus inaccessible

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect, P1)

Description

[James Teh [:Jamie]]

STR (with the NVDA screen reader):

1. Press alt+s to open the History menu.
   • Expected: NVDA should announce "Show All History" (the first item in the menu).
   • Actual: NVDA announces "History menu" and nothing else.
2. Press down arrow to move to the next item.
   • Expected: NVDA should announce "Clear Recent History" (the second item in the menu).
   • Actual: NVDA says nothing.
3. Press escape to close the menu.
4. Press alt+b to open the Bookmarks menu.
   • Expected: NVDA should announce "Show All Bookmarks" (the first item in the menu).
   • Actual: NVDA announces "Bookmarks menu" and nothing else.
5. Press down arrow to move to the next item.
   • Expected: NVDA should announce "Bookmark This Page" (the second item in the menu).
   • Actual: NVDA says nothing.

The a11y tree beneath these menus is completely empty.

7:13.30 INFO: Last good revision: 8bd696e50a12db4f6be4ded519ef4f97ed6fed60
7:13.30 INFO: First bad revision: bd7cd9d7b6ade7bf9fa8441a8686f19c36f2a7e5
7:13.30 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=8bd696e50a12db4f6be4ded519ef4f97ed6fed60&tochange=bd7cd9d7b6ade7bf9fa8441a8686f19c36f2a7e5

This suggests bug 1539651 is the culprit.

Alex, any ideas?

Comment 1

[James Teh [:Jamie]]

[Tracking Requested - why for this release]: The Bookmarks and History menus are now inaccessible to screen reader users (and likely users of other assistive technology).

Comment 2

[James Teh [:Jamie]]

Eitan noted that he had some concerns regarding shadow DOM and bug 686400. Just to be sure, I reverted that patch (and all subsequent related patches) locally and the problem still persists.

Comment 3

[James Teh [:Jamie]]

Commenting out the eNoXBLKids flag for XULMenupopupAccessible seems to fix this:
https://searchfox.org/mozilla-central/rev/30b01f4f60dbcbd6b01500a26b3100c28005cf62/accessible/xul/XULMenuAccessible.cpp#355
But isn't the point of this patch to get rid of XBL; i.e. we now have less XBL children rather than more? is our eNoXBLKids flag doing the wrong thing for some reason?

Comment 5

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #3)

Commenting out the eNoXBLKids flag for XULMenupopupAccessible seems to fix this:
https://searchfox.org/mozilla-central/rev/30b01f4f60dbcbd6b01500a26b3100c28005cf62/accessible/xul/XULMenuAccessible.cpp#355
But isn't the point of this patch to get rid of XBL; i.e. we now have less XBL children rather than more? is our eNoXBLKids flag doing the wrong thing for some reason?

I suspect that FlattenedChildIterator, that a11y::TreeWaleker is based at, ignores menuitems under a menupopup because they are slotted under an arrowscrollbox in menupoup's shadow DOM. Then, TreeWalker doesn't go into arrowscrollbox because arrowscrollbox is not accessible itself, and menupoup accessible has eNoXBLKids, which prevents TreeWalker from looking into XBL anonymous content of arrowscrollbox. That's why removing this flag helps to fix the bug.

So I'd say this flag shall be removed. However it may have side affect, for example, it will appear scroll-up/down buttons in the accessible tree. It's probably ok though. If not, then aria-hidden should help.

Comment 6

[James Teh [:Jamie]]

Thanks Alex. Would bug 1514926 (converting arrowscrollbox to CustomElement) also fix this? (I'm not necessarily suggesting this should be the solution, just trying to make sure I understand correctly.)

Comment 8

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #6)

Thanks Alex. Would bug 1514926 (converting arrowscrollbox to CustomElement) also fix this? (I'm not necessarily suggesting this should be the solution, just trying to make sure I understand correctly.)

yes, but same way as the flag removal, i.e. scrollup-down buttons will be visible in the accessible tree. Also, it will make the flag obsolete. So it seems the flag removal is something that we should do anyways, I'm not certain about scrollup-down buttons though.

Comment 9

[alexander :surkov (:asurkov)]

It looks like we can remove xblkids flag support from a11y tree trawersal:

• a generic menupopup is an XBL, but it contains arrowsrollbox, which makes it same as bookmarks menu (will be converted to CE in bug 1555497)
• comboboxes (except textbox autocompelete) and menuitems are are custom elements now

Comment 10

[alexander :surkov (:asurkov)]

Attachment: Bug 1573337 - remove XBL anon content filtering support from accessible tree traversal

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9406ccf380bb
remove XBL anon content filtering support from accessible tree traversal r=Jamie

Comment 12

[alexander :surkov (:asurkov)]

(In reply to Pulsebot from comment #11)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9406ccf380bb
remove XBL anon content filtering support from accessible tree traversal
r=Jamie

thank you for landing!

Comment 13

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/9406ccf380bb

Comment 14

[Jean-Philippe MENGUAL]

I confirm it is working again now, since ysterday. Many thanks for the fix.

Comment 15

[Camelia Badau [:cbadau], Desktop QA]

Verified fixed on Window 7 x64, Windows 8.1 x86 and Windows 10 x64 with NVDA screen reader using Firefox 70 Beta 9 (buildID: 20190923154733).