Closed Bug 1573337 Opened 2 months ago Closed 2 months ago

History and Bookmarks menus inaccessible

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox68 --- unaffected
firefox69 --- unaffected
firefox70 + verified

People

(Reporter: Jamie, Assigned: surkov)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

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?

Flags: needinfo?(surkov.alexander)

[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).

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.

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?

Duplicate of this bug: 1573107

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

Flags: needinfo?(surkov.alexander)

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

Flags: needinfo?(surkov.alexander)
Duplicate of this bug: 1573877

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

Flags: needinfo?(surkov.alexander)

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
Pushed by mzehe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9406ccf380bb
remove XBL anon content filtering support from accessible tree traversal r=Jamie

(In reply to Pulsebot from comment #11)

Pushed by mzehe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9406ccf380bb
remove XBL anon content filtering support from accessible tree traversal
r=Jamie

thank you for landing!

Assignee: nobody → surkov.alexander
Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

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

Flags: qe-verify+

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

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.