Closed Bug 1335018 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Site Identity, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 54
Tracking Status
firefox51 --- wontfix
firefox52 + verified
firefox-esr52 --- fixed
firefox53 --- verified
firefox54 --- verified

People

(Reporter: MarcoZ, Assigned: johannh)

References

Details

(Keywords: access, Whiteboard: [fxprivacy])

Attachments

(1 file)

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).
Priority: -- → P1
Whiteboard: [fxprivacy]
Priority: P1 → --
Whiteboard: [fxprivacy] → [fxprivacy] [triage]
Priority: -- → P2
[Tracking Requested - why for this release]: Low-hanging a11y fruit that should make it to ESR
Priority: P2 → P1
Duplicate of this bug: 1335019
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
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
Attachment #8835560 - Flags: review?(mzehe) → review+
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.
Attachment #8835560 - Flags: review?(gijskruitbosch+bugs) → review-
(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.
Blocks: 1335737
(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?)
Tracking a11y issue for 52.
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 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?
Attachment #8835560 - Flags: review?(gijskruitbosch+bugs) → review+
(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 :)
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/de990775a354
Make the side identity popup adhere to role=alertdialog for a11y. r=Gijs,MarcoZ
Duplicate of this bug: 1335737
Flags: qe-verify+
See Also: → 1340112
https://hg.mozilla.org/mozilla-central/rev/de990775a354
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Will you ask for uplift to 52 and 53 please?
Flags: needinfo?(jhofmann)
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
Flags: needinfo?(jhofmann)
Attachment #8835560 - Flags: approval-mozilla-beta?
Attachment #8835560 - Flags: approval-mozilla-aurora?
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+
Attachment #8835560 - Flags: approval-mozilla-beta?
Attachment #8835560 - Flags: approval-mozilla-beta+
Attachment #8835560 - Flags: approval-mozilla-aurora?
Attachment #8835560 - Flags: approval-mozilla-aurora+
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?
Flags: needinfo?(jhofmann)
Please make sure that "Preferences -> Keyboard -> Shortcuts -> All controls" is checked on all machines. Otherwise this should not focus anything on OSX I think.
Flags: needinfo?(jhofmann) → needinfo?(simona.marcu)
(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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(simona.marcu)
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
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]
Updating tracking flags based on the above comments. Thanks for the verification! ;)
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
Flags: needinfo?(jhofmann)
Depends on: 1343210
Good point, filed bug 1343210. We should only do that when the identity popup is opened with the keyboard.
Flags: needinfo?(jhofmann)
> 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.
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
Duplicate of this bug: 1335737
You need to log in before you can comment on or make changes to this bug.