Closed Bug 1335016 Opened 3 years ago Closed 3 years ago

[a11y] The identity-popup-security-expander button has no label, doesn't indicate expanded state to screen readers.

Categories

(Firefox :: Site Identity, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox51 --- wontfix
firefox52 --- wontfix
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: MarcoZ, Assigned: johannh)

References

Details

(Keywords: access, Whiteboard: [fxprivacy])

Attachments

(1 file)

After opening the site identity popup, and tabbing once, you land on a button with ID identity-popup-security-expander. This button only announces as "button" to screen readers. Its visual label probably comes from some CSS background image or Unicode symbol that screen readers don't speak. Also, it appears to be a toggle between showing and hiding the information.

To solve, give the same element an aria-label attribute with a localized string that says "Show Security Info" and "Hide Security Info" or something similarly sensible, depending on whether the button is currently in the state of showing the element, or hiding it, when pressed.
Whiteboard: [fxprivacy] [triage]
Priority: -- → P2
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Marco, can you please check if this patch works for you? Thanks!
Checked the copy with phlsa on IRC and we agreed on "Show connection details" and "Hide connection details".
Comment on attachment 8842419 [details]
Bug 1335016 - Add tooltip to identity popup expander button.

https://reviewboard.mozilla.org/r/116278/#review117864

::: browser/base/content/browser.js:6885
(Diff revision 2)
>  
>    toggleSubView(name, anchor) {
>      let view = this._identityPopupMultiView;
>      if (view.showingSubView) {
>        view.showMainView();
> +      this._popupExpander.tooltipText = gNavigatorBundle.getString("identity.showDetails.tooltip");

nit: you can reduce code duplication by putting the string id in a variable and doing a single getString call.
Attachment #8842419 - Flags: review?(florian) → review+
Comment on attachment 8842419 [details]
Bug 1335016 - Add tooltip to identity popup expander button.

https://reviewboard.mozilla.org/r/116278/#review119076
Attachment #8842419 - Flags: review?(mzehe) → review+
Comment on attachment 8842419 [details]
Bug 1335016 - Add tooltip to identity popup expander button.

There was a problem where the button would not receive the "Show connection details" text after showing and hiding the sub view, since showing the main view does not cause the "toggleSubView" function (contrary to what that code says). This should fix it.

Florian, can you quickly go over this again? Thank you!
Attachment #8842419 - Flags: review+ → review?(florian)
Comment on attachment 8842419 [details]
Bug 1335016 - Add tooltip to identity popup expander button.

https://reviewboard.mozilla.org/r/116278/#review119170

::: browser/base/content/browser.js:6892
(Diff revision 3)
> +    let id = `identity-popup-${name}View`;
> +    let subView = document.getElementById(id);
> +
> +    // Listen for the subview showing or hiding to change
> +    // the tooltip on the expander button.
> +    subView.addEventListener("ViewShowing", this);

Do we really need to add these event listeners each time? I guess it doesn't really matter as addEventListener will discard duplicates.

::: browser/base/content/browser.js:7425
(Diff revision 3)
> +    if (event.type == "ViewShowing") {
> +      this._popupExpander.tooltipText = gNavigatorBundle.getString("identity.hideDetails.tooltip");
> +    } else if (event.type == "ViewHiding") {
> +      this._popupExpander.tooltipText = gNavigatorBundle.getString("identity.showDetails.tooltip");
> +    }
> +

Is a return statement somehow missing here?
Attachment #8842419 - Flags: review?(florian)
Comment on attachment 8842419 [details]
Bug 1335016 - Add tooltip to identity popup expander button.

https://reviewboard.mozilla.org/r/116278/#review119196

::: browser/base/content/browser.js:6892
(Diff revision 3)
> +    let id = `identity-popup-${name}View`;
> +    let subView = document.getElementById(id);
> +
> +    // Listen for the subview showing or hiding to change
> +    // the tooltip on the expander button.
> +    subView.addEventListener("ViewShowing", this);

Yeah that was my thought, too. It's how it's done a couple of times in other code and since toggleSubView is theoretically able to support multiple subviews (though we only have one atm) it's the only way to make sure we actually listen to *that* subView that was just opened.

If you really prefer it, we could add an init function where we add the listener.

::: browser/base/content/browser.js:7425
(Diff revision 3)
> +    if (event.type == "ViewShowing") {
> +      this._popupExpander.tooltipText = gNavigatorBundle.getString("identity.hideDetails.tooltip");
> +    } else if (event.type == "ViewHiding") {
> +      this._popupExpander.tooltipText = gNavigatorBundle.getString("identity.showDetails.tooltip");
> +    }
> +

I left it out since the below code technically works without it, but I guess it would also be safe to return here.
Comment on attachment 8842419 [details]
Bug 1335016 - Add tooltip to identity popup expander button.

https://reviewboard.mozilla.org/r/116278/#review119210
Attachment #8842419 - Flags: review?(florian) → review+
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/295fe482610d
Add tooltip to identity popup expander button. r=florian,MarcoZ
https://hg.mozilla.org/mozilla-central/rev/295fe482610d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
this adds new strings, so not a candidate for beta.
You need to log in before you can comment on or make changes to this bug.