Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Specify color for focus ring #476

Merged
merged 1 commit into from
Sep 19, 2018
Merged

Specify color for focus ring #476

merged 1 commit into from
Sep 19, 2018

Conversation

petemill
Copy link
Member

Fix brave/brave-browser#1190

In Chromium, this is specified in Native Theme, and overriden on macOS from the OS-level focus border color, but only for light theme (i.e. not incognito). In lieu of overriding all the Native Themes, this provides the 1 specific color to the FocusRing view. Whilst we may very well subclass all the Native Themes at some point, this is a pain-free way to get there for this feature of the design spec now.

image

image

Tested on Windows and macOS

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Needed or QA/No-QA-Needed) to include the closed issue in milestone

Test Plan:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

@petemill petemill added the UI label Sep 19, 2018
@petemill petemill self-assigned this Sep 19, 2018
simonhong
simonhong reviewed Sep 19, 2018
View reviewed changes
chromium_src/ui/views/controls/focus_ring.cc Outdated
class FocusRingTheme {
public:
SkColor GetSystemColor(int id) {
DCHECK(id == ui::NativeTheme::kColorId_FocusedBorderColor);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we guarantee id is always focused border color?
It seems that ColorIdForValidity can return two kinds of id.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch @simonhong. I'll modify to support the other Id.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This other value is supported now. Though I could not get this state with our featureset. The places that I see focusring being used didn't have an error state:

  • LocationBar
  • Bookmark name popup
  • Create Shortcut popup
  • edit bookmark popup

@simonhong
Copy link
Member

I searched kColorId_FocusedBorderColor and found that many places uses it as a border color for same purpose.
Don't we need to check these places?

@petemill petemill mentioned this pull request Sep 19, 2018
Merged
11 tasks
@petemill
Specify color for focus ring
725def1 
Fix brave/brave-browser#1190

In Chromium, this is specified in Native Theme, and overriden on macOS from the OS-level focus border color, but only for light theme (i.e. not incognito). In lieu of overriding all the Native Themes, this provides the 1 specific color to the FocusRing view. Whilst we may very well subclass all the Native Themes at some point, this is a pain-free way to get there for this feature of the design spec now.
@petemill
Copy link
Member Author

@simonhong I searched too, and those items seemed minor, or not within the toolbar. The main purpose of this was to get the focus color for the LocationBar, but it's desirable that we have it for the rest of the Toolbar too, which this achieves. If some popups use a different (system-specific) color (when they use a different theme anyway), then that is ok. Do you agree?

@simonhong
Copy link
Member

@petemill Ok, I agree!

simonhong
simonhong approved these changes Sep 19, 2018
View reviewed changes
Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@petemill petemill merged commit d4cf221 into master Sep 19, 2018
@petemill petemill deleted the ui/focus-ring-color branch September 19, 2018 05:00
petemill added a commit that referenced this pull request Sep 19, 2018
@petemill
Merge pull request #476 from brave/ui/focus-ring-color
1a69af6 
Specify color for focus ring
@petemill
Copy link
Member Author

0.55.x 1a69af6

@bbondy bbondy added this to the 0.55.x - Release milestone Jan 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonhong simonhong simonhong approved these changes

@bsclifton bsclifton Awaiting requested review from bsclifton

Assignees

@petemill petemill

Labels
UI
Projects
None yet
Milestone
0.55.x - Release
Development

Successfully merging this pull request may close these issues.

Browser UI focus ring should be Brave color cross-platform
3 participants
@petemill @simonhong @bbondy