Use the used color scheme to set scrollbar overlay colors

Overlay scrollbars support both a light and a dark theme. This was set based on the background color of the scrolling element, but only if one was set. This patch also sets the scrollbar color theme based on the element's used color scheme. This improves the scrollbar appearance when the system is in dark mode and no explicit background color style has been set on the scroller. Bug: 1285450 Change-Id: I76191b4db66546901fb20a5ebede109c8176b1eb Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3458007 Reviewed-by: Steve Kobes <skobes@chromium.org> Commit-Queue: Philip Rogers <pdr@chromium.org> Cr-Commit-Position: refs/heads/main@{#970816} NOKEYCHECK=True GitOrigin-RevId: ffd45abbadfcd00158086f2413b99b1a209549d6
mjfroman · Feb 14, 2022 · eef28e4 · eef28e4
1 parent 6069989
commit eef28e4
Show file tree

Hide file tree

Showing 11 changed files with 168 additions and 46 deletions.
diff --git a/blink/renderer/core/frame/visual_viewport.cc b/blink/renderer/core/frame/visual_viewport.cc
@@ -1163,4 +1163,9 @@ void VisualViewport::Paint(GraphicsContext& context) const {
   }
 }
 
+void VisualViewport::UsedColorSchemeChanged() {
+  // The scrollbar overlay color theme depends on the used color scheme.
+  RecalculateScrollbarOverlayColorTheme();
+}
+
 }  // namespace blink
diff --git a/blink/renderer/core/frame/visual_viewport.h b/blink/renderer/core/frame/visual_viewport.h
@@ -279,6 +279,8 @@ class CORE_EXPORT VisualViewport : public GarbageCollected<VisualViewport>,
 
   void Paint(GraphicsContext&) const;
 
+  void UsedColorSchemeChanged();
+
  private:
   bool DidSetScaleOrLocation(float scale,
                              bool is_pinch_gesture_active,

diff --git a/blink/renderer/core/frame/visual_viewport_test.cc b/blink/renderer/core/frame/visual_viewport_test.cc
@@ -2800,5 +2800,32 @@ TEST_P(VisualViewportTest, ScrollbarGeometryOnSizeChange) {
   EXPECT_EQ(gfx::Size(7, 113), vertical_scrollbar->bounds());
 }
 
+TEST_F(VisualViewportSimTest, PreferredOverlayScrollbarColorTheme) {
+  ColorSchemeHelper color_scheme_helper(*(WebView().GetPage()));
+  color_scheme_helper.SetPreferredColorScheme(
+      mojom::blink::PreferredColorScheme::kDark);
+  SimRequest request("https://example.com/test.html", "text/html");
+  LoadURL("https://example.com/test.html");
+  request.Complete(R"HTML(
+          <!DOCTYPE html>
+          <meta name="color-scheme" content="light dark">
+          <style>
+            html { height: 2000px; }
+          </style>
+      )HTML");
+  Compositor().BeginFrame();
+
+  const VisualViewport& visual_viewport =
+      WebView().GetPage()->GetVisualViewport();
+  EXPECT_EQ(ScrollbarOverlayColorTheme::kScrollbarOverlayColorThemeLight,
+            visual_viewport.GetScrollbarOverlayColorTheme());
+
+  color_scheme_helper.SetPreferredColorScheme(
+      mojom::blink::PreferredColorScheme::kLight);
+  Compositor().BeginFrame();
+  EXPECT_EQ(ScrollbarOverlayColorTheme::kScrollbarOverlayColorThemeDark,
+            visual_viewport.GetScrollbarOverlayColorTheme());
+}
+
 }  // namespace
 }  // namespace blink
diff --git a/blink/renderer/core/layout/layout_view.cc b/blink/renderer/core/layout/layout_view.cc
@@ -35,6 +35,7 @@
 #include "third_party/blink/renderer/core/frame/local_frame.h"
 #include "third_party/blink/renderer/core/frame/local_frame_view.h"
 #include "third_party/blink/renderer/core/frame/settings.h"
+#include "third_party/blink/renderer/core/frame/visual_viewport.h"
 #include "third_party/blink/renderer/core/html/html_iframe_element.h"
 #include "third_party/blink/renderer/core/html/plugin_document.h"
 #include "third_party/blink/renderer/core/input/event_handler.h"
@@ -890,6 +891,23 @@ void LayoutView::UpdateFromStyle() {
     SetHasBoxDecorationBackground(true);
 }
 
+void LayoutView::StyleDidChange(StyleDifference diff,
+                                const ComputedStyle* old_style) {
+  NOT_DESTROYED();
+  LayoutBlockFlow::StyleDidChange(diff, old_style);
+
+  LocalFrame& frame = GetFrameView()->GetFrame();
+  if (frame.IsMainFrame()) {
+    // |VisualViewport::UsedColorScheme| depends on the LayoutView's used
+    // color scheme.
+    if (!old_style ||
+        old_style->UsedColorScheme() !=
+            frame.GetPage()->GetVisualViewport().UsedColorScheme()) {
+      frame.GetPage()->GetVisualViewport().UsedColorSchemeChanged();
+    }
+  }
+}
+
 RecalcLayoutOverflowResult LayoutView::RecalcLayoutOverflow() {
   NOT_DESTROYED();
   if (!NeedsLayoutOverflowRecalc())

diff --git a/blink/renderer/core/layout/layout_view.h b/blink/renderer/core/layout/layout_view.h
@@ -362,6 +362,9 @@ class CORE_EXPORT LayoutView : public LayoutBlockFlow {
 
   TrackedDescendantsMap& SvgTextDescendantsMap();
 
+ protected:
+  void StyleDidChange(StyleDifference, const ComputedStyle* old_style) override;
+
  private:
   bool CanHaveChildren() const override;
 

diff --git a/blink/renderer/core/page/page.cc b/blink/renderer/core/page/page.cc
@@ -366,7 +366,7 @@ SpatialNavigationController& Page::GetSpatialNavigationController() {
 }
 
 void Page::PlatformColorsChanged() {
-  for (const Page* page : AllPages())
+  for (const Page* page : AllPages()) {
     for (Frame* frame = page->MainFrame(); frame;
          frame = frame->Tree().TraverseNext()) {
       if (auto* local_frame = DynamicTo<LocalFrame>(frame)) {
@@ -375,6 +375,7 @@ void Page::PlatformColorsChanged() {
           view->InvalidatePaintForViewAndDescendants();
       }
     }
+  }
 }
 
 void Page::ColorSchemeChanged() {

diff --git a/blink/renderer/core/paint/paint_layer_scrollable_area.cc b/blink/renderer/core/paint/paint_layer_scrollable_area.cc
@@ -1223,21 +1223,9 @@ void PaintLayerScrollableArea::UpdateAfterStyleChange(
 
   UpdateResizerStyle(old_style);
 
-  // Whenever background changes on the scrollable element, the scroll bar
-  // overlay style might need to be changed to have contrast against the
-  // background.
-  // Skip the need scrollbar check, because we dont know do we need a scrollbar
-  // when this method get called.
-  Color old_background;
-  if (old_style) {
-    old_background =
-        old_style->VisitedDependentColor(GetCSSPropertyBackgroundColor());
-  }
-  Color new_background = GetLayoutBox()->StyleRef().VisitedDependentColor(
-      GetCSSPropertyBackgroundColor());
-
-  if (new_background != old_background)
-    RecalculateScrollbarOverlayColorTheme(new_background);
+  // The scrollbar overlay color theme depends on styles such as the background
+  // color and the used color scheme.
+  RecalculateScrollbarOverlayColorTheme();
 
   if (NeedsScrollbarReconstruction()) {
     RemoveScrollbarsForReconstruction();

diff --git a/blink/renderer/core/paint/paint_layer_scrollable_area_test.cc b/blink/renderer/core/paint/paint_layer_scrollable_area_test.cc
@@ -18,6 +18,7 @@
 #include "third_party/blink/renderer/core/paint/paint_layer.h"
 #include "third_party/blink/renderer/core/scroll/scroll_types.h"
 #include "third_party/blink/renderer/core/scroll/scrollbar_theme.h"
+#include "third_party/blink/renderer/core/testing/color_scheme_helper.h"
 #include "third_party/blink/renderer/platform/graphics/compositing/paint_artifact_compositor.h"
 
 using testing::_;
@@ -397,6 +398,91 @@ TEST_P(PaintLayerScrollableAreaTest, OverlayScrollbarColorThemeUpdated) {
             black_layer->GetScrollableArea()->GetScrollbarOverlayColorTheme());
 }
 
+TEST_P(PaintLayerScrollableAreaTest,
+       RecalculatesScrollbarOverlayIfBackgroundChanges) {
+  SetBodyInnerHTML(R"HTML(
+    <style>
+      #scroller {
+        width: 10px;
+        height: 10px;
+        overflow: scroll;
+      }
+      .forcescroll { height: 1000px; }
+    </style>
+    <div id="scroller">
+      <div class="forcescroll"></div>
+    </div>
+  )HTML");
+  PaintLayer* scroll_paint_layer = GetPaintLayerByElementId("scroller");
+  EXPECT_EQ(
+      ScrollbarOverlayColorTheme::kScrollbarOverlayColorThemeDark,
+      scroll_paint_layer->GetScrollableArea()->GetScrollbarOverlayColorTheme());
+
+  GetElementById("scroller")
+      ->setAttribute(html_names::kStyleAttr, "background: rgb(34, 85, 51);");
+  UpdateAllLifecyclePhasesForTest();
+  EXPECT_EQ(
+      ScrollbarOverlayColorTheme::kScrollbarOverlayColorThemeLight,
+      scroll_paint_layer->GetScrollableArea()->GetScrollbarOverlayColorTheme());
+
+  GetElementById("scroller")
+      ->setAttribute(html_names::kStyleAttr, "background: rgb(236, 143, 185);");
+  UpdateAllLifecyclePhasesForTest();
+  EXPECT_EQ(
+      ScrollbarOverlayColorTheme::kScrollbarOverlayColorThemeDark,
+      scroll_paint_layer->GetScrollableArea()->GetScrollbarOverlayColorTheme());
+}
+
+// The scrollbar overlay color theme should follow the used color scheme when a
+// background color is not available on the scroller itself.
+TEST_P(PaintLayerScrollableAreaTest, PreferredOverlayScrollbarColorTheme) {
+  ColorSchemeHelper color_scheme_helper(GetDocument());
+  color_scheme_helper.SetPreferredColorScheme(
+      mojom::blink::PreferredColorScheme::kDark);
+  SetBodyInnerHTML(R"HTML(
+    <meta name="color-scheme" content="light dark">
+    <style>
+      .scroller {
+        width: 10px;
+        height: 10px;
+        overflow: scroll;
+      }
+      #white { background-color: white; }
+      #black { background-color: black; }
+      .forcescroll { height: 1000px; }
+    </style>
+    <div class="scroller" id="none">
+      <div class="forcescroll"></div>
+    </div>
+    <div class="scroller" id="white">
+      <div class="forcescroll"></div>
+    </div>
+    <div class="scroller" id="black">
+      <div class="forcescroll"></div>
+    </div>
+  )HTML");
+
+  PaintLayer* none_layer = GetPaintLayerByElementId("none");
+  PaintLayer* white_layer = GetPaintLayerByElementId("white");
+  PaintLayer* black_layer = GetPaintLayerByElementId("black");
+  EXPECT_EQ(ScrollbarOverlayColorTheme::kScrollbarOverlayColorThemeLight,
+            none_layer->GetScrollableArea()->GetScrollbarOverlayColorTheme());
+  EXPECT_EQ(ScrollbarOverlayColorTheme::kScrollbarOverlayColorThemeDark,
+            white_layer->GetScrollableArea()->GetScrollbarOverlayColorTheme());
+  EXPECT_EQ(ScrollbarOverlayColorTheme::kScrollbarOverlayColorThemeLight,
+            black_layer->GetScrollableArea()->GetScrollbarOverlayColorTheme());
+
+  color_scheme_helper.SetPreferredColorScheme(
+      mojom::blink::PreferredColorScheme::kLight);
+  UpdateAllLifecyclePhasesForTest();
+  EXPECT_EQ(ScrollbarOverlayColorTheme::kScrollbarOverlayColorThemeDark,
+            none_layer->GetScrollableArea()->GetScrollbarOverlayColorTheme());
+  EXPECT_EQ(ScrollbarOverlayColorTheme::kScrollbarOverlayColorThemeDark,
+            white_layer->GetScrollableArea()->GetScrollbarOverlayColorTheme());
+  EXPECT_EQ(ScrollbarOverlayColorTheme::kScrollbarOverlayColorThemeLight,
+            black_layer->GetScrollableArea()->GetScrollbarOverlayColorTheme());
+}
+
 TEST_P(PaintLayerScrollableAreaTest, HideTooltipWhenScrollPositionChanges) {
   SetBodyInnerHTML(R"HTML(
     <style>

diff --git a/blink/renderer/core/scroll/scrollable_area.cc b/blink/renderer/core/scroll/scrollable_area.cc
@@ -44,6 +44,7 @@
 #include "third_party/blink/renderer/core/input/event_handler.h"
 #include "third_party/blink/renderer/core/layout/layout_box.h"
 #include "third_party/blink/renderer/core/layout/layout_shift_tracker.h"
+#include "third_party/blink/renderer/core/layout/layout_view.h"
 #include "third_party/blink/renderer/core/page/chrome_client.h"
 #include "third_party/blink/renderer/core/page/page.h"
 #include "third_party/blink/renderer/core/paint/paint_layer_scrollable_area.h"
@@ -611,19 +612,28 @@ void ScrollableArea::SetScrollbarOverlayColorTheme(
   }
 }
 
-void ScrollableArea::RecalculateScrollbarOverlayColorTheme(
-    const Color& background_color) {
+void ScrollableArea::RecalculateScrollbarOverlayColorTheme() {
   ScrollbarOverlayColorTheme old_overlay_theme =
       GetScrollbarOverlayColorTheme();
-  ScrollbarOverlayColorTheme overlay_theme = kScrollbarOverlayColorThemeDark;
-
-  // Reduce the background color from RGB to a lightness value
-  // and determine which scrollbar style to use based on a lightness
-  // heuristic.
-  double hue, saturation, lightness;
-  background_color.GetHSL(hue, saturation, lightness);
-  if (lightness <= .5 && background_color.Alpha())
-    overlay_theme = kScrollbarOverlayColorThemeLight;
+
+  // Start with a scrollbar overlay theme based on the used color scheme.
+  ScrollbarOverlayColorTheme overlay_theme =
+      UsedColorScheme() == mojom::blink::ColorScheme::kDark
+          ? kScrollbarOverlayColorThemeLight
+          : kScrollbarOverlayColorThemeDark;
+
+  // If there is a background color set on the scroller, use the lightness of
+  // the background color for the scrollbar overlay color theme.
+  if (GetLayoutBox()) {
+    Color background_color = GetLayoutBox()->StyleRef().VisitedDependentColor(
+        GetCSSPropertyBackgroundColor());
+    if (background_color.Alpha()) {
+      double hue, saturation, lightness;
+      background_color.GetHSL(hue, saturation, lightness);
+      overlay_theme = lightness <= 0.5 ? kScrollbarOverlayColorThemeLight
+                                       : kScrollbarOverlayColorThemeDark;
+    }
+  }
 
   if (old_overlay_theme != overlay_theme)
     SetScrollbarOverlayColorTheme(overlay_theme);

diff --git a/blink/renderer/core/scroll/scrollable_area.h b/blink/renderer/core/scroll/scrollable_area.h
@@ -57,7 +57,6 @@ class Layer;
 
 namespace blink {
 class ChromeClient;
-class Color;
 class CompositorAnimationTimeline;
 class Document;
 class LayoutBox;
@@ -223,7 +222,7 @@ class CORE_EXPORT ScrollableArea : public GarbageCollectedMixin {
   // overflow:overlay might be deprecated soon.
   bool HasOverlayScrollbars() const;
   void SetScrollbarOverlayColorTheme(ScrollbarOverlayColorTheme);
-  void RecalculateScrollbarOverlayColorTheme(const Color& background_color);
+  void RecalculateScrollbarOverlayColorTheme();
   ScrollbarOverlayColorTheme GetScrollbarOverlayColorTheme() const {
     return static_cast<ScrollbarOverlayColorTheme>(
         scrollbar_overlay_color_theme_);

diff --git a/blink/renderer/core/scroll/scrollable_area_test.cc b/blink/renderer/core/scroll/scrollable_area_test.cc
@@ -155,23 +155,6 @@ TEST_P(ScrollableAreaTest, InvalidatesNonCompositedScrollbarsWhenThumbMoves) {
   ThreadState::Current()->CollectAllGarbageForTesting();
 }
 
-TEST_P(ScrollableAreaTest, RecalculatesScrollbarOverlayIfBackgroundChanges) {
-  ScopedTestingPlatformSupport<TestingPlatformSupportWithMockScheduler>
-      platform;
-
-  MockScrollableArea* scrollable_area =
-      MockScrollableArea::Create(ScrollOffset(0, 100));
-
-  EXPECT_EQ(kScrollbarOverlayColorThemeDark,
-            scrollable_area->GetScrollbarOverlayColorTheme());
-  scrollable_area->RecalculateScrollbarOverlayColorTheme(Color(34, 85, 51));
-  EXPECT_EQ(kScrollbarOverlayColorThemeLight,
-            scrollable_area->GetScrollbarOverlayColorTheme());
-  scrollable_area->RecalculateScrollbarOverlayColorTheme(Color(236, 143, 185));
-  EXPECT_EQ(kScrollbarOverlayColorThemeDark,
-            scrollable_area->GetScrollbarOverlayColorTheme());
-}
-
 TEST_P(ScrollableAreaTest, ScrollableAreaDidScroll) {
   ScopedTestingPlatformSupport<TestingPlatformSupportWithMockScheduler>
       platform;