1601708 - about:preferences is outlined after closing dialog

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Tyson Smith [:tsmith]]

Attachment: example.png

Closing a dialog in about:preferences causes the content window to be outlined with the selection 'dotted line'.

STR:

• open about:preferences
• scroll to bottom and click Settings... button under Network Settings
• close the Settings dialog window

Expected behavior:
Dialog window should close and the preferences page should display normally

Expected behavior:
Dialog window closes and the preferences page is outlined (see attachment)

This is reproducible on Windows Nightly (73) and Beta.

Comment 1

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
Visual regression we haven't yet shipped.

Kirk, could you take a look? I expect this is to do with whether the root element visually shows focus and a focus() call somewhere. It's possible that switching to using the focus manager and passing the appropriate flags to the focus call would avoid the focus outline when necessary.

I also expect this affects both Linux and Windows, but it doesn't seem to affect Mac.

Comment 2

[Dão Gottwald [:dao]]

(In reply to :Gijs (he/him) from comment #1)

[Tracking Requested - why for this release]:
Visual regression we haven't yet shipped.

Are you sure this is merely visual? It seems like focus should go back to the Settings button, but failing that the outline seems like a feature rather than a bug, as a means to tell the user where focus is. If before bug 1588142 we messed up focus without showing it, that seems like a bigger problem, and I'm not sure that's a state we'd want to go back to.

Comment 3

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #2)

(In reply to :Gijs (he/him) from comment #1)

[Tracking Requested - why for this release]:
Visual regression we haven't yet shipped.

Are you sure this is merely visual?

Yes. On prior builds, we focus the <window> root element, but there's no visual indication.

My understanding is that that's expected when not moving focus with the keyboard - while clicking around a page, we don't show focus outlines on Windows/Linux unless/until the user starts tab-navigating in the same window, IIRC. Am I misremembering?

It seems like focus should go back to the Settings button,

Maybe, that seems like a separate issue.

but failing that the outline seems like a feature rather than a bug, as a means to tell the user where focus is. If before bug 1588142 we messed up focus without showing it, that seems like a bigger problem, and I'm not sure that's a state we'd want to go back to.

I mean, the focus outline is cut off by the middle pane, so it's not a good focus outline, either...

Comment 4

[Dão Gottwald [:dao]]

(In reply to :Gijs (he/him) from comment #3)

My understanding is that that's expected when not moving focus with the keyboard - while clicking around a page, we don't show focus outlines on Windows/Linux unless/until the user starts tab-navigating in the same window, IIRC. Am I misremembering?

When moving focus via script, I think the expectation is that we show the focus ring. Otherwise, how is the user supposed to know? This is an entirely different case than the user explicitly clicking on a focusable element.

It seems like focus should go back to the Settings button,

Maybe, that seems like a separate issue.

It seems like the core issue to me, and if we just visually hide this that seems like an accessibility regression...

but failing that the outline seems like a feature rather than a bug, as a means to tell the user where focus is. If before bug 1588142 we messed up focus without showing it, that seems like a bigger problem, and I'm not sure that's a state we'd want to go back to.

I mean, the focus outline is cut off by the middle pane, so it's not a good focus outline, either...

Sure, that's another bug on top of another bug, and by removing the outline we can kind of wallpaper over both, but it still seems wrong to me that focus moves to the root element in the first place.

Comment 5

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #4)

(In reply to :Gijs (he/him) from comment #3)

My understanding is that that's expected when not moving focus with the keyboard - while clicking around a page, we don't show focus outlines on Windows/Linux unless/until the user starts tab-navigating in the same window, IIRC. Am I misremembering?

When moving focus via script, I think the expectation is that we show the focus ring. Otherwise, how is the user supposed to know? This is an entirely different case than the user explicitly clicking on a focusable element.

I don't think showing focus on the root when moving focus there via script is helpful. I don't think we do it for the case where we focus the content area from tabbrowser after typing in the URL and hitting enter, right? If the content area as a whole has focus, nothing happens for keypresses other than tab navigation or things handled separately by the page via keypress handlers, so I don't think focus indication is expected/necessary. Otherwise we'd show a focus outline around the content area all the time, and I don't think that's desirable.

It seems like focus should go back to the Settings button,

Maybe, that seems like a separate issue.

It seems like the core issue to me, and if we just visually hide this that seems like an accessibility regression...

It was visually hidden before so I don't think "regression" is accurate? If we move focus to the button and not show any focus indication on that node, then yes I agree that would be problematic.

but failing that the outline seems like a feature rather than a bug, as a means to tell the user where focus is. If before bug 1588142 we messed up focus without showing it, that seems like a bigger problem, and I'm not sure that's a state we'd want to go back to.

I mean, the focus outline is cut off by the middle pane, so it's not a good focus outline, either...

Sure, that's another bug on top of another bug, and by removing the outline we can kind of wallpaper over both, but it still seems wrong to me that focus moves to the root element in the first place.

The code is at https://searchfox.org/mozilla-central/rev/690e903ef689a4eca335b96bd903580394864a1c/browser/components/preferences/in-content/subdialogs.js#697-702,706 from bug 1340987, it seems maybe to do with https://bugzilla.mozilla.org/show_bug.cgi?id=1340987#c76 .

I still think passing the BYKEY flag there is wrong.

Comment 6

[Dão Gottwald [:dao]]

(In reply to :Gijs (he/him) from comment #5)

When moving focus via script, I think the expectation is that we show the focus ring. Otherwise, how is the user supposed to know? This is an entirely different case than the user explicitly clicking on a focusable element.

I don't think showing focus on the root when moving focus there via script is helpful. I don't think we do it for the case where we focus the content area from tabbrowser after typing in the URL and hitting enter, right?

That doesn't move focus within the loaded document, it's the initial focus state. No surprise here from the user's perspective, hence no focus ring.

If the content area as a whole has focus, nothing happens for keypresses other than tab navigation or things handled separately by the page via keypress handlers, so I don't think focus indication is expected/necessary.

The key here is knowing that you're in that state in the first place. Normally when you close a dialog focus goes back to where it was before, so that's what I'd expect as a user.

Otherwise we'd show a focus outline around the content area all the time, and I don't think that's desirable.

No, that doesn't follow from what I'm saying.

It was visually hidden before so I don't think "regression" is accurate?

Not a shipped regression but a regression nonetheless. My point is that we're in an arguably less broken state than before bug 1588142, and we shouldn't go back to that worse state.

If we move focus to the button and not show any focus indication on that node, then yes I agree that would be problematic.

You're framing this in a weird way. Focus was on that button before, so restoring that shouldn't count as moving focus. The button having focus would be expected whereas resetting focus to the root element is entirely unexpected. That said, the focus ring might still be helpful on the button.

Comment 7

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #6)

If we move focus to the button and not show any focus indication on that node, then yes I agree that would be problematic.

You're framing this in a weird way.

I don't think it's productive or in any way helpful to call what I'm saying "weird".

Focus was on that button before, so restoring that shouldn't count as moving focus. The button having focus would be expected whereas resetting focus to the root element is entirely unexpected.

As an engineer, of course, focus was on the button. (Also as an engineer: unsure we currently keep track of what element triggered opening a dialog in a way we can use from subdialogs.js to re-focus that...)

However, as a user, I don't think that focus was on the button. I had no indication that that focus moved at all when clicking the button - except when it moved into the dialog (assuming that shows a focus indicator, anyway). We don't visually indicate focus changes to any of the UI elements (except input fields) in the prefs through mouse interactions, afaict, so why would I think focus moved at all? What's more, if I'm interacting with a mouse, why would it matter? All I know is now I'm seeing this ugly checkered line around the content area for no reason...

My primary concern is that we shouldn't ship the cut-off indicator. I think it'd be preferable not to ship the indicator at all. If moving focus to the button without implying it's happening using the keyboard makes everyone happy, let's do that.

Comment 8

[Dão Gottwald [:dao]]

(In reply to :Gijs (he/him) from comment #7)

Focus was on that button before, so restoring that shouldn't count as moving focus. The button having focus would be expected whereas resetting focus to the root element is entirely unexpected.

As an engineer, of course, focus was on the button. (Also as an engineer: unsure we currently keep track of what element triggered opening a dialog in a way we can use from subdialogs.js to re-focus that...)

I don't think we need to know what element triggered the dialog, not in that narrow sense anyway; we'd just need to keep track of document.activeElement when opening the dialog.

However, as a user, I don't think that focus was on the button. I had no indication that that focus moved at all when clicking the button - except when it moved into the dialog (assuming that shows a focus indicator, anyway). We don't visually indicate focus changes to any of the UI elements (except input fields) in the prefs through mouse interactions, afaict, so why would I think focus moved at all?

This is not true on Windows and Linux, on Mac it depends on full keyboard access being enabled from what I remember.

What's more, if I'm interacting with a mouse, why would it matter? All I know is now I'm seeing this ugly checkered line around the content area for no reason...

For a mouse-only user my accessibility concerns are obviously moot. As soon as one starts using the keyboard they become relevant. We need to cater for both types of users.

Comment 9

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #8)

However, as a user, I don't think that focus was on the button. I had no indication that that focus moved at all when clicking the button - except when it moved into the dialog (assuming that shows a focus indicator, anyway). We don't visually indicate focus changes to any of the UI elements (except input fields) in the prefs through mouse interactions, afaict, so why would I think focus moved at all?

This is not true on Windows and Linux, on Mac it depends on full keyboard access being enabled from what I remember.

Which bit isn't true? I tested it (on Windows) before writing that comment. I don't see any visual changes to the checkbox when ticking/unticking a checkbox and then moving the mouse away, for instance. Ditto opening and then closing a menulist, etc.

Comment 10

[Dão Gottwald [:dao]]

(In reply to :Gijs (he/him) from comment #9)

(In reply to Dão Gottwald [::dao] from comment #8)

However, as a user, I don't think that focus was on the button. I had no indication that that focus moved at all when clicking the button - except when it moved into the dialog (assuming that shows a focus indicator, anyway). We don't visually indicate focus changes to any of the UI elements (except input fields) in the prefs through mouse interactions, afaict, so why would I think focus moved at all?

This is not true on Windows and Linux, on Mac it depends on full keyboard access being enabled from what I remember.

Which bit isn't true? I tested it (on Windows) before writing that comment. I don't see any visual changes to the checkbox when ticking/unticking a checkbox and then moving the mouse away, for instance. Ditto opening and then closing a menulist, etc.

All of your examples visibly move focus from what I can tell. I tested on Windows 10 with Nightly in about:preferences using only the mouse. For checkboxes we have a focus ring around the label, not the checkbox itself.

The focus change is visible even for the Settings button because that gains focus on mousedown and we open the dialog on mouseup.

Comment 11

[:Gijs (he/him)]

Attachment: Options - Firefox Nightly 2019-12-11 15-23-58.mp4

Here's what I see. I guess you can initially see focus disappearing from the search box, but otherwise, I don't see any indication of focus?

Are you perhaps checking after opening a subdialog? That shows focus indicators because of the MOVE_BYKEY code that I highlighted earlier activating focus highlighting (even for subsequent mouse use).

Comment 12

[:Gijs (he/him)]

(huh, I guess the open file dialog and menulists don't appear in the builtin windows screen recorder tool, which makes it a bit more difficult to follow, but hopefully the gist is obvious.)

Comment 13

[Dão Gottwald [:dao]]

(In reply to :Gijs (he/him) from comment #11)

Created attachment 9115209 [details]
Options - Firefox Nightly 2019-12-11 15-23-58.mp4

Here's what I see. I guess you can initially see focus disappearing from the search box, but otherwise, I don't see any indication of focus?

Are you perhaps checking after opening a subdialog? That shows focus indicators because of the MOVE_BYKEY code that I highlighted earlier activating focus highlighting (even for subsequent mouse use).

No, opening the dialog is the last thing I tried.

I'm not sure why we see different things but it's kind of moot since we need to consider keyboard users anyway.

Comment 14

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [::dao] from comment #13)

Are you perhaps checking after opening a subdialog? That shows focus indicators because of the MOVE_BYKEY code that I highlighted earlier activating focus highlighting (even for subsequent mouse use).

No, opening the dialog is the last thing I tried.

I'm not sure why we see different things but it's kind of moot since we need to consider keyboard users anyway.

I haven't confirmed this but I'm guessing the difference may have been that I pressed Alt to get to about:preferences from the menu bar.

Comment 15

[Tim Nguyen :ntim]

Maybe Marco or Jamie have an input on which element should be focused in this case?

Comment 16

[James Teh [:Jamie]]

Generally, when closing a dialog, focus should return to the button whence it came. So, in the example from comment 0, it should return to the Settings button.

Comment 17

[Robin Steuber (they/them) [:bytesized]]

Attachment: Bug 1601708 - Return focus to the previously active element when a subdialog closes

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d2b9b4e977a7
Return focus to the previously active element when a subdialog closes r=Gijs,dao

Comment 19

[Razvan Maries]

Backed out for Lint failure.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=281575352&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&searchStr=linting%2Copt%2Cjavascript%2Cchecks%2Csource-test-mozlint-eslint%2Cjs%28es%29&revision=d2b9b4e977a779e3fa9fe3ccf97bac7ac07afbcd

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=281575352&repo=autoland&lineNumber=268

Backout: https://hg.mozilla.org/integration/autoland/rev/dcab270db63af6dfb9ab0e3740c06866c49cb993

Comment 20

[Robin Steuber (they/them) [:bytesized]]

Whoops. I'll fix it.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/63822634f6ff
Return focus to the previously active element when a subdialog closes r=Gijs,dao

Comment 22

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/63822634f6ff

Comment 23

[Julien Cristau [:jcristau] (back April 22)]

Should we uplift to avoid shipping this regression?

Comment 24

[Robin Steuber (they/them) [:bytesized]]

Comment on attachment 9115560 [details]
Bug 1601708 - Return focus to the previously active element when a subdialog closes

Beta/Release Uplift Approval Request

• User impact if declined: When returning from a dialog in about:preferences, focus will be returned to the document rather than the most recently active element, displaying a dotted line around the document.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See bug description
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Code change is pretty small and only affects what element is focused.
• String changes made/needed:

Comment 25

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 9115560 [details]
Bug 1601708 - Return focus to the previously active element when a subdialog closes

about:preferences regression fix, approved for 72.0b9

Comment 26

[Dorel Luca [:dluca]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6f8d7292b8d8

Comment 27

[Gyula Palko[:gpalko]]

Verified as fixed on Nightly 73.0a1(20191223214135) and Beta 72.0b10(20191221003008) on Win10 x64, Ubuntu 18.04 and MacOS 10.14