Marco Zehe (:MarcoZ) Reporter
	Description • 8 years ago

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.

	Johann Hofmann [:johannh] Assignee
	Comment 2 • 7 years ago

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

	Johann Hofmann [:johannh] Assignee
	Comment 3 • 7 years ago

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

	Johann Hofmann [:johannh] Assignee
	Comment 4 • 7 years ago

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

	Florian Quèze [:florian]
	Comment 6 • 7 years ago
mozreview-review

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.

	Marco Zehe (:MarcoZ) Reporter
	Comment 7 • 7 years ago
mozreview-review

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

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

	Johann Hofmann [:johannh] Assignee
	Comment 9 • 7 years ago

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!

	Florian Quèze [:florian]
	Comment 10 • 7 years ago
mozreview-review

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?

	Johann Hofmann [:johannh] Assignee
	Comment 11 • 7 years ago
mozreview-review

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.

	Florian Quèze [:florian]
	Comment 13 • 7 years ago
mozreview-review

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

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

	Pulsebot
	Comment 14 • 7 years ago

Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/295fe482610d
Add tooltip to identity popup expander button. r=florian,MarcoZ

	Carsten Book [:Tomcat]
	Comment 15 • 7 years ago
bugherder

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

	Julien Cristau [:jcristau]
	Comment 16 • 7 years ago

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