1697295 - When Proton is enabled, hide the "Choose Camera", "Choose Microphone" labels, and replace with icons

Status: RESOLVED FIXED

(Firefox :: Site Permissions, enhancement, P1)

Description

[Mike Conley (:mconley) (:⚙️)]

These icons should be immediately to the left of the device selectors. We also need to make sure that we don't accidentally harm accessibility here - with the labels hidden, we need to make sure that the icons properly label the menulist elements with clear descriptions of what they're for.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1697295 - Replace camera and microphone labels with icons in WebRTC permission panel when Proton is enabled. r?pbz!,Gijs!

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Hey Jamie,

So in the attached patch, with Proton enabled (browser.proton.doorhangers.enabled), the labels above the microphone and camera selectors are hidden, and we instead show icons for those selectors. I'm using tooltiptext on the icons, and aria-labelledby on the menulist nodes pointing at the icons to give them labels, and the Accessibility panel in the Browser Toolbox makes me think this is working, but I want to make sure it makes sense.

Also, the tooltiptext strings themselves, "Camera" and "Microphone", aren't final - I'm waiting for those from betsymikal. But hopefully this patch demonstrates what I'm thinking.

Comment 3

[James Teh [:Jamie]]

Hmm. This is going to be tricky.

On the surface, your patch looks good, including in the A11y Inspector. Unfortunately, as I suspected, it doesn't work so well with NVDA in practice. The problem is that images aren't included when NVDA builds dialog/alert text because they aren't considered to be text. So, when the alert appears, you get this (I have one camera and two microphones):

alert  Will you allow webrtc.github.io to use your camera and microphone?
Integrated Camera
Microphone  combo box  Microphone (Logitech USB Headset)  collapsed
Remember this decision  check box  not checked
Don’t Allow  button  Alt+D
Allow  button  Alt+A

Note that you hear "Integrated Camera", but not "Camera" before it. In contrast, you do hear "Microphone" before the dropdown because it is the label of the dropdown.

This is one of those cases where screen reader "guessing" heuristics get it wrong. :(

Fixing this is not going to be easy or clean. The only thing I've been able to come up with is using aria-describedby on the alert to explicitly specify the elements which make up the dialog text. However, that turned out not to be feasible in a previous attempt (bug 1693644 comment 5):

Argh. That's harder than I thought it'd be. The description is actually generated by the popupnotification custom element binding.

The only other option I can think of is manually setting aria-description to contain all the text we want (including the "Will you allow webrtc.github.io to use your camera and microphone?"), but that's really hacky and also fragile if the notification ever mutates (apart from the value of the dropdown).

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Thanks, Jamie. Is there precedent for changing parts of the UI when we know that software like NVDA is connected? I know that, at least at one point, we would show an indication when "accessibility services" were enabled. Would it be feasible to fallback to the original behaviour with the labels rather than the icons if those services are in use?

Comment 5

[James Teh [:Jamie]]

There isn't precedent for this in Firefox (aside from the indicator, which is now disabled) and I'd rather not introduce such differentiation. Aside from the obvious concerns around "accessibility specific behaviour", there are privacy concerns too; e.g. a screenshot might reveal that a user is using an a11y tool because they get a label and not an icon. Also, tying it to specific a11y tools (e.g. NVDA, JAWS) is brittle and not future proof, and tying it to any a11y service might impact IME users, users of certain enterprise SSO tools, etc.

I'm trying to dream up some other hack to get around this, but I'm drawing a blank so far.

Comment 6

[James Teh [:Jamie]]

This (maybe ugly?) patch fixes the problem by giving the description element an id and allowing notifications to take an ariaDescribedBy option which gets set on the panel when it's shown. I don't know this code at all, so there might be a cleaner way to do this. Also, this patch doesn't account for the case where the menulist is shown. In that case, ariaDescribedBy needs to be changed to not reference the icon or the label, since the control is interactive and thus shouldn't be included in the dialog text.

diff --git a/browser/actors/WebRTCParent.jsm b/browser/actors/WebRTCParent.jsm
index 8688a0bca190c..955c79a76eca9 100644
--- a/browser/actors/WebRTCParent.jsm
+++ b/browser/actors/WebRTCParent.jsm
@@ -1132,6 +1132,7 @@ function prompt(aActor, aBrowser, aRequest) {
       // If we haven't handled the permission yet, we want to show the doorhanger.
       return false;
     },
+    ariaDescribedBy: "webRTC-shareDevices-notification-description webRTC-selectCamera-icon webRTC-selectCamera-label webRTC-selectMicrophone-icon webRTC-selectMicrophone-label",
   };
 
   function shouldShowAlwaysRemember() {
diff --git a/toolkit/content/widgets/popupnotification.js b/toolkit/content/widgets/popupnotification.js
index 1ec4c6f5a5c3f..272eacfd0e30b 100644
--- a/toolkit/content/widgets/popupnotification.js
+++ b/toolkit/content/widgets/popupnotification.js
@@ -144,6 +144,9 @@
         this.appendNotificationContent(popupnotificationcontent);
       }
 
+      const desc = this.querySelector(".popup-notification-description");
+      desc.id = this.getAttribute("popupid") + "-notification-description";
+
       this.initializeAttributeInheritance();
     }
 
diff --git a/toolkit/modules/PopupNotifications.jsm b/toolkit/modules/PopupNotifications.jsm
index 2fc54a589969b..dba3974a2e835 100644
--- a/toolkit/modules/PopupNotifications.jsm
+++ b/toolkit/modules/PopupNotifications.jsm
@@ -1156,6 +1156,12 @@ PopupNotifications.prototype = {
 
       this.panel.appendChild(popupnotification);
 
+      if (n.options.ariaDescribedBy) {
+        this.panel.setAttribute("aria-describedby", n.options.ariaDescribedBy);
+      } else {
+        this.panel.removeAttribute("aria-describedby");
+      }
+
       // The popupnotification may be hidden if we got it from the chrome
       // document rather than creating it ad hoc.
       popupnotification.show();

Comment 7

[James Teh [:Jamie]]

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

this patch doesn't account for the case where the menulist is shown. In that case, ariaDescribedBy needs to be changed to not reference the icon or the label, since the control is interactive and thus shouldn't be included in the dialog text.

Ug. That's not going to be feasible with my approach because that decision is made in options.eventCallback. So somehow options.eventCallback needs to set a value for aria-describedby which will be picked up by the panel when it shows. I'm not sure if it's possible to access the notification options from eventCallback. If not, I guess it'll have to be set as an attribute (but not aria-describedby itself) on the popupnotification?

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Thanks for helping me out with this, Jamie. So, suppose I'm able to set aria-describedby on the panel element (which has role="alert" on it) before its popupshown event fires. Presuming that I get the IDs right in the attribute, would that sufficiently describe the purpose of the panel?

Comment 9

[James Teh [:Jamie]]

It should, yes. Please note the importance of not including any interactive controls or labels for interactive controls, though. That is, if there is a menulist, do not include it or its label.

Please let me know if you need any help with this.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Okay, I think I have something like what you described working in my latest patch in https://phabricator.services.mozilla.com/D107718. If you have a second, would you mind checking to see if this makes it clearer what the panel is asking for?

Comment 11

[James Teh [:Jamie]]

Ug. My tree is out of date, so I need to wait a year and a day for it to build... but the first thing that occurs to me looking at the patch is that the description element doesn't have an id set, so that isn't going to work. Was there perhaps a file missing from your patch? See my patch above for toolkit/content/widgets/popupnotification.js for one way of doing this.

Otherwise, the patch looks good, but I'll test it for real once my build is done.

Comment 12

[James Teh [:Jamie]]

This patch seems pretty broken for me. As I suspected in comment 11, the description doesn't have an id. Also, the labels for the single camera/microphone to be shared seem to be always hidden.

Looking more closely, I don't think this patch differentiates between the menulist and non-menulist cases for aria-describedby. It differentiates based on whether microphone or camera is relevant at all (0 devices vs non-0), but not based on whether the menulist is shown (1 vs > 1). For the menulist case, it must not include the label or icon in aria-describedby.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Ack, sorry, I misunderstood about when to set the label/icon. I'll have a new patch up tomorrow. Thanks for testing it!

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Okay Jamie, I think I have something that works better - I found the "description" attribute in the accessibility devtools inspector, and it seems like it's reading out reasonable things. Would you mind checking if the latest patch does what you would expect?

Comment 15

[James Teh [:Jamie]]

Works nicely. Thank you. Sorry this has been so painful.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5d2a519de39c
Replace camera and microphone labels with icons in WebRTC permission panel when Proton is enabled. r=pbz,Jamie,fluent-reviewers,desktop-theme-reviewers,harry,Gijs

Comment 17

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/5d2a519de39c