1432800 - On about:sessionrestore, the "Restore Session" button doesn't have a focus ring by default

Status: VERIFIED FIXED

(Toolkit :: Themes, defect, P3)

Description

[Blake Winton (:bwinton) (:☕️)]

For some reason my computer is rebooting every night, and when I relaunch Firefox, it shows me about:sessionrestore instead of just automatically restoring my session, so I see the session restore page a lot, and it would be nice if I could just hit <enter> to restore my previous session, instead of having to move the mouse and click on it or hit <tab> twice before hitting <enter>.

Comment 1

[Mike de Boer [:mikedeboer]]

Apology for the late reply! I'd take a patch for this tweak anytime!

Comment 2

[Harry Twyford]

I was testing this on Mac and it seems as if the session restore button is focused by default as-is. What tripped me up is that <enter> doesn't select the button, only spacebar. I filed bug 1462127.

Comment 3

[Harry Twyford]

Attachment: Bug 1432800 - Mirror -focusring and |*button.primary:focus states.

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

Comment 4

[Harry Twyford]

I tested across a few Windows+Mac machines and the problem appears to be one of clarity -- the Restore button is focused when the page is loaded, but <xul|button>s had no styling to indicate they were focused. This was a particular problem on Windows, when there wasn't even an outline around the button so it looked as if nothing was focused at all (but the button was focused -- pressing spacebar on page load would click it). I've submitted changes that mirrors various XUL element's :hover states with :focus states for clarity.

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8976700 [details]
Bug 1432800 - Mirror -focusring and |*button.primary:focus states.

https://reviewboard.mozilla.org/r/244790/#review252482

Buttons already have focus indicators:

Linux: https://searchfox.org/mozilla-central/rev/dc6d85680539cb7e5fe2843d38a30a0581bfefe1/toolkit/themes/linux/global/in-content/common.css#15
Windows: https://searchfox.org/mozilla-central/rev/dc6d85680539cb7e5fe2843d38a30a0581bfefe1/toolkit/themes/windows/global/button.css#35

But we're using :-moz-focusring, which is different from :focus in that, on Windows, it will only be visible after changing focus with the keyboard.

To address this, I think we should add a rule that sets the outline specifically for *|button.primary:focus.

Comment 6

[Harry Twyford]

I'll add the rule for *|button.primary:focus. The current patch added focus states to a number of other XUL elements as well. Do you recommend I update them all with a :focus state to match their :-moz-focusring state in their toolkit/themes/*/global/*.css files? Or should I just leave them be? For instance checkbox:-moz-focusring.

If -moz-focusring is only visible after changing focus with the keyboard, would this bug replicate on any default-focused *:-moz-focusring element that doesn't have a :focus state styling?

Comment 7

[Dão Gottwald [:dao]]

(In reply to Harry Twyford [:harry] from comment #6)
> I'll add the rule for *|button.primary:focus. The current patch added focus
> states to a number of other XUL elements as well. Do you recommend I update
> them all with a :focus state to match their :-moz-focusring state in their
> toolkit/themes/*/global/*.css files? Or should I just leave them be? For
> instance checkbox:-moz-focusring.

They should stay as they are. If we used :focus there, just clicking these elements with the mouse would show the focus ring. Mouse users don't really need that, and :-moz-focusring was introduced specifically to avoid the visual noise in that case.

> If -moz-focusring is only visible after changing focus with the keyboard,
> would this bug replicate on any default-focused *:-moz-focusring element
> that doesn't have a :focus state styling?

Yes, it would.

Comment 8

[Harry Twyford]

Comment on attachment 8976700 [details]
Bug 1432800 - Mirror -focusring and |*button.primary:focus states.

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

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 8976700 [details]
Bug 1432800 - Mirror -focusring and |*button.primary:focus states.

https://reviewboard.mozilla.org/r/244790/#review260398

::: toolkit/themes/windows/global/button.css:35
(Diff revision 2)
>    text-align: center;
>  }
>  
>  /* .......... focused state .......... */
>  
> +*|button.primary:focus,

Let's not put this in button.css but common.css / common.inc.css.

Comment 10

[Harry Twyford]

Comment on attachment 8976700 [details]
Bug 1432800 - Mirror -focusring and |*button.primary:focus states.

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

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 8976700 [details]
Bug 1432800 - Mirror -focusring and |*button.primary:focus states.

https://reviewboard.mozilla.org/r/244790/#review261570

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/270585226424
Mirror -focusring and |*button.primary:focus states. r=dao

Comment 13

[Raul Gurzau (:RaulG)]

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

Comment 14

[Cristina Coroiu [:ccoroiu]]

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

Comment 15

[Maria Berlinger [:mberlinger], Services QA]

I have managed to reproduce this issue on an affected Firefox 60.0a1 (20180124220129) build using Windows 10 x64. 

This issue is verified fixed using Firefox 63.0b6 (20180913141435) and 64.0a1 (2018-09-13) on the following OSes: Windows 10 x64, Ubuntu 18.04.1 x64.