Focus styling for toolbar buttons

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P1
normal
RESOLVED FIXED
6 months ago
a month ago

People

(Reporter: Jamie, Assigned: yzen)

Tracking

unspecified
Firefox 67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(4 attachments, 7 obsolete attachments)

47 bytes, text/x-phabricator-request
Details | Review
2.24 KB, image/png
Details
9.41 MB, video/quicktime
Details
47 bytes, text/x-phabricator-request
Details | Review
Reporter

Description

6 months ago
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.
Component: Toolbars and Customization → Theme
Assignee

Comment 1

3 months ago

MozReview-Commit-ID: LTjyHF64Q35

Assignee

Updated

3 months ago
Status: NEW → ASSIGNED

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.

Flags: needinfo?(yzenevich)

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.

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

Flags: needinfo?(yzenevich)
Assignee

Comment 5

3 months ago

(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?

Flags: needinfo?(yzenevich) → needinfo?(dao+bmo)

(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...

Flags: needinfo?(dao+bmo)
Assignee

Comment 7

3 months ago
Posted image Version 1 (original) (obsolete) —
Assignee

Comment 8

3 months ago
Posted image Version 2 (original + box-shadow) (obsolete) —
Assignee

Comment 13

3 months ago

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).

Flags: needinfo?(epang)
Flags: needinfo?(dao+bmo)

(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.

Flags: needinfo?(dao+bmo) → needinfo?(yzenevich)

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.

Assignee

Updated

3 months ago
Attachment #9045643 - Attachment is obsolete: true
Flags: needinfo?(yzenevich)
Assignee

Updated

3 months ago
Attachment #9045644 - Attachment is obsolete: true
Assignee

Updated

3 months ago
Attachment #9045645 - Attachment is obsolete: true
Assignee

Updated

3 months ago
Attachment #9045646 - Attachment is obsolete: true
Assignee

Updated

3 months ago
Attachment #9045648 - Attachment is obsolete: true
Assignee

Updated

3 months ago
Attachment #9045649 - Attachment is obsolete: true
Assignee

Comment 16

3 months ago

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.

Attachment #9046568 - Flags: feedback?(dao+bmo)

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.

Attachment #9046568 - Flags: feedback?(dao+bmo) → feedback+

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

Flags: needinfo?(epang)
Attachment #9047499 - Attachment is obsolete: true

Comment 20

2 months ago
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fffb4b5030da
add keyboard focus styling for toolbar/urlbar buttons. r=dao

Comment 23

2 months ago
Pushed by yura.zenevich@gmail.com:
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

(In reply to Pulsebot from comment #23)

Pushed by yura.zenevich@gmail.com:
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.

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

Assignee

Comment 26

2 months ago

(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 ?

Flags: needinfo?(yzenevich) → needinfo?(dao+bmo)

Comment 27

2 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
Assignee

Updated

a month ago
Flags: needinfo?(dao+bmo)
You need to log in before you can comment on or make changes to this bug.