Closed Bug 1699053 Opened 4 years ago Closed 4 years ago

[macOS VoiceOver] The Screen Reader has inconsistent behaviour with the webRTC permission panel

Categories

(Firefox :: Site Permissions, defect, P2)

Desktop
macOS
defect

Tracking

()

VERIFIED FIXED
89 Branch
Accessibility Severity s2
Tracking Status
firefox86 --- wontfix
firefox87 --- wontfix
firefox88 --- verified
firefox89 --- verified

People

(Reporter: tbabos, Assigned: morgan)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(5 files)

Attached image Nightly 88 - bug.png (deleted) —

Affected Versions:
All latest Firefox versions

Tested on:
MacOS

Steps to Reproduce:

  1. Enable the macOS screen reader before starting Firefox
  2. Go to https://mozilla.github.io/webrtc-landing/gum_test.html
  3. Use Option+Command+Right Arrow to navigate along with the screen reader to the Microphone&Camera button
  4. Follow the voiceOver guidance to pres the button (Option+Command+Space) or hit Enter

Expected Result:
As soon as the permission prompt is displayed, the voice-over should start reading out loud all the content from the prompt (such as with NVDA on Windows)

Actual Results:
2 possible behaviors, inconsistent :

  • Only the question is detected by the screen reader: "Allow mozilla.github.io to use your camera and microphone?"
  • Everything is detected correctly: question, camera label, microphone menulist, etc

Note:
After refreshing the page and following again the steps the screen reader will not detect the toggled permission prompt at all, see attached screenshot.

Regression-Range:
Reproducible on FX 86, also intermittently. There are 2 bugs here (from which 1 is intermittent) and the refresh page aspect mentioned in notes is also causing troubles narrowing the regression-range. Not related to Proton changes.

Severity:
Marking this as S2 given a11y is an important area to cover and at this point the webRTC prompt might not be functional for screen reader users. Please feel free to change the severity if need be after further investigations, I assume people who use screen readers on a daily basis will have a more technical and in-depth analysis of the severity for this issue.

Attached image FX86-bug.png (deleted) —
Attached image Nightly 88 Works.png (deleted) —
Attached image After Refresh.png (deleted) —

Morgan or Eitan, would you be able to look into this? This is a desktop priority, so it should be prioritised over other work. Whoever takes it, feel free to clear the other NI. :)

Note that the WebRTC permission panel changed in two ways recently:

  1. Bug 1693644: If there's only one camera or microphone, a label is used for the value instead of a menulist.
  2. Bug 1697295: Icons for camera and microphone instead of labels. This necessitated using aria-describedby to explicitly specify the text of the alert, since otherwise, the graphic is ignored by NVDA. However, even if VO ignores the description on the alert (which it may well do if it's using live eregion semantics), it should still be able to see the acc name of the graphic, etc.

This raises an interesting question: how does VO handle buttons, menulists, etc. in our doorhanger notifications (e.g. permission prompts like this one) when they appear?

Flags: needinfo?(mreschenberg)
Flags: needinfo?(eitan)
Keywords: access
Whiteboard: [access-s2]

I can take a look at this today, but would love to hear if eitan has any ideas re: why the behaviour is different on reload. My guess is this has to do with how we handle children of a menupopup in ignoreWithParent. May be related to bug 1694261

I find VO doesn't automatically focus the popup, though it does say "firefox has new window", and it populates the tree with the popup. You can reach it by stepping out to the web area and VO+next'ing. Does this popup currently have an auto-focus attr or something else that should pull SR focus to it when opened?

Flags: needinfo?(mreschenberg)

The panel has role="alert" - would that do it?

Flags: needinfo?(mreschenberg)

(In reply to Mike Conley (:mconley) (:⚙️) (Catching up on needinfos) from comment #7)

The panel has role="alert" - would that do it?

its possible we have that role mapped incorrectly on mac ! I will look.

Flags: needinfo?(mreschenberg)

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

Note that the WebRTC permission panel changed in two ways recently:

Sorry, I missed that this is not related to Proton changes as noted in comment 0, so the changes I mentioned aren't relevant (and thus nor is any pref).

(In reply to Morgan Reschenberg [:morgan] from comment #6)

I find VO doesn't automatically focus the popup,

I wouldn't expect it to. You can focus it with f6, though.

though it does say "firefox has new window",

Even though it doesn't get VO focus, I would expect it to read the content of the alert as soon as it appears; the prompt, etc. Does it do this consistently for you?

Flags: needinfo?(mreschenberg)

Okay got it to reproduce and debugged a bit today. I think this is a live regions issue. When you open the dialog on an initial page load, you get the following series of events:

A11Y EVENTS: events fired; 48:36.811
  {
    type: alert
    target: 0x136345280; role: alert, idx: 10, node: 0x129d25310, panel@id="notification-popup" [ type="arrow" position="after_start" orient="vertical" noautofocus="true" role="alert" flip="both" side="top" consumeoutsideclicks="false" class="popup-notification-panel panel-no-padding" followanchor="true" noautohide="true" hasbeenopened="true" animate="open" animating="true" ]
  }

A11Y EVENTS: events fired; 48:36.811
  {
    type: live region added
    target: 0x136345280; role: alert, idx: 10, node: 0x129d25310, panel@id="notification-popup" [ type="arrow" position="after_start" orient="vertical" noautofocus="true" role="alert" flip="both" side="top" consumeoutsideclicks="false" class="popup-notification-panel panel-no-padding" followanchor="true" noautohide="true" hasbeenopened="true" animate="open" animating="true" ]
  }

A11Y EVENTS: events fired; 48:36.855
  {
    type: alert
    target: 0x136345280; role: alert, idx: 10, node: 0x129d25310, panel@id="notification-popup" [ type="arrow" position="after_start" orient="vertical" noautofocus="true" role="alert" flip="both" side="top" consumeoutsideclicks="false" class="popup-notification-panel panel-no-padding" followanchor="true" noautohide="true" hasbeenopened="true" animate="open" arrowposition="after_start" panelopen="true" ]
  }

but on subsequent reloads, you get the following (no live region added event):

A11Y EVENTS: events fired; 48:47.411
  {
    type: alert
    target: 0x136345280; role: alert, idx: 10, node: 0x129d25310, panel@id="notification-popup" [ type="arrow" position="after_start" orient="vertical" noautofocus="true" role="alert" flip="both" side="top" consumeoutsideclicks="false" class="popup-notification-panel panel-no-padding" followanchor="true" noautohide="true" hasbeenopened="true" arrowposition="after_start" animate="open" animating="true" ]
  }

A11Y EVENTS: events fired; 48:47.426
  {
    type: alert
    target: 0x136345280; role: alert, idx: 10, node: 0x129d25310, panel@id="notification-popup" [ type="arrow" position="after_start" orient="vertical" noautofocus="true" role="alert" flip="both" side="top" consumeoutsideclicks="false" class="popup-notification-panel panel-no-padding" followanchor="true" noautohide="true" hasbeenopened="true" arrowposition="after_start" animate="open" panelopen="true" ]
  }

I also don't see any live-region-removed events (though not sure if its necessary to send them when we're destroying moxAcc's because we send that UIElementDestroyed notification.) Looks like we hit this clause the first time around, but not after (https://searchfox.org/mozilla-central/rev/200bfc652c5128011e7725fc7c201eb125d453e7/accessible/mac/AccessibleWrap.mm#50).... suspicious

Flags: needinfo?(mreschenberg)

Yeah I think the issue is we're not re-creating the live region on refresh. For some reason, we never get a hide event for the notification popup (though we do for all its descendants). We also never get a reorder on its parent to indicate it's been removed (though we do get reorders on it when its descendants go away -- by descendants I mean the popup button, the regular buttons, and the static text).

In other live region examples (ie. https://dequeuniversity.com/library/aria/liveregion-playground) when the page is refreshed, the live region is recreated because the node is recreated. I guess maybe the popup is reused after its opened, despite page refreshes. I also found that the only way to re-trigger the original notification is to restart the browser or to open a new window. Just opening a new tab doesn't work -- which is why I think the popup is reused for any other notifications going forward.

We are correctly sending AXLiveRegionChanged notifications both times when the popup is populated. It seems VoiceOver ignores those without a proceeding creation event, however.

This is kinda tricky because normally we check if a node is a live region when we get a SHOW event for it, and if it is we fire an AXLiveRegionCreated event. However, because we only get the show once, we only fire that event once :( It sounds like we might need an exception for chrome-based live regions? Like, if they get reorders and they previously have 0 children, fire the event, maybe?

:Jamie where else in the browser do we use live regions apart from these alerts? I assume doorhangers would have similar behaviour 🤔
Also, how does NVDA deal with this kinda thing? I assume it doesn't really care if a region is created, just if we get changes within one?

Flags: needinfo?(jteh)

(In reply to Morgan Reschenberg [:morgan] from comment #11)

Yeah I think the issue is we're not re-creating the live region on refresh. For some reason, we never get a hide event for the notification popup (though we do for all its descendants). We also never get a reorder on its parent to indicate it's been removed

It never does get removed. The popup gets reused across the browser session. It's one of those weird XUL accessibles that gets the invisible state instead of being removed from the tree when it's "hidden". That said, we should still fire a hide event in that case, so that's definitely a core a11y bug.

We are correctly sending AXLiveRegionChanged notifications both times when the popup is populated. It seems VoiceOver ignores those without a proceeding creation event, however.

The thing I don't follow is: there is a creation event the first time the popup appears. If we're not firing a hide event, then we should never be telling Mac the live region got destroyed. So shouldn't VO still think the live region is created? And if not, why does it think the live region died if we didn't tell it so?

This is kinda tricky because normally we check if a node is a live region when we get a SHOW event for it, and if it is we fire an AXLiveRegionCreated event. However, because we only get the show once, we only fire that event once :(

We only fire it once, but we also never fire the destroyed event. Do we not respond to updates to a live region after it's been created?

:Jamie where else in the browser do we use live regions apart from these alerts? I assume doorhangers would have similar behaviour 🤔

The alert discussed in this bug is a doorhanger. The other place is a11yUtils.announce, but note that this case does not use that.

Also, how does NVDA deal with this kinda thing? I assume it doesn't really care if a region is created, just if we get changes within one?

NVDA treats "alerts" a little differently to normal live regions. Gecko fires an alert event whenever the children of an alert mutate. NVDA catches that and reads the alert.

Flags: needinfo?(jteh)
Flags: needinfo?(mreschenberg)

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

(In reply to Morgan Reschenberg [:morgan] from comment #11)

We are correctly sending AXLiveRegionChanged notifications both times when the popup is populated. It seems VoiceOver ignores those without a proceeding creation event, however.

The thing I don't follow is: there is a creation event the first time the popup appears. If we're not firing a hide event, then we should never be telling Mac the live region got destroyed. So shouldn't VO still think the live region is created? And if not, why does it think the live region died if we didn't tell it so?

I don't know why it doesn't maintain the live region as "active" in whatever internal accounting it does. Technically we don't do any special bookkeeping around "live region destroyed", we rely on the event that gets sent when an element expires, and we just expect VO to interpret that as "this element is gone, if it was a live region that live region is gone too". We currently (correctly) aren't firing that destroyed event, so VO in theory should never be notified of the destruction. My guess is there's something about a page refresh (ie. getting AXLoadComplete, AXLayoutComplete) that cause VO to invalidate its state. From looking at the Webkit code and tooling Safari's events, it looks like our event handling is consistent with how they handle live regions (ie. there's no special event for "destroy" and they only fire a creation event once per live region, unless the region is destroyed and then re-added to the tree. all subsequent changes get "AXLiveRegionChanged" events).

Flags: needinfo?(mreschenberg)

A few updates from today, and some questions:

  • Per your earlier comment, I started to investigate where we get hide events, and why we might be missing this dialog. To that end I added a check in DocAccessible::AttributeChanged to check if the regular hidden attr (not aria-hidden) was modified, and if so, to fire contentInserted or contentRemoved appropriately. I was able to verify that this got called on the alert, but it only got called once -- on the initial launch. From then on, subsequent open/close'ings didn't add/change the hidden attr, and so didn't get caught by the new if check :( I verified this by inspecting with the browser console before/after/during opening and closing -- the alert doesn't have a hidden attr past the first change.

  • Maybe it's another attribute? I used the inspector and found the popup gets panelOpen set to true when the alert is visible. The attr is removed when the alert is closed. I couldn't find an nsgkAtom for panelOpen though, so I wasn't sure how to listen for it in AttributeChanged or AttrChangedImpl. Is there a way to do that, or is that some kind of custom property/attribute? Looking around, that attr seemed to be tied to an event, though, so...

  • I went through the DOM events fired when that alert opens and closes, and I found five: popupShown, popupShowing, popupPositioned, popupHiding, popupHidden. I noticed we have support for popupShown and popupHiding here, but not for the other two similar ones, so I tried adding them. Since the alert fires both hidden/hiding and show/showing at the same time, I wasn't sure if there was a reason we were only supporting one. Is there? When I added handling, I had the popupShowing event do the same thing it does here, but for alerts too. The popupHidden one I called into the existing handler and also added alert to the existing tooltip if-case. When I step through these new cases with lldb, though, I never see the alert box show up even though in the JS it seems like those events are being dispatched on the alert. Also, because I'm working with elements and not accessibles, I wasn't sure how to check the role/type of what I do see show up. Is there a way to do that aside from attempting to create an acc from the given element?

Anyway, all this to say I've poked around to my limits for today. If there's anything you can think of that I haven't tried, lemme know! I think you're right too that the main problem here is we don't fire a hide event when we should. I think this might also be an issue for other doorhangers on Mac, like the "click here to update firefox" one that appears from the hamburger menu -- its contents remain navigable by VO even when it isn't open :( The bookmarks editor in the URL bar is another example. These don't deal with live-regions/live-region events, but I wonder if not firing a hide event means we don't properly expire the container.

Flags: needinfo?(jteh)

Okay. So given all of this, now we just have to figure out how to deal with it. :)

Possible solutions:

  1. Watch for Gecko EVENT_ALERT. If it's fired on a XUL element, fire Mac AXLiveRegionCreated.

    The problem with this is that there could be multiple alert events in some cases. That is probably a bug, but nevertheless one we have to deal with for now. Will VO barf if we fire multiple AXLiveRegionCreated events?

  2. Same as 1), but keep track of whether we've fired AXLiveRegionCreated for an Accessible. Clear that state when the alert is hidden.

    We can add code to handle the alert hiding in RootAccessible::HandlePopupHidingEvent. I guess we'd fire an AccStateChangeEvent for INVISIBLE, catch that in the Mac code and clear the flag.

  3. Remove hidden XUL alerts from the a11y tree when they hide, like we do for CSS hiding. This would be similar to bug 1652211. This is probably the correct solution long-term, but it's probably riskier short-term, especially if we want to uplift to beta.

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

Okay. So given all of this, now we just have to figure out how to deal with it. :)

Possible solutions:

  1. Watch for Gecko EVENT_ALERT. If it's fired on a XUL element, fire Mac AXLiveRegionCreated.

    The problem with this is that there could be multiple alert events in some cases. That is probably a bug, but nevertheless one we have to deal with for now. Will VO barf if we fire multiple AXLiveRegionCreated events?

Ah I tried a variant of this yesterday! Because we have two alert events for every alert, I was worried we'd get double notifications, so instead I modified this function to send both a create and a changed notification at once, instead of just the changed one. That seemed to be fine, and I didn't get double notifications since we're currently doing the changed notifs correctly. I can try mapping it to alert and see what VO thinks, if you think that's a cleaner solution.

  1. Same as 1), but keep track of whether we've fired AXLiveRegionCreated for an Accessible. Clear that state when the alert is hidden.

    We can add code to handle the alert hiding in RootAccessible::HandlePopupHidingEvent. I guess we'd fire an AccStateChangeEvent for INVISIBLE, catch that in the Mac code and clear the flag.

We sorta have some bookkeeping for this on the mac side already -- when a live region is added we mark the accessible as a live region and send the live region created event. We could pretty easily add another event handler that clears this var. The problem, though, is that we're not getting a platform event (EVENT_LIVE_REGION_ADDED) so we'd never re-enter the event handler where we'd fire the creation event :( its the same problem.

  1. Remove hidden XUL alerts from the a11y tree when they hide, like we do for CSS hiding. This would be similar to bug 1652211. This is probably the correct solution long-term, but it's probably riskier short-term, especially if we want to uplift to beta.

I will look at this! I have not explored CSS hiding.

Mid-air collision. :)

(In reply to Morgan Reschenberg [:morgan] from comment #14)

I noticed we have support for popupShown and popupHiding here, but not for the other two similar ones, so I tried adding them. Since the alert fires both hidden/hiding and show/showing at the same time, I wasn't sure if there was a reason we were only supporting one. Is there?

I'm not sure, but popuphiding should be sufficient.

When I added handling, I had the popupShowing event do the same thing it does here, but for alerts too. The popupHidden one I called into the existing handler and also added alert to the existing tooltip if-case. When I step through these new cases with lldb, though, I never see the alert box show up even though in the JS it seems like those events are being dispatched on the alert.

I'm not sure how you were checking for alerts, but it's not a XUL "alert" element; i.e. the tag name isn't "alert", so it's not quite as simple as "tooltip". Rather, it's a XUL "panel" element with role alert. Arguably, we should remove all hidden "panels" from the tree as well, but I worry about risk here.

Also, because I'm working with elements and not accessibles, I wasn't sure how to check the role/type of what I do see show up. Is there a way to do that aside from attempting to create an acc from the given element?

There should already be an acc. (If there isn't, we don't care, since it's already not in the a11y tree, so we aren't worried about it.) That said, there are only two ways a XUL panel can get role alert:

  1. It has ARIA role="alert" (in which case you can check the role attribute); or
  2. It has the attribute noautofocus="true".

I think you're right too that the main problem here is we don't fire a hide event when we should.

Does Mac react to hide events or just elements being removed from the tree? I'm guessing a hide event wouldn't be enough.

(In reply to Morgan Reschenberg [:morgan] from comment #16)

The problem, though, is that we're not getting a platform event (EVENT_LIVE_REGION_ADDED) so we'd never re-enter the event handler where we'd fire the creation event :( its the same problem.

I don't quite follow. Gecko EVENT_ALERT should fire (albeit twice :( ) whenever the alert becomes "visible".

I will look at this! I have not explored CSS hiding.

To be clear, CSS hiding is handled entirely separately. My point was that XUL panels are one of the few cases left where we don't remove them from the tree when they're hidden. The way to fix that is the same way Eitan did for XUL tooltips in bug 1652211.

Flags: needinfo?(jteh)
Assignee: nobody → mreschenberg
Attachment #9211136 - Attachment description: WIP: Bug 1699053: Ensure popup custom DOM events cause content insertion/removal on XUL panels. r?Jamie → Bug 1699053: Ensure popup custom DOM events cause content insertion/removal on XUL panels. r?Jamie
Status: NEW → ASSIGNED
Attachment #9211136 - Attachment description: Bug 1699053: Ensure popup custom DOM events cause content insertion/removal on XUL panels. r?Jamie → WIP: Bug 1699053: Ensure popup custom DOM events cause content insertion/removal on XUL panels. r?Jamie
Depends on: 1700735
Blocks: 1700735
No longer depends on: 1700735
Attachment #9211136 - Attachment description: WIP: Bug 1699053: Ensure popup custom DOM events cause content insertion/removal on XUL panels. r?Jamie → Bug 1699053: Ensure popup custom DOM events cause content insertion/removal on XUL panels. r?Jamie
Attachment #9211136 - Attachment description: Bug 1699053: Ensure popup custom DOM events cause content insertion/removal on XUL panels. r?Jamie → WIP: Bug 1699053: Ensure popup custom DOM events cause content insertion/removal on XUL panels. r?Jamie
Attachment #9211136 - Attachment description: WIP: Bug 1699053: Ensure popup custom DOM events cause content insertion/removal on XUL panels. r?Jamie → Bug 1699053: Ensure popup custom DOM events cause content insertion/removal on XUL panels. r?Jamie
Pushed by mreschenberg@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9cf3c2a874cd Ensure popup custom DOM events cause content insertion/removal on XUL panels. r=Jamie
Regressions: 1703562
Regressions: 1703620
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 89 Branch

Verified-fixed on latest Firefox Nightly 89.0a1 (2021-04-08) (64-bit) on MacOS 10.15 and MacOS M1 11.
All the information in the doorhanger was read out loud when the doorhanger got displayed. I tried when we have only labels, menulist and both in the same doorhanger and it worked fine.

However, I submitted a follow-up Bug 1703815 since the "Remember this decision", "Block", "Allow" string are read out loud 2 times.

Please nominate this for Beta approval when you get a chance.

Flags: needinfo?(eitan) → needinfo?(mreschenberg)

Comment on attachment 9211136 [details]
Bug 1699053: Ensure popup custom DOM events cause content insertion/removal on XUL panels. r?Jamie

Beta/Release Uplift Approval Request

  • User impact if declined: Users that rely on VoiceOver will not be notified of permission alerts on subsequent page loads.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): While this patch changes how alerts function for all platforms, it increases the amount of events we provide to assistive technologies meaning AT's that are already happy with the provided set of events will not be adversely affected. It also properly removes panels from the tree when they are hidden, which makes their behaviour more typical in terms of AT expectations.
  • String changes made/needed:
Flags: needinfo?(mreschenberg)
Attachment #9211136 - Flags: approval-mozilla-beta?
Blocks: xula11y

Comment on attachment 9211136 [details]
Bug 1699053: Ensure popup custom DOM events cause content insertion/removal on XUL panels. r?Jamie

Approved for 88.0rc1.

Attachment #9211136 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Verified fixed on RC 88.0 (64-bit) on macOS 10.15.

Status: RESOLVED → VERIFIED
Accessibility Severity: --- → s2
Whiteboard: [access-s2]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: