New issue
Advanced search Search tips

Issue 831926 link

Starred by 1 user
Status: Fixed
Owner:
patricia...@chromium.org
Closed: May 2018
Cc:
pkasting@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 3
Type: Bug



Sign in to add a comment

Consolidate the two FocusRing static installation methods.

Project Member Reported by patricia...@chromium.org, Apr 12 2018 
crrev.com/r/1002716 introduces an additional views::FocusRing::Install() method that accepts a custom colour instead of using a ui::NativeTheme::ColorId value. This is required because the top-chrome is coloured with a dark theme in incognito mode, and thus requires a differently coloured focus ring, while the secondary UI remains on the default (non-dark) theme, and thus uses the original focus ring color (ui::NativeTheme::kColorId_FocusedBorderColor).

This introduces a bit more complexity because these two Install() methods overlap in different ways, so investigate a way to consolidate these methods into one, with the original views::FocusRing::Install(View, ui::NativeTheme::ColorId) signature preferred.
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 19 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a7a562592505582e21f66c6a802eff61b15ca9cb

commit a7a562592505582e21f66c6a802eff61b15ca9cb
Author: Patti <patricialor@chromium.org>
Date: Thu Apr 19 05:21:49 2018

FocusRing/Views: Use only one Install() signature accepting a SkColor.

Consolidate the two existing FocusRing::Install() functions into one for
simplicity and update callers. FocusRing users may still apply overrides to the
FocusRing color, but must observe for changes to the NativeTheme if that is the
source of their chosen color.

Bug:  831926 
Change-Id: Ica2a94ffef155f29f1f3d08cccec0c87055f4c83
Reviewed-on: https://chromium-review.googlesource.com/1013671
Commit-Queue: Patti <patricialor@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551948}
[modify] https://crrev.com/a7a562592505582e21f66c6a802eff61b15ca9cb/ui/views/controls/combobox/combobox.cc
[modify] https://crrev.com/a7a562592505582e21f66c6a802eff61b15ca9cb/ui/views/controls/focus_ring.cc
[modify] https://crrev.com/a7a562592505582e21f66c6a802eff61b15ca9cb/ui/views/controls/focus_ring.h
[modify] https://crrev.com/a7a562592505582e21f66c6a802eff61b15ca9cb/ui/views/controls/textfield/textfield.cc

Comment 2 by patricia...@chromium.org, May 7 2018

Cc: -patricia...@chromium.org
Owner: patricia...@chromium.org
Status: Fixed (was: Available)
I guess there's only one Install() signature now, so closing this as Fixed.
►

Sign in to add a comment