1335018 - [a11y] Make the site identity popup itself more accessible.

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P1)

Description

[Marco Zehe (:MarcoZ)]

To improve the site identity popup, give the element with ID identity-popup a role "alertdialog". It has many characteristics of a dialog, like it having to be dismissed somehow, and several interactive elements, but it also should speak immediately when pressed. The information should be picked up automatically from its children, so this role should, in theory, suffice (famous last words).

Comment 1

[Johann Hofmann [:johannh]]

[Tracking Requested - why for this release]: Low-hanging a11y fruit that should make it to ESR

Comment 2

[Johann Hofmann [:johannh]]

Attachment: Bug 1335018 - Make the side identity popup adhere to role=alertdialog for a11y.

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

Comment 4

[Marco Zehe (:MarcoZ)]

Comment on attachment 8835560 [details]
Bug 1335018 - Make the side identity popup adhere to role=alertdialog for a11y.

https://reviewboard.mozilla.org/r/111252/#review112486

Comment 5

[:Gijs (he/him)]

Comment on attachment 8835560 [details]
Bug 1335018 - Make the side identity popup adhere to role=alertdialog for a11y.

https://reviewboard.mozilla.org/r/111252/#review112510

::: browser/base/content/browser.js:7329
(Diff revision 1)
>      this._identityPopup.openPopup(this._identityIcon, "bottomcenter topleft");
>    },
>  
>    onPopupShown(event) {
>      if (event.target == this._identityPopup) {
> +      document.getElementById("identity-popup-security-expander").focus();

This doesn't do anything on OS X, I assume, if focusing buttons is not enabled? Or if it does, we should maybe make it not do anything.

In any case, I'm a bit confused why this is necessary, as the panel doesn't have `noautofocus`. Can you elaborate? I also don't see anything about focus in the original report.

Comment 6

[Johann Hofmann [:johannh]]

(In reply to :Gijs from comment #5)
> Comment on attachment 8835560 [details]
> Bug 1335018 - Make the side identity popup adhere to role=alertdialog for
> a11y.
> 
> https://reviewboard.mozilla.org/r/111252/#review112510
> 
> ::: browser/base/content/browser.js:7329
> (Diff revision 1)
> >      this._identityPopup.openPopup(this._identityIcon, "bottomcenter topleft");
> >    },
> >  
> >    onPopupShown(event) {
> >      if (event.target == this._identityPopup) {
> > +      document.getElementById("identity-popup-security-expander").focus();
> 
> This doesn't do anything on OS X, I assume, if focusing buttons is not
> enabled? Or if it does, we should maybe make it not do anything.
> 

Egh, right, this looks broken on OSX with the default setting. Do you know how to check whether button focus is enabled in OSX?

> In any case, I'm a bit confused why this is necessary, as the panel doesn't
> have `noautofocus`. Can you elaborate? I also don't see anything about focus
> in the original report.

The rationale was that the screenreader will not start reading unless an element inside the dialog is focused. Doing this is required if you assign the alertdialog role (https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_alertdialog_role).

There was also bug 1335019 which asked for exactly that, I marked it as dupe since this needs to be done here anyway.

Comment 7

[:Gijs (he/him)]

(In reply to Johann Hofmann [:johannh] from comment #6)
> > In any case, I'm a bit confused why this is necessary, as the panel doesn't
> > have `noautofocus`. Can you elaborate? I also don't see anything about focus
> > in the original report.
> 
> The rationale was that the screenreader will not start reading unless an
> element inside the dialog is focused. Doing this is required if you assign
> the alertdialog role
> (https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/
> ARIA_Techniques/Using_the_alertdialog_role).
> 
> There was also bug 1335019 which asked for exactly that, I marked it as dupe
> since this needs to be done here anyway.

My understanding is that this should happen by default for panels that are shown and don't have noautofocus. Do you not see focus being moved into the panel? Is there maybe other code that's interfering with that? The fix from bug 1171085 relied on this bit working, and I don't think we have custom focus for the other permission panel things which also use role=alertdialog (but I could be wrong! What do we do there?)

Comment 8

[Julien Cristau [:jcristau]]

Tracking a11y issue for 52.

Comment 9

[Johann Hofmann [:johannh]]

Comment on attachment 8835560 [details]
Bug 1335018 - Make the side identity popup adhere to role=alertdialog for a11y.

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

Comment 10

[Johann Hofmann [:johannh]]

So after a lot of conversations on IRC here's the summary:

- Focusing popups/panels is not a thing. It's a no-op.
- alertdialog does not automatically move focus into the panel. The focus manager will, however, give the next focus to an open panel (if noautofocus is not set).
- There are problems with that behavior for the identity popup if multiple panels are open, however (bug 1335737)
- Manually focusing the next element is, according to Marco and Neil, still the best thing to do.
- We can use document.commandDispatcher.advanceFocusIntoSubtree(popup) instead of focusing an arbitrary element. This also solves the OSX focus issues.

Comment 11

[:Gijs (he/him)]

Comment on attachment 8835560 [details]
Bug 1335018 - Make the side identity popup adhere to role=alertdialog for a11y.

https://reviewboard.mozilla.org/r/111252/#review114140

Tentative r=me given I saw all the debate about this on IRC. I'm still confused why we need to do this - my impression was that panels without noautofocus already did the equivalent of advanceFocusIntoSubtree. If they don't, I don't understand what the point of "noautofocus" on a panel is. Enn would know more about that, I guess?

Comment 12

[Johann Hofmann [:johannh]]

(In reply to :Gijs from comment #11)
> Comment on attachment 8835560 [details]
> Bug 1335018 - Make the side identity popup adhere to role=alertdialog for
> a11y.
> 
> https://reviewboard.mozilla.org/r/111252/#review114140
> 
> Tentative r=me given I saw all the debate about this on IRC. I'm still
> confused why we need to do this - my impression was that panels without
> noautofocus already did the equivalent of advanceFocusIntoSubtree. If they
> don't, I don't understand what the point of "noautofocus" on a panel is. Enn
> would know more about that, I guess?

AFAIU from reading nsFocusManager the focus changes to the panel but does not advance into the panel on popupshown, but pressing tab always advances focus into an open panel without noautohide. I hope that makes sense :)

Comment 13

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=932d597e9e37

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/de990775a354
Make the side identity popup adhere to role=alertdialog for a11y. r=Gijs,MarcoZ

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/de990775a354

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

Will you ask for uplift to 52 and 53 please?

Comment 18

[Johann Hofmann [:johannh]]

Comment on attachment 8835560 [details]
Bug 1335018 - Make the side identity popup adhere to role=alertdialog for a11y.

Approval Request Comment
[Feature/Bug causing the regression]: None, original implementation was faulty
[User impact if declined]: The site identity popup is not properly focusable with the keyboard and does not read well with screenreaders.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: It works for me on Nightly.
[Needs manual test from QE? If yes, steps to reproduce]:  Focus the urlbar and Shift+Tab your way to the identity block. Hit enter or space. The first item in the identity popup should be selected.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Very small frontend-only changes (one line of markup and one line of code).
[String changes made/needed]: None

Comment 19

[Julien Cristau [:jcristau]]

Comment on attachment 8835560 [details]
Bug 1335018 - Make the side identity popup adhere to role=alertdialog for a11y.

a11y for site identity popup, aurora53+, beta52+

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/32f96350bb49

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/85a6f67ca811

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/85a6f67ca811

Comment 23

[Simona Badau, Desktop QA]

Verified as fixed using the latest Nightly 54.0a1 (Build ID: 20170222030329) on Ubuntu 16.04 x64 and Windows 10 x64 (used the instructions from Comment 18).

But, the results on Mac OS X machines are quite inconsistent. This is fixed on Mac OS X 10.10 on an iMac (21.5-inch, Late 2013) and Mac OS X 10.11 on a MacBook Pro (Retina, 15-inch, Late 2013) but it's not fixed on Mac OS X 10.10/10.11/10.12 on a Mac mini (Mid,2011).

Johann, do you have any thoughts on this? Should I log a new bug for this?

Comment 24

[Johann Hofmann [:johannh]]

Please make sure that "Preferences -> Keyboard -> Shortcuts -> All controls" is checked on all machines. Otherwise this should not focus anything on OSX I think.

Comment 25

[Simona Badau, Desktop QA]

(In reply to Johann Hofmann [:johannh] from comment #24)
> Please make sure that "Preferences -> Keyboard -> Shortcuts -> All controls"
> is checked on all machines. Otherwise this should not focus anything on OSX
> I think.

Thanks Johann! That did the trick, the first item in the identity popup is now selected on Mac OS X 10.10 and Mac OS X 10.11 on the Mac mini (Mid,2011) machine. 

Based on this and on Comment 23, setting the status-firefox54: to verified.

Comment 26

[Azmina]

I have reproduced this bug with Nightly 53.0a1 (2016-12-31) on Windows 8.1 (64 bit!).

This bug's fix is verified on Aurora 53.0a2, Beta 52.0b8.

Build ID : 20170222004022
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0

Build ID : 20170220070057
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0

Comment 27

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 53.0a1 on Ubuntu 16.04 LTS!

This bug's fix is verified with Aurora 53.0a2, Beta 52.0b8!

Build ID   : 20170223084136
User Agent : Mozilla/5.0 (X11; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0

Build ID   : 20170220070057
User Agent : Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0

[bugday-20170222]

Comment 28

[Cornel Ionce [:noni] [Hubs QA]]

Updating tracking flags based on the above comments. Thanks for the verification! ;)

Comment 29

[Matthew N. [:MattN]]

This caused a focus ring to appear on the button to open the subview for linux64 and windows8-64 but not windows7: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=a9ec72f82299250e6023988e238931bbca0ef7fa&newProject=mozilla-central&newRev=8c8b54b13be7ec12cb8e104b772162a80b524497&filter=controlCenter

I think UX doesn't like focus rings showing up if the user didn't use the keyboard to move focus. Was this a known issue or considered?

Possibly unrelated: This was the first revision that started intermittent screenshot issues with a broken controlCenter layout (Is this like nhnt11 saw last week?) but I'm not sure if this bug is the cause: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=a9ec72f82299250e6023988e238931bbca0ef7fa&newProject=mozilla-central&newRev=8c8b54b13be7ec12cb8e104b772162a80b524497&filter=%5Eosx.*controlCenter

Comment 30

[Johann Hofmann [:johannh]]

Good point, filed bug 1343210. We should only do that when the identity popup is opened with the keyboard.

Comment 31

[Johann Hofmann [:johannh]]

> Possibly unrelated: This was the first revision that started intermittent screenshot issues with a broken controlCenter layout (Is this like nhnt11 saw last week?) but I'm not sure if this bug is the cause

Yes, that's very strange. If this bug triggered it then at least bug 1343210 will make the problem only happen on keyboard navigation. I guess the root cause is somewhere deeper on platform level. The patch isn't doing anything out of the ordinary afaics.