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

Status: RESOLVED FIXED

(Firefox :: Site Identity, defect, P2)

Description

[Marco Zehe (:MarcoZ)]

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.

Comment 1

[Johann Hofmann [:johannh]]

Attachment: Bug 1335016 - Add tooltip to identity popup expander button.

Review commit: https://reviewboard.mozilla.org/r/116278/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116278/

Comment 2

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9147f9d83f41

Comment 3

[Johann Hofmann [:johannh]]

Marco, can you please check if this patch works for you? Thanks!

Comment 4

[Johann Hofmann [:johannh]]

Checked the copy with phlsa on IRC and we agreed on "Show connection details" and "Hide connection details".

Comment 5

[Johann Hofmann [:johannh]]

Comment on attachment 8842419 [details]
Bug 1335016 - Add tooltip to identity popup expander button.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116278/diff/1-2/

Comment 6

[Florian Quèze [:florian]]

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.

Comment 7

[Marco Zehe (:MarcoZ)]

Comment on attachment 8842419 [details]
Bug 1335016 - Add tooltip to identity popup expander button.

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

Comment 8

[Johann Hofmann [:johannh]]

Comment on attachment 8842419 [details]
Bug 1335016 - Add tooltip to identity popup expander button.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116278/diff/2-3/

Comment 9

[Johann Hofmann [:johannh]]

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!

Comment 10

[Florian Quèze [: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?

Comment 11

[Johann Hofmann [:johannh]]

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 12

[Johann Hofmann [:johannh]]

Comment on attachment 8842419 [details]
Bug 1335016 - Add tooltip to identity popup expander button.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116278/diff/3-4/

Comment 13

[Florian Quèze [:florian]]

Comment on attachment 8842419 [details]
Bug 1335016 - Add tooltip to identity popup expander button.

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/295fe482610d
Add tooltip to identity popup expander button. r=florian,MarcoZ

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/295fe482610d

Comment 16

[Julien Cristau [:jcristau]]

this adds new strings, so not a candidate for beta.