Changeset 275811 in webkit

Timestamp:

Apr 11, 2021 3:53:50 PM (15 months ago)

Author:

Cameron McCormack

Message:

Fix initial horizontal scrollbar position when vertical scrollbar is on the left.
​https://bugs.webkit.org/show_bug.cgi?id=224409

Reviewed by Darin Adler.

Source/WebCore:

Scrollable elements that place their vertical scrollbar on the left
(e.g. when they're direction: rtl or when the OS language is RTL and
the relevant setting to always follow OS scrollbar side is used) have
an incorrect scroll origin, which leads to creating a horizontal
scrollbar with an incorrect initial offset. This is because
RenderLayerScrollableArea::computeScrollDimensions runs prior to the
scrollbars being created. So whenever we start or stop having a
(non-overlay) vertical scrollbar on the left, we need to recompute
the scroll origin and fix up the horizontal scrollbar's offset.

Tests: fast/scrolling/rtl-scrollbars-initial-position-dynamic.html

fast/scrolling/rtl-scrollbars-initial-position.html

• rendering/RenderLayerScrollableArea.cpp:

(WebCore::RenderLayerScrollableArea::computeScrollDimensions):
(WebCore::RenderLayerScrollableArea::computeScrollOrigin): Split this
out of computeScrollDimensions so we can call it from
updateScrollbarsAfterLayout and updateScrollbarsAfterStyleChange, and
have it ask the horizontal scrollbar to update its offset. We could
condition this on the scroll origin actually having changed, but
that's going to be a similar check that Scrollbar::offsetDidChange
does to ensure the offset value did indeed change. We don't want to
condition this on shouldPlaceBlockDirectionScrollbarOnLeft(), since
that's dependent on the current style, and we may be reacting to a
style change removed the vertical scrollbar on the left.
(WebCore::RenderLayerScrollableArea::updateScrollbarsAfterLayout):
(WebCore::RenderLayerScrollableArea::updateScrollbarsAfterStyleChange):

• rendering/RenderLayerScrollableArea.h:

LayoutTests:

• fast/scrolling/rtl-scrollbars-initial-position-dynamic-expected.html: Added.
• fast/scrolling/rtl-scrollbars-initial-position-dynamic.html: Added.
• fast/scrolling/rtl-scrollbars-initial-position-expected.html: Added.
• fast/scrolling/rtl-scrollbars-initial-position.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/scrolling/rtl-scrollbars-initial-position-dynamic-expected.html (added)
• LayoutTests/fast/scrolling/rtl-scrollbars-initial-position-dynamic.html (added)
• LayoutTests/fast/scrolling/rtl-scrollbars-initial-position-expected.html (added)
• LayoutTests/fast/scrolling/rtl-scrollbars-initial-position.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/rendering/RenderLayerScrollableArea.cpp (modified) (4 diffs)
• Source/WebCore/rendering/RenderLayerScrollableArea.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2021-04-11  Cameron McCormack  <heycam@apple.com>
+
+        Fix initial horizontal scrollbar position when vertical scrollbar is on the left.
+        https://bugs.webkit.org/show_bug.cgi?id=224409
+
+        Reviewed by Darin Adler.
+
+        * fast/scrolling/rtl-scrollbars-initial-position-dynamic-expected.html: Added.
+        * fast/scrolling/rtl-scrollbars-initial-position-dynamic.html: Added.
+        * fast/scrolling/rtl-scrollbars-initial-position-expected.html: Added.
+        * fast/scrolling/rtl-scrollbars-initial-position.html: Added.
+
 2021-04-11  Sam Weinig  <weinig@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-04-11  Cameron McCormack  <heycam@apple.com>
+
+        Fix initial horizontal scrollbar position when vertical scrollbar is on the left.
+        https://bugs.webkit.org/show_bug.cgi?id=224409
+
+        Reviewed by Darin Adler.
+
+        Scrollable elements that place their vertical scrollbar on the left
+        (e.g. when they're `direction: rtl` or when the OS language is RTL and
+        the relevant setting to always follow OS scrollbar side is used) have
+        an incorrect scroll origin, which leads to creating a horizontal
+        scrollbar with an incorrect initial offset.  This is because
+        RenderLayerScrollableArea::computeScrollDimensions runs prior to the
+        scrollbars being created.  So whenever we start or stop having a
+        (non-overlay) vertical scrollbar on the left, we need to recompute
+        the scroll origin and fix up the horizontal scrollbar's offset.
+
+        Tests: fast/scrolling/rtl-scrollbars-initial-position-dynamic.html
+               fast/scrolling/rtl-scrollbars-initial-position.html
+
+        * rendering/RenderLayerScrollableArea.cpp:
+        (WebCore::RenderLayerScrollableArea::computeScrollDimensions):
+        (WebCore::RenderLayerScrollableArea::computeScrollOrigin): Split this
+        out of computeScrollDimensions so we can call it from
+        updateScrollbarsAfterLayout and updateScrollbarsAfterStyleChange, and
+        have it ask the horizontal scrollbar to update its offset.  We could
+        condition this on the scroll origin actually having changed, but
+        that's going to be a similar check that Scrollbar::offsetDidChange
+        does to ensure the offset value did indeed change.  We don't want to
+        condition this on shouldPlaceBlockDirectionScrollbarOnLeft(), since
+        that's dependent on the current style, and we may be reacting to a
+        style change removed the vertical scrollbar on the left.
+        (WebCore::RenderLayerScrollableArea::updateScrollbarsAfterLayout):
+        (WebCore::RenderLayerScrollableArea::updateScrollbarsAfterStyleChange):
+        * rendering/RenderLayerScrollableArea.h:
+
 2021-04-11  Darin Adler  <darin@apple.com>
 

trunk/Source/WebCore/rendering/RenderLayerScrollableArea.cpp

@@ -1007 +1007 @@
 void RenderLayerScrollableArea::computeScrollDimensions()
 {
+    m_scrollDimensionsDirty = false;
+
+    m_scrollWidth = roundToInt(overflowRight() - overflowLeft());
+    m_scrollHeight = roundToInt(overflowBottom() - overflowTop());
+
+    computeScrollOrigin();
+    computeHasCompositedScrollableOverflow();
+}
+
+void RenderLayerScrollableArea::computeScrollOrigin()
+{
     RenderBox* box = m_layer.renderBox();
     ASSERT(box);
-
-    m_scrollDimensionsDirty = false;
-
-    m_scrollWidth = roundToInt(overflowRight() - overflowLeft());
-    m_scrollHeight = roundToInt(overflowBottom() - overflowTop());
 
     int scrollableLeftOverflow = roundToInt(overflowLeft() - box->borderLeft());
@@ -1021 +1027 @@
     setScrollOrigin(IntPoint(-scrollableLeftOverflow, -scrollableTopOverflow));
 
-    computeHasCompositedScrollableOverflow();
+    // Horizontal scrollbar offsets depend on the scroll origin when vertical
+    // scrollbars are on the left.
+    if (m_hBar)
+        m_hBar->offsetDidChange();
 }
 
@@ -1095 +1104 @@
         if (box->hasVerticalScrollbarWithAutoBehavior())
             setHasVerticalScrollbar(hasVerticalOverflow);
+
+        if (autoVerticalScrollBarChanged && shouldPlaceBlockDirectionScrollbarOnLeft())
+            computeScrollOrigin();
 
         m_layer.updateSelfPaintingLayer();
@@ -1553 +1565 @@
 
     // To avoid doing a relayout in updateScrollbarsAfterLayout, we try to keep any automatic scrollbar that was already present.
-    bool needsHorizontalScrollbar = box->hasOverflowClip() && ((horizontalScrollbar() && styleDefinesAutomaticScrollbar(box->style(), HorizontalScrollbar)) || styleRequiresScrollbar(box->style(), HorizontalScrollbar));
-    bool needsVerticalScrollbar = box->hasOverflowClip() && ((verticalScrollbar() && styleDefinesAutomaticScrollbar(box->style(), VerticalScrollbar)) || styleRequiresScrollbar(box->style(), VerticalScrollbar));
+    bool hadVerticalScrollbar = m_vBar;
+    bool needsHorizontalScrollbar = box->hasOverflowClip() && ((m_hBar && styleDefinesAutomaticScrollbar(box->style(), HorizontalScrollbar)) || styleRequiresScrollbar(box->style(), HorizontalScrollbar));
+    bool needsVerticalScrollbar = box->hasOverflowClip() && ((m_vBar && styleDefinesAutomaticScrollbar(box->style(), VerticalScrollbar)) || styleRequiresScrollbar(box->style(), VerticalScrollbar));
     setHasHorizontalScrollbar(needsHorizontalScrollbar);
     setHasVerticalScrollbar(needsVerticalScrollbar);
+
+    if (hadVerticalScrollbar != needsVerticalScrollbar || (needsVerticalScrollbar && oldStyle && box->style().shouldPlaceBlockDirectionScrollbarOnLeft() != oldStyle->shouldPlaceBlockDirectionScrollbarOnLeft()))
+        computeScrollOrigin();
 
     // With non-overlay overflow:scroll, scrollbars are always visible but may be disabled.

trunk/Source/WebCore/rendering/RenderLayerScrollableArea.h

@@ -232 +232 @@
 
     void computeScrollDimensions();
+    void computeScrollOrigin();
     void computeHasCompositedScrollableOverflow();