1055973 - In-content prefs should trap focus (tab key navigation) in subdialogs and the browser UI when they are open (it should not be possible to focus items in the main page)

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P3)

Description

[Alice0775 White]

Steps To Reproduce:
1. Open Preferences and sub-dialog
2. Press TAB key several times

Actual Results:
The focus element moves to main preferences which is behind of sub-dialog now

Expected Results:
The focus element should be only in between the browser UI and the sub-dialog

Comment 1

[:Gijs (he/him)]

Marco, do you know if there is a sane "web" way to constrain tab selection to items in a given container (element)? Of course, we could steal the tab key, but I'd prefer something that felt less hacky... :-\

Comment 2

[Marco Zehe (:MarcoZ)]

Nope, there is only the way of specifically controlling the tab key when it reaches the last element, or the first (when shift-tabbing), and define where it should go next. This is what we get when putting stuff in content that used to be modal, we have to make sure of the modaility ourselves. Every web author faces this when constructing overlays, popups or dialogs and wants to implement full keyboard support.

Comment 3

[Fabian]

It seems bug 840640 (the dialog element) and bug 921504 (inert attribute) are relevant here. In the absence of such support, it seems setting tabindex=-1 on all interactive elements except those in the dialog is the necessary workaround.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

https://www.marcozehe.de/2015/02/05/advanced-aria-tip-2-accessible-modal-dialogs/

Comment 5

[:Gijs (he/him)]

Attachment: prevent focus from leaving the dialog for anything but the browser UI,

This might fix bug 1128175 while we're here.

Comment 6

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #5)
> Created attachment 8575965 [details] [diff] [review]
> prevent focus from leaving the dialog for anything but the browser UI,
> 
> This might fix bug 1128175 while we're here.

Nope. Still, patch works otherwise.

Comment 7

[:Gijs (he/him)]

Comment on attachment 8575965 [details] [diff] [review]
prevent focus from leaving the dialog for anything but the browser UI,

Egh, nope, not on Windows. I love focus, I really do.

Comment 8

[:Gijs (he/him)]

Attachment: prevent focus from leaving the dialog for anything but the browser UI,

Now with the right window and taking into account the node might be anonymous, esp. for dialog buttons.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8575997 [details] [diff] [review]
prevent focus from leaving the dialog for anything but the browser UI,

Review of attachment 8575997 [details] [diff] [review]:
-----------------------------------------------------------------

Built and tested on Windows 8.1 and it works good.

::: browser/components/preferences/in-content/subdialogs.js
@@ +214,3 @@
>      this._overlay.style.opacity = ""; // XXX: focus hack continued from _onContentLoaded
> +
> +    this._trapFocus();

_untrapFocus is called from _removeDialogEventListeners. It would be nice if _trapFocus was called from _addDialogEventListeners so that we have symmetry.

Comment 10

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> Comment on attachment 8575997 [details] [diff] [review]
> prevent focus from leaving the dialog for anything but the browser UI,
> 
> Review of attachment 8575997 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Built and tested on Windows 8.1 and it works good.
> 
> ::: browser/components/preferences/in-content/subdialogs.js
> @@ +214,3 @@
> >      this._overlay.style.opacity = ""; // XXX: focus hack continued from _onContentLoaded
> > +
> > +    this._trapFocus();
> 
> _untrapFocus is called from _removeDialogEventListeners. It would be nice if
> _trapFocus was called from _addDialogEventListeners so that we have symmetry.

As discussed on IRC, it needs to have the content document for the frame, that's why it's in onLoad.

Try push, as fx-team is closed anyway:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=abf6376ccbb9

Comment 11

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/3aea1d4e06c5

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3aea1d4e06c5

Comment 13

[:Gijs (he/him)]

Comment on attachment 8575997 [details] [diff] [review]
prevent focus from leaving the dialog for anything but the browser UI,

Approval Request Comment
[Feature/regressing bug #]: in-content prefs
[User impact if declined]: confusing keyboard accessibility on the in-content prefs
[Describe test coverage new/current, TreeHerder]: nope :-(
[Risks and why]: medium-low; is unlikely to make the keyboard-a11y situation worse, should go a long way to making it better
[String/UUID change made/needed]: nope

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8575997 [details] [diff] [review]
prevent focus from leaving the dialog for anything but the browser UI,

approving uplift in the hopes it does make keyboard a11y better.

Comment 15

[Camelia Badau [:cbadau], Desktop QA]

Verified fixed on Windows 7 64bit and 32bit using latest Nightly 39.0a1 (buildID: 20150315030236).

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/aed23e04c61d

Comment 17

[Camelia Badau [:cbadau], Desktop QA]

Verified fixed on Windows 7 64bit using latest Aurora 38.0a2 (buildID: 20150322004013).