Closed Bug 1703482 Opened 8 months ago Closed 8 months ago

With native context menus enabled VoiceOver doesn’t read the context menu options

Categories

(Core :: Widget: Cocoa, defect, P1)

Firefox 89
All
macOS
defect

Tracking

()

VERIFIED FIXED
89 Branch
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] [access-s2][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 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/ ).

Flags: needinfo?(rtestard)

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

Keywords: access
Whiteboard: [proton-context-menus] → [proton-context-menus] [access-s2]

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

Flags: needinfo?(rtestard)
Priority: -- → P2
Whiteboard: [proton-context-menus] [access-s2] → [proton-context-menus] [access-s2] [priority:2a]

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.

Assignee: nobody → mstange.moz
Blocks: 1700679
Status: NEW → ASSIGNED
Priority: P2 → P1
Whiteboard: [proton-context-menus] [access-s2] [priority:2a] → [proton-context-menus] [access-s2]

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

Flags: needinfo?(mreschenberg)
Depends on: 1703702

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

Blocks: 1703617

(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

Flags: needinfo?(mreschenberg)

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

Depends on: 1703899

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: [] }
]
Flags: needinfo?(mstange.moz)

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

Flags: needinfo?(mstange.moz)
Whiteboard: [proton-context-menus] [access-s2] → [proton-context-menus] [access-s2][mac:mr1]

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

Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/e700003850eb
Workaround: Set aria-hidden="true" while a popup is open as a native context menu. r=morgan
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → 89 Branch

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

See Also: → 1705157
Flags: qe-verify+

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

See Also: → 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

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