Eliminate GetReadableColor(); it's ineffective.

It uses the mechanic of lightness-inverting the source color and picking the
option that has better contrast.  But lightness-inverting may fail to improve
contrast, since contrast is based on luminance, which is not linearly related to
lightness.  For example, HSL 60,100%,36% (contrast ratio against white 2.12)
lightness-inverts to HSL 60,100%,64% (contrast ratio against white 1.06).

Instead, use functions like GetColorWithMinimumContrast() or
GetColorWithMaxContrast() to ensure that the colors in question have sufficient
contrast.  I tried to make the changes here relatively behavior-preserving, so
GetReadableColor(SK_ColorWHITE, ...) was converted to GetColorWithMaxContrast()
since the old code would have chosen between white and black, and other cases
were converted to GetColorWithMinimumContrast().

Bug: 659451
Change-Id: Id8508a9c8d65236f85b7fc5246b22ec5bb8afe0b
Reviewed-on: https://chromium-review.googlesource.com/c/1407690
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623037}

ash/app_list/views/app_list_item_view.cc

@@ -220,7 +220,6 @@ AppListItemView::AppListItemView(AppsGridView* apps_grid_view,
   }
 
   title_->SetBackgroundColor(SK_ColorTRANSPARENT);
-  title_->SetAutoColorReadabilityEnabled(false);
   title_->SetHandlesTooltips(false);
   title_->SetFontList(AppListConfig::instance().app_title_font());
   title_->SetLineHeight(AppListConfig::instance().app_title_max_line_height());

ash/shelf/shelf_tooltip_bubble.cc

@@ -48,8 +48,6 @@ ShelfTooltipBubble::ShelfTooltipBubble(views::View* anchor,
   SkColor background_color =
       theme->GetSystemColor(ui::NativeTheme::kColorId_TooltipBackground);
   label->SetBackgroundColor(background_color);
-  // The background is not opaque, so we can't do subpixel rendering.
-  label->SetSubpixelRenderingEnabled(false);
   AddChildView(label);
 
   gfx::Insets insets(kArrowTopBottomOffset, kArrowLeftRightOffset);

ash/system/power/power_button_menu_item_view.cc

@@ -59,7 +59,6 @@ PowerButtonMenuItemView::PowerButtonMenuItemView(
   AddChildView(icon_view_);
 
   title_->SetBackgroundColor(SK_ColorTRANSPARENT);
-  title_->SetAutoColorReadabilityEnabled(false);
   title_->SetEnabledColor(kItemTitleColor);
   title_->SetText(title_text);
   AddChildView(title_);

ash/touch/touch_observer_hud.cc

@@ -25,6 +25,7 @@
 #include "ui/events/event.h"
 #include "ui/gfx/animation/animation_delegate.h"
 #include "ui/gfx/canvas.h"
+#include "ui/gfx/color_utils.h"
 #include "ui/gfx/geometry/insets.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/size.h"
@@ -354,11 +355,15 @@ TouchObserverHUD::TouchObserverHUD(aura::Window* initial_root)
   label_container_->SetLayoutManager(
       std::make_unique<views::BoxLayout>(views::BoxLayout::kVertical));
 
+  constexpr SkColor kShadowColor = SK_ColorWHITE;
+  const SkColor label_color =
+      color_utils::GetColorWithMaxContrast(kShadowColor);
   for (int i = 0; i < kMaxTouchPoints; ++i) {
     touch_labels_[i] = new views::Label;
-    touch_labels_[i]->SetBackgroundColor(SkColorSetARGB(0, 255, 255, 255));
+    touch_labels_[i]->SetEnabledColor(label_color);
+    touch_labels_[i]->SetBackgroundColor(SK_ColorTRANSPARENT);
     touch_labels_[i]->SetShadows(gfx::ShadowValues(
-        1, gfx::ShadowValue(gfx::Vector2d(1, 1), 0, SK_ColorWHITE)));
+        1, gfx::ShadowValue(gfx::Vector2d(1, 1), 0, kShadowColor)));
     label_container_->AddChildView(touch_labels_[i]);
   }
   label_container_->SetX(0);

chrome/browser/themes/theme_service.cc

@@ -566,8 +566,8 @@ SkColor ThemeService::GetDefaultColor(int id, bool incognito) const {
     }
 #if BUILDFLAG(ENABLE_SUPERVISED_USERS)
     case ThemeProperties::COLOR_SUPERVISED_USER_LABEL:
-      return color_utils::GetReadableColor(
-          SK_ColorWHITE, GetColor(kLabelBackground, incognito));
+      return color_utils::GetColorWithMaxContrast(
+          GetColor(kLabelBackground, incognito));
     case ThemeProperties::COLOR_SUPERVISED_USER_LABEL_BACKGROUND:
       return color_utils::BlendTowardMaxContrast(
           GetColor(ThemeProperties::COLOR_FRAME, incognito), 0x80);

chrome/browser/ui/views/frame/opaque_browser_frame_view.cc

@@ -62,8 +62,6 @@ using content::WebContents;
 
 namespace {
 
-constexpr SkColor kTitleBarFeatureColor = SK_ColorWHITE;
-
 class CaptionButtonBackgroundImageSource : public gfx::CanvasImageSource {
  public:
   CaptionButtonBackgroundImageSource(const gfx::ImageSkia& bg_image,
@@ -715,7 +713,7 @@ SkColor OpaqueBrowserFrameView::GetFrameForegroundColor(
     if (has_site_theme && !platform_observer_->IsUsingSystemTheme())
       return color_utils::GetThemedAssetColor(frame_color);
   }
-  return color_utils::GetReadableColor(kTitleBarFeatureColor, frame_color);
+  return color_utils::GetColorWithMaxContrast(frame_color);
 }
 
 void OpaqueBrowserFrameView::PaintRestoredFrameBorder(

chrome/browser/ui/views/frame/opaque_browser_frame_view_browsertest.cc

@@ -100,16 +100,17 @@ IN_PROC_BROWSER_TEST_F(HostedAppOpaqueBrowserFrameViewTest, NoThemeColor) {
   if (!InstallAndLaunchHostedApp())
     return;
   EXPECT_EQ(hosted_app_button_container_->active_color_for_testing(),
-            SK_ColorBLACK);
+            gfx::kGoogleGrey900);
 }
 
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
 IN_PROC_BROWSER_TEST_F(HostedAppOpaqueBrowserFrameViewTest, SystemThemeColor) {
   SetThemeMode(ThemeMode::kSystem);
-  ASSERT_TRUE(InstallAndLaunchHostedApp(SK_ColorBLACK));
+  // The color here should be ignored in system mode.
+  ASSERT_TRUE(InstallAndLaunchHostedApp(SK_ColorRED));
 
   EXPECT_EQ(hosted_app_button_container_->active_color_for_testing(),
-            SK_ColorBLACK);
+            gfx::kGoogleGrey900);
 }
 #endif  // defined(OS_LINUX) && !defined(OS_CHROMEOS)
 

chrome/browser/ui/views/location_bar/custom_tab_bar_view.cc

@@ -111,8 +111,9 @@ class CustomTabBarTitleOriginView : public views::View {
 
     // We need to disable auto color readability, as we want to match the active
     // color in the title bar, which is subtly different.
-    // TODO(http://crbug.com/883177): Enable this if we use GetReadableColor(..)
-    // for the app title text instead of GetThemedAssetColor(..).
+    // TODO(http://crbug.com/883177): Enable this if we use
+    // GetColorWithMinimumContrast() for the app title text instead of
+    // GetThemedAssetColor().
     title_label_->SetAutoColorReadabilityEnabled(false);
     location_label_->SetAutoColorReadabilityEnabled(false);
 

chrome/browser/ui/views/status_bubble_views.cc

@@ -493,10 +493,10 @@ void StatusBubbleViews::StatusView::OnPaint(gfx::Canvas* canvas) {
       theme_provider_->GetColor(ThemeProperties::COLOR_TAB_TEXT), bubble_color,
       0.6f);
 
-  canvas->DrawStringRect(
-      text_, GetFont(),
-      color_utils::GetReadableColor(blended_text_color, bubble_color),
-      text_rect);
+  canvas->DrawStringRect(text_, GetFont(),
+                         color_utils::GetColorWithMinimumContrast(
+                             blended_text_color, bubble_color),
+                         text_rect);
 }
 
 

chrome/browser/ui/views/subtle_notification_view.cc

@@ -55,9 +55,7 @@ class SubtleNotificationView::InstructionView : public views::View {
   // more segments delimited by a pair of pipes ('|'); each of these segments
   // will be displayed as a keyboard key. e.g., "Press |Alt|+|Q| to exit" will
   // have "Alt" and "Q" rendered as keys.
-  InstructionView(const base::string16& text,
-                  SkColor foreground_color,
-                  SkColor background_color);
+  explicit InstructionView(const base::string16& text);
 
   const base::string16 text() const { return text_; }
   void SetText(const base::string16& text);
@@ -68,19 +66,13 @@ class SubtleNotificationView::InstructionView : public views::View {
   // keyboard key.
   void AddTextSegment(const base::string16& text, bool format_as_key);
 
-  SkColor foreground_color_;
-  SkColor background_color_;
-
   base::string16 text_;
 
   DISALLOW_COPY_AND_ASSIGN(InstructionView);
 };
 
 SubtleNotificationView::InstructionView::InstructionView(
-    const base::string16& text,
-    SkColor foreground_color,
-    SkColor background_color)
-    : foreground_color_(foreground_color), background_color_(background_color) {
+    const base::string16& text) {
   // The |between_child_spacing| is the horizontal margin of the key name.
   SetLayoutManager(std::make_unique<views::BoxLayout>(
       views::BoxLayout::kHorizontal, gfx::Insets(), kKeyNameMarginHorizPx));
@@ -119,9 +111,12 @@ void SubtleNotificationView::InstructionView::SetText(
 
 void SubtleNotificationView::InstructionView::AddTextSegment(
     const base::string16& text, bool format_as_key) {
+  constexpr SkColor kForegroundColor = SK_ColorWHITE;
+
   views::Label* label = new views::Label(text, kInstructionTextContext);
-  label->SetEnabledColor(foreground_color_);
-  label->SetBackgroundColor(background_color_);
+  label->SetEnabledColor(kForegroundColor);
+  label->SetBackgroundColor(kSubtleNotificationBackgroundColor);
+
   if (!format_as_key) {
     AddChildView(label);
     return;
@@ -136,22 +131,19 @@ void SubtleNotificationView::InstructionView::AddTextSegment(
   key->AddChildView(label);
   // The key name has a border around it.
   std::unique_ptr<views::Border> border(views::CreateRoundedRectBorder(
-      kKeyNameBorderPx, kKeyNameCornerRadius, foreground_color_));
+      kKeyNameBorderPx, kKeyNameCornerRadius, kForegroundColor));
   key->SetBorder(std::move(border));
   AddChildView(key);
 }
 
 SubtleNotificationView::SubtleNotificationView() : instruction_view_(nullptr) {
-  const SkColor kForegroundColor = SK_ColorWHITE;
-
   std::unique_ptr<views::BubbleBorder> bubble_border(new views::BubbleBorder(
       views::BubbleBorder::NONE, views::BubbleBorder::NO_ASSETS,
       kSubtleNotificationBackgroundColor));
   SetBackground(std::make_unique<views::BubbleBackground>(bubble_border.get()));
   SetBorder(std::move(bubble_border));
 
-  instruction_view_ = new InstructionView(base::string16(), kForegroundColor,
-                                          kSubtleNotificationBackgroundColor);
+  instruction_view_ = new InstructionView(base::string16());
 
   int outer_padding_horiz = kOuterPaddingHorizPx;
   int outer_padding_vert = kOuterPaddingVertPx;

ui/gfx/color_utils.cc

@@ -64,13 +64,6 @@ float Linearize(float eight_bit_component) {
                                  : pow((component + 0.055f) / 1.055f, 2.4f);
 }
 
-SkColor LightnessInvertColor(SkColor color) {
-  HSL hsl;
-  SkColorToHSL(color, &hsl);
-  hsl.l = 1.0f - hsl.l;
-  return HSLToSkColor(hsl, SkColorGetA(color));
-}
-
 }  // namespace
 
 float GetContrastRatio(SkColor color_a, SkColor color_b) {
@@ -332,11 +325,6 @@ SkColor GetThemedAssetColor(SkColor theme_color) {
   return AlphaBlend(SK_ColorBLACK, theme_color, kThemedForegroundBlackFraction);
 }
 
-SkColor GetReadableColor(SkColor foreground, SkColor background) {
-  return PickContrastingColor(foreground, LightnessInvertColor(foreground),
-                              background);
-}
-
 SkColor PickContrastingColor(SkColor foreground1,
                              SkColor foreground2,
                              SkColor background) {

ui/gfx/color_utils.h

@@ -125,16 +125,6 @@ GFX_EXPORT SkColor BlendTowardMaxContrast(SkColor color, SkAlpha alpha);
 // This is a copy of |getThemedAssetColor()| in ColorUtils.java.
 GFX_EXPORT SkColor GetThemedAssetColor(SkColor theme_color);
 
-// Given a foreground and background color, try to return a foreground color
-// that is "readable" over the background color by luma-inverting the foreground
-// color and then using PickContrastingColor() to pick the one with greater
-// contrast.  During this process, alpha values will be ignored; the returned
-// color will have the same alpha as |foreground|.
-//
-// NOTE: This won't do anything but waste time if the supplied foreground color
-// has a luma value close to the midpoint (0.5 in the HSL representation).
-GFX_EXPORT SkColor GetReadableColor(SkColor foreground, SkColor background);
-
 // Returns whichever of |foreground1| or |foreground2| has higher contrast with
 // |background|.
 GFX_EXPORT SkColor PickContrastingColor(SkColor foreground1,

ui/views/controls/label.cc

@@ -33,6 +33,14 @@
 #include "ui/views/native_cursor.h"
 #include "ui/views/selection_controller.h"
 
+namespace {
+
+bool IsOpaque(SkColor color) {
+  return SkColorGetA(color) == SK_AlphaOPAQUE;
+}
+
+}  // namespace
+
 namespace views {
 
 const char Label::kViewClassName[] = "Label";
@@ -465,8 +473,7 @@ void Label::PaintText(gfx::Canvas* canvas) {
     return;
 
   for (View* view = this; view; view = view->parent()) {
-    if (view->background() &&
-        SkColorGetA(view->background()->get_color()) == SK_AlphaOPAQUE)
+    if (view->background() && IsOpaque(view->background()->get_color()))
       break;
 
     if (view->layer() && view->layer()->fills_bounds_opaquely()) {
@@ -860,16 +867,23 @@ gfx::Size Label::GetTextSize() const {
   return size;
 }
 
+SkColor Label::GetForegroundColor(SkColor foreground,
+                                  SkColor background) const {
+  return (auto_color_readability_ && IsOpaque(background))
+             ? color_utils::GetColorWithMinimumContrast(foreground, background)
+             : foreground;
+}
+
 void Label::RecalculateColors() {
-  actual_enabled_color_ = auto_color_readability_ ?
-      color_utils::GetReadableColor(requested_enabled_color_,
-                                    background_color_) :
-      requested_enabled_color_;
+  actual_enabled_color_ =
+      GetForegroundColor(requested_enabled_color_, background_color_);
+  // Using GetResultingPaintColor() here allows non-opaque selection backgrounds
+  // to still participate in auto color readability, assuming
+  // |background_color_| is itself opaque.
   actual_selection_text_color_ =
-      auto_color_readability_
-          ? color_utils::GetReadableColor(requested_selection_text_color_,
-                                          selection_background_color_)
-          : requested_selection_text_color_;
+      GetForegroundColor(requested_selection_text_color_,
+                         color_utils::GetResultingPaintColor(
+                             selection_background_color_, background_color_));
 
   ApplyTextColors();
   SchedulePaint();
@@ -879,15 +893,13 @@ void Label::ApplyTextColors() const {
   if (!display_text_)
     return;
 
-  bool subpixel_rendering_suppressed =
-      SkColorGetA(background_color_) != SK_AlphaOPAQUE ||
-      !subpixel_rendering_enabled_;
   display_text_->SetColor(actual_enabled_color_);
   display_text_->set_selection_color(actual_selection_text_color_);
   display_text_->set_selection_background_focused_color(
       selection_background_color_);
-  display_text_->set_subpixel_rendering_suppressed(
-      subpixel_rendering_suppressed);
+  const bool subpixel_rendering_enabled =
+      subpixel_rendering_enabled_ && IsOpaque(background_color_);
+  display_text_->set_subpixel_rendering_suppressed(!subpixel_rendering_enabled);
 }
 
 void Label::UpdateColorsFromTheme(const ui::NativeTheme* theme) {

ui/views/controls/label.h

@@ -81,8 +81,9 @@ class VIEWS_EXPORT Label : public View,
 
   // Enables or disables auto-color-readability (enabled by default).  If this
   // is enabled, then calls to set any foreground or background color will
-  // trigger an automatic mapper that uses color_utils::GetReadableColor() to
-  // ensure that the foreground colors are readable over the background color.
+  // trigger an automatic mapper that uses
+  // color_utils::GetColorWithMinimumContrast() to ensure that the foreground
+  // colors are readable over the background color.
   void SetAutoColorReadabilityEnabled(bool enabled);
 
   // Sets the color.  This will automatically force the color to be readable
@@ -311,6 +312,10 @@ class VIEWS_EXPORT Label : public View,
   // Get the text size for the current layout.
   gfx::Size GetTextSize() const;
 
+  // Returns the appropriate foreground color to use given the proposed
+  // |foreground| and |background| colors.
+  SkColor GetForegroundColor(SkColor foreground, SkColor background) const;
+
   // Updates text and selection colors from requested colors.
   void RecalculateColors();
 

ui/views/controls/styled_label_unittest.cc

@@ -378,19 +378,19 @@ TEST_F(StyledLabelTest, StyledRangeTextStyleBold) {
 }
 
 TEST_F(StyledLabelTest, Color) {
-  const std::string text_red("RED");
+  const std::string text_blue("BLUE");
   const std::string text_link("link");
   const std::string text("word");
-  InitStyledLabel(text_red + text_link + text);
+  InitStyledLabel(text_blue + text_link + text);
 
-  StyledLabel::RangeStyleInfo style_info_red;
-  style_info_red.override_color = SK_ColorRED;
-  styled()->AddStyleRange(gfx::Range(0u, text_red.size()), style_info_red);
+  StyledLabel::RangeStyleInfo style_info_blue;
+  style_info_blue.override_color = SK_ColorBLUE;
+  styled()->AddStyleRange(gfx::Range(0u, text_blue.size()), style_info_blue);
 
   StyledLabel::RangeStyleInfo style_info_link =
       StyledLabel::RangeStyleInfo::CreateForLink();
   styled()->AddStyleRange(
-      gfx::Range(text_red.size(), text_red.size() + text_link.size()),
+      gfx::Range(text_blue.size(), text_blue.size() + text_link.size()),
       style_info_link);
 
   styled()->SetBounds(0, 0, 1000, 1000);
@@ -413,7 +413,7 @@ TEST_F(StyledLabelTest, Color) {
   container->AddChildView(link);
   const SkColor kDefaultLinkColor = link->enabled_color();
 
-  EXPECT_EQ(SK_ColorRED,
+  EXPECT_EQ(SK_ColorBLUE,
             static_cast<Label*>(styled()->child_at(0))->enabled_color());
   EXPECT_EQ(kDefaultLinkColor,
             static_cast<Label*>(styled()->child_at(1))->enabled_color());