1506510 - Focus styling for toolbar buttons

Status: RESOLVED FIXED

(Firefox :: Theme, enhancement, P1)

Description

[James Teh [:Jamie]]

Spun off bug 1436086 comment 12.

Bug 1436086 will make it possible to focus all toolbar buttons with the keyboard. At present, these buttons aren't styled correctly for focus.

Yura posted a quick fix for testing here: https://pastebin.mozilla.org/9084896
Note that this only currently covers buttons on the toolbar itself; i.e. it doesn't cover the buttons inside the urlbar.

Comment 1

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Bug 1506510 - add keyboard focus styling for toolbar/urlbar buttons. r=dao

MozReview-Commit-ID: LTjyHF64Q35

Comment 2

[Dão Gottwald [:dao]]

Is the plan to enable this by default for all users or will users have to opt in?

To the extent that this is an accessibility feature, I'd like to better cater for users with subpar vision, where I think the proposed styling is likely not good enough. I'd therefore prefer a traditional dotted focus ring.

Comment 3

[Marco Zehe (:MarcoZ)]

Answering for Yura: The plan is to definitely turn this on for all users, and later, possibly even remove the pref so this cannot be disabled any more and is just the default behavior.

Comment 4

[Dão Gottwald [:dao]]

That doesn't actually answer the main part of my comment.

Comment 5

[Yura Zenevich [:yzen] (please use "needinfo")]

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

Is the plan to enable this by default for all users or will users have to opt in?

To the extent that this is an accessibility feature, I'd like to better cater for users with subpar vision, where I think the proposed styling is likely not good enough. I'd therefore prefer a traditional dotted focus ring.

I guess there are several things we can do:

1. Remove styling proposed by this patch (e.g. consistent with hover styling) and reinstate the dotted outline styling.
2. Add dotted outline styling in addition to the styling proposed by this patch.
3. Enhance proposed styling to have better contrast and either have or not have the dotted outline.

Personally I would prefer 3 without the dotted outline.
Thoughts?

Comment 6

[Dão Gottwald [:dao]]

(In reply to Yura Zenevich [:yzen] from comment #5)

I guess there are several things we can do:

1. Remove styling proposed by this patch (e.g. consistent with hover styling) and reinstate the dotted outline styling.
2. Add dotted outline styling in addition to the styling proposed by this patch.
3. Enhance proposed styling to have better contrast and either have or not have the dotted outline.

Personally I would prefer 3 without the dotted outline.

I imagine it might be quite difficult to meet a11y contrast requirements with option 3, but I guess you could give it a try. Option 2 would work but might look weird and in the end worse than option 1...

Comment 7

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Version 1 (original)

Comment 8

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Version 2 (original + box-shadow)

Comment 9

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Version 3 (box shadow only - low contrast same as active toolbar bg colour)

Comment 10

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Version 4 (box shadow only - same as active bg colour when brighttext present)

Comment 11

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Version 5 (box shadow only - active toolbar bg colour with opacity = 1)

Comment 12

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Version 6 (box shadow only - corresponds to platform focus ring colour)

Comment 13

[Yura Zenevich [:yzen] (please use "needinfo")]

Eric, Dão could you comment on if you like any of the focus styles or perhaps something else?

Note I am also adding a transition for the box shadow to make the focus and focus change more noticeable (for a11y purposes).

Comment 14

[Dão Gottwald [:dao]]

(In reply to Yura Zenevich [:yzen] from comment #9)

Created attachment 9045645 [details]
Version 3 (box shadow only - low contrast same as active toolbar bg colour)

I'm not sure what "active toolbar bg colour" means. Can you elaborate?

Created attachment 9045649 [details]
Version 6 (box shadow only - corresponds to platform focus ring colour)

There's no such thing on Windows and Linux, I don't think. We'd have to use some custom color.

By default, focus rings are dotted and use the current text color on Windows and Linux. Incidentally, this guarantees sufficient contrast.

Comment 15

[Dão Gottwald [:dao]]

Attachment: Edge's toolbar button focus ring

FYI, here's how the toolbar button focus ring looks in Edge. It's way more prominent than their hover styling. Apparently they opted for maximum visibility, most likely for accessibility reasons.

Comment 16

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Version if currentColor styling (mac)

This version uses currentColor for the colour of the focus styling. I like that it would be consistent with the original dotted outline. Contrast seems to be good too.

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 9046568 [details]
Version if currentColor styling (mac)

Looks good to me. If wanted we could also use the native blue on Mac and currentColor on the other platforms.

Comment 18

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Bug 1506510 - allow for customization of the keyboard focus styling for toolbar/urlbar buttons. r=dao

Depends on D19958

Comment 19

[Eric Pang [:epang] UX]

From a visual perspective, this works for me, thanks Yura!

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fffb4b5030da
add keyboard focus styling for toolbar/urlbar buttons. r=dao

Comment 21

[Cristian Brindusan [:cbrindusan]]

Backed out changeset fffb4b5030da (Bug 1506510) for bc failures at browser_popupNotification_keyboard.js.

Backout: https://hg.mozilla.org/integration/autoland/rev/6bb6c4d3a048831ca9ea1d2c51ce1007ea6b070b

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=fffb4b5030daf853ff9d61453f3132c3295a6a3b&selectedJob=233640624

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

There are also bc failures on browser_bug462289.js
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=233639966&repo=autoland&lineNumber=1457

Comment 22

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Bug 1506510 - add/fix keyboard focus styling for notification and blocked permission icon buttons. r=dao

Depends on D19958

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/19ec728351ac
add keyboard focus styling for toolbar/urlbar buttons. r=dao
https://hg.mozilla.org/integration/autoland/rev/3204779395ce
add/fix keyboard focus styling for notification and blocked permission icon buttons. r=dao

Comment 24

[Dão Gottwald [:dao]]

(In reply to Pulsebot from comment #23)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/19ec728351ac
add keyboard focus styling for toolbar/urlbar buttons. r=dao
https://hg.mozilla.org/integration/autoland/rev/3204779395ce
add/fix keyboard focus styling for notification and blocked permission icon
buttons. r=dao

From what I can tell browser_popupNotification_keyboard.js is still going to fail here, as the first patch flips browser.toolbars.keyboard_navigation and the second patch doesn't address the implied behavior change.

Comment 25

[Dão Gottwald [:dao]]

Oh, looking again, I see you made quite a few test changes. Would have liked to review these too.

Comment 26

[Yura Zenevich [:yzen] (please use "needinfo")]

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

Oh, looking again, I see you made quite a few test changes. Would have liked to review these too.

Sorry, yeah I made sure that the tests pass and account for the tab stops/arrow key navigation where necessary. I can request to back these out, if preferred ?

Comment 27

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/19ec728351ac
https://hg.mozilla.org/mozilla-central/rev/3204779395ce