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 the browser.proton.enabled prefs enabled.
• Enable VoiceOver.

Steps to reproduce

1. Launch Firefox.
2. Right click on any page, text link, image link, tab, bookmark, etc
3. 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

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

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

AGreed, marking as P2a and will discuss as potential P1 candidate at triage today

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.

(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? :)

Hi Morgan, thanks for the offer! I have filed bug 1703617 for the accessibility review.

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.

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.

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

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.

(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 to nsXULPopupManager::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.

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.

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

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

(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

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

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: [] }
]

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

This is a lower risk alternative to the approach in bug 1703899 and can be
removed once bug 1703899 is fixed.

https://hg.mozilla.org/mozilla-central/rev/e700003850eb

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

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

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