With native context menus enabled VoiceOver doesn’t read the context menu options
Categories
(Core :: Widget: Cocoa, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox89 | --- | verified |
People
(Reporter: emilghitta, Assigned: mstange)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Keywords: access, Whiteboard: [proton-context-menus] [mac:mr1])
Attachments
(1 file)
Affected versions
- Firefox 89.0a1 (BuildId:20210406152948)
Affected platforms
- macOS 11
- macOS 10.15.7
Unaffected platforms
- Windows 10 64bit.
- Ubuntu 20.04
Preconditions
- Have the
widget.macos.native-context-menus
and thebrowser.proton.enabled prefs
enabled. - Enable VoiceOver.
Steps to reproduce
- Launch Firefox.
- Right click on any page, text link, image link, tab, bookmark, etc
- Navigate through the available context menu options.
Expected result
- VoiceOver reads all the available context menu options.
Actual result
- VoiceOver doesn’t read the available context menu options.
Regression Range
- I don’t think that this is a regression.
Notes
- It seems that the address bar context menu is not affected.
- The following context menus are affected: Page context menu, text link context menu, image context menu, vide context menu, tabs context menu, bookmark toolbar context menu, bookmark context menu, input fields context menu, toolbar icon context menu
Comment 1•4 years ago
|
||
(In reply to Emil Ghitta, QA [:emilghitta] from comment #0)
Regression Range
- I don’t think that this is a regression.
To be clear, I think you mean that this isn't a regression while native context menus are turned on. It's a regression compared to the previous menu implementation, right? Because with the "old" menu VO seems to work fine.
With the new items, VO seems to detect a menu appears but claims it has 0 items.
Romain, I think this should be pretty high priority as we're breaking a11y support for something we've recently noted as now supporting ( https://blog.mozilla.org/accessibility/voiceover-support-for-macos-in-firefox-87/ ).
Reporter | ||
Comment 2•4 years ago
|
||
(In reply to :Gijs (he/him) from comment #1)
To be clear, I think you mean that this isn't a regression while native context menus are turned on. It's a regression compared to the previous menu implementation, right? Because with the "old" menu VO seems to work fine.
Yes, that is correct.
Updated•4 years ago
|
Comment 3•4 years ago
|
||
AGreed, marking as P2a and will discuss as potential P1 candidate at triage today
Assignee | ||
Comment 4•4 years ago
|
||
Agreed, this should block enabling native context menus by default. I'm also going to file a bug for a more comprehensive accessibility review of native context menus.
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Comment 5•4 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #4)
Agreed, this should block enabling native context menus by default. I'm also going to file a bug for a more comprehensive accessibility review of native context menus.
Hi! I'd be happy to take on that review, can you forward me the bug once its filed? :)
Assignee | ||
Comment 6•4 years ago
|
||
Hi Morgan, thanks for the offer! I have filed bug 1703617 for the accessibility review.
Assignee | ||
Comment 7•4 years ago
|
||
I found RootAccessible::ProcessDOMEvent
which has a fair amount of code to handle various popup-related events. These events are fired both from non-native menus and from native menus. But if we handle them for native menus, our accessibility notifications may conflict with the built-in accessibility support for native menus.
I am now checking how this works for menubar menus. Those have been using native menus all along. Maybe we bypass our accessibility code for them, or maybe they work by accident.
Assignee | ||
Comment 8•4 years ago
|
||
When running with A11YLOG=DOMEvents
, opening menus in the native menubar logs popupshown events with the targets marked as [not accessible]
, but when opening native context menus, the targets are not marked as [not accessible]
. I think the menubar has display:none so we don't have frames for it, so that's probably why our accessibility code ignores it.
I have confirmed that, if I remove the handling of the popupshown
event in RootAccessible, this bug is fixed and menus are indicated with the correct number of items to VoiceOver.
I will now try to find a way to annotate DOMEvents from native menus as "initiated from native menu" so that the accessibility code can ignore them. Alternatively, I could try to exclude <menupopup> subtrees for native context menus from the accessibility tree.
Comment 9•4 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #8)
Alternatively, I could try to exclude <menupopup> subtrees for native context menus from the accessibility tree.
I think that would make some sense, since we want Mac's native a11y exposure to be the source of truth for those. If there's some way to easily detect those, you could exclude them in nsAccessibiliyService::CreateAccessible. Alternatively, you could set aria-hidden="true" on the menupopups, but that might be tricky to do since we don't want that on other platforms.
Assignee | ||
Comment 10•4 years ago
|
||
Thanks for the feedback! The problem with excluding them upfront is that the same menu can be shown as a native menu or as a non-native menu, depending on which value for aIsContextMenu
is passed in the call to nsXULPopupManager::ShowPopupAtScreen
. And the native menu pref can change at runtime.
I'll give aria-hidden="true"
a try. Maybe I can set it dynamically just before the menu is opened.
Comment 11•4 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #10)
Thanks for the feedback! The problem with excluding them upfront is that the same menu can be shown as a native menu or as a non-native menu, depending on which value for
aIsContextMenu
is passed in the call tonsXULPopupManager::ShowPopupAtScreen
.
Is there some way to query whether the menu is native or non-native after the popupshown event is fired? If so, we could change menupopups to only exist in the a11y tree when they're actually visible, as we've already done for tooltips (bug 1652211) and panels (bug 1699053). Then, the menupopup would be excluded in CreateAccessible if it's currently native.
Assignee | ||
Comment 12•4 years ago
|
||
We can definitely add a piece of state on the nsMenuPopupFrame that indicates whether it's currently open as a native menu.
Let me make sure I understood your suggestion correctly:
- While the menupopup is closed, don't create an accessible for it.
- When the popupshown event fires, if the popup is open as a non-native menu, insert it into the a11y tree and then handle the event the same way we do today.
- When the popuphiding event fires, if the popup is currently present in the a11y tree (which indicates it was opened as a non-native menu), handle the event the same way as today and then remove the menupopup from the a11y tree.
I'm a bit scared to implement this because it'll change the behavior for non-native menupopups and also affect other platforms, but I can give it a try.
Comment 13•4 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #12)
We can definitely add a piece of state on the nsMenuPopupFrame that indicates whether it's currently open as a native menu.
Great!
Let me make sure I understood your suggestion correctly:
You did.
I'm a bit scared to implement this because it'll change the behavior for non-native menupopups and also affect other platforms, but I can give it a try.
We've already done this for tooltips and panels, so I think it's reasonable to do it for menupopups as well. I think it'd be reasonable for the a11y team to write that patch, but we'll need the nsMenuPopupFrame state for the native context menu fix.
Morgan, do you have the cycles to file a bug and write the patch to only include menupopups in the tree when they're visible? It'll be very similar to your panel patch. It'll then (hopefully) be a tiny patch to use the (soon to be) nsMenuPopupFrame state to exclude native context menus.
Assignee | ||
Comment 14•4 years ago
|
||
(In reply to James Teh [:Jamie] from comment #13)
We've already done this for tooltips and panels, so I think it's reasonable to do it for menupopups as well. I think it'd be reasonable for the a11y team to write that patch, but we'll need the nsMenuPopupFrame state for the native context menu fix.
Sounds great! I've put up a patch for the nsMenuPopupFrame state in bug 1703702.
Comment 15•4 years ago
|
||
(In reply to James Teh [:Jamie] from comment #13)
Morgan, do you have the cycles to file a bug and write the patch to only include menupopups in the tree when they're visible? It'll be very similar to your panel patch. It'll then (hopefully) be a tiny patch to use the (soon to be) nsMenuPopupFrame state to exclude native context menus.
Yep, I can get that done 😀 I'll add it here once I file it
Comment 16•4 years ago
|
||
It sounds like hiding the menupopups from the tree when hidden is "more correct" in that it makes them function how we typically expect shown/hidden accessibles to function, but just to add another option: since this is a mac specific issue, we also have handling on the mac side for showing/ignoring accessibles based on their visibiilty and/or their parent chain structure. This happens in our ignoreWithParent
functions. I added a bit of handling for non-native context menus there already, you can see that code here: https://searchfox.org/mozilla-central/rev/21110f35dbb95d3c41c8c5bd363ec689900af30f/accessible/mac/mozSelectableElements.mm#166
There's handling in that file for both menus and menu items
Comment 17•4 years ago
|
||
Also: I assume we're interested in hiding the entire menupopup subtree, right? Like, we're aiming for:
role: ROLE_COMBOBOX,
children: []
instead of:
role: ROLE_COMBOBOX,
children: [
{role: ROLE_COMBOBOX_OPTION,
children: [] },
{role: ROLE_COMBOBOX_OPTION,
children: [] }
]
Assignee | ||
Comment 18•4 years ago
|
||
Yes, we want to hide the entire subtree. However, I'm suprised to see COMBOBOX in the tree you pasted, I think it should be MENUPOPUP instead: https://searchfox.org/mozilla-central/rev/3a798ef9252896fb389679f06dd3203169565af0/accessible/xul/XULMenuAccessible.cpp#409
Updated•4 years ago
|
Assignee | ||
Comment 19•4 years ago
|
||
This is a lower risk alternative to the approach in bug 1703899 and can be
removed once bug 1703899 is fixed.
Comment 20•4 years ago
|
||
Comment 21•4 years ago
|
||
bugherder |
Reporter | ||
Comment 22•4 years ago
|
||
VoiceOver now reads the majority of the native context menus options but while trying to cover all the context menus which are in QA's scope, I noticed that the native context menu options for the password input fields are not being read by VoiceOver.
More details can be found in Bug 1705157
Updated•3 years ago
|
Reporter | ||
Comment 23•3 years ago
|
||
In an effort to extend the macOS versions coverage for the VoiceOver cases I've noticed that macOS 10.13.6 is still affected by this issue (see Bug 1706966).
Reporter | ||
Comment 24•3 years ago
|
||
Both Bug 1705157 & Bug 1706966 seem to be reproducible with other browsers as well but VoiceOver now reads all the other context menu options as expected so that's a major improvement!
Verified this on Firefox 89.0b8 (BuildId:20210504185920) on macOS 10.14, 10.15 & 11
Updated•1 year ago
|
Description
•