paddingBoxRect() is wrong with RTL scrollbars on the left
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 13 Jun 2019 03:21:36 +0000 (03:21 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 13 Jun 2019 03:21:36 +0000 (03:21 +0000)
https://bugs.webkit.org/show_bug.cgi?id=198816

Reviewed by Jon Lee.

Source/WebCore:

RenderBox::paddingBoxRect() needs to offset the left side of the box for the
vertical scrollbar, if it's placed on the left.

Test: compositing/geometry/rtl-overflow-scroll.html

* rendering/RenderBox.cpp:
(WebCore::RenderBox::paddingBoxRect const):
* rendering/RenderBox.h:
(WebCore::RenderBox::paddingBoxRect const): Deleted.
* rendering/RenderLayerBacking.cpp:
(WebCore::RenderLayerBacking::updateGeometry):
* rendering/RenderListBox.cpp:
(WebCore::RenderListBox::controlClipRect const):

LayoutTests:

* compositing/geometry/rtl-overflow-scroll-expected.html: Added.
* compositing/geometry/rtl-overflow-scroll.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@246389 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/compositing/geometry/rtl-overflow-scroll-expected.html [new file with mode: 0644]
LayoutTests/compositing/geometry/rtl-overflow-scroll.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderBox.cpp
Source/WebCore/rendering/RenderBox.h
Source/WebCore/rendering/RenderLayerBacking.cpp
Source/WebCore/rendering/RenderListBox.cpp

index 3d85523..eaf8977 100644 (file)
@@ -1,3 +1,13 @@
+2019-06-12  Simon Fraser  <simon.fraser@apple.com>
+
+        paddingBoxRect() is wrong with RTL scrollbars on the left
+        https://bugs.webkit.org/show_bug.cgi?id=198816
+
+        Reviewed by Jon Lee.
+
+        * compositing/geometry/rtl-overflow-scroll-expected.html: Added.
+        * compositing/geometry/rtl-overflow-scroll.html: Added.
+
 2019-06-12  Eric Carlson  <eric.carlson@apple.com>
 
         [High Sierra / Mojave Debug WK2] Layout Test media/video-restricted-invisible-autoplay-allowed-when-visible.html is a flaky failure
diff --git a/LayoutTests/compositing/geometry/rtl-overflow-scroll-expected.html b/LayoutTests/compositing/geometry/rtl-overflow-scroll-expected.html
new file mode 100644 (file)
index 0000000..74d5bc9
--- /dev/null
@@ -0,0 +1,37 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ internal:AsyncOverflowScrollingEnabled=true ] -->
+<html>
+<head>
+    <style>
+        .scroller {
+            direction: rtl;
+            width: 300px;
+            height: 300px;
+            border: 5px solid black;
+            padding: 2px;
+            box-sizing: border-box;
+            overflow: hidden;
+        }
+        
+        .contents {
+            width: 200%;
+            height: 200%;
+            background-color: green;
+        }
+        
+        .scrollbar-hider {
+            position: absolute;
+            top: 8px;
+            left: 13px;
+            width: 16px;
+            height: 300px;
+            background-color: gray;
+        }
+    </style>
+</head>
+<body>
+    <div class="scroller">
+        <div class="contents"></div>
+    </div>
+    <div class="scrollbar-hider"></div>
+</body>
+</html>
diff --git a/LayoutTests/compositing/geometry/rtl-overflow-scroll.html b/LayoutTests/compositing/geometry/rtl-overflow-scroll.html
new file mode 100644 (file)
index 0000000..dedd8f9
--- /dev/null
@@ -0,0 +1,38 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ internal:AsyncOverflowScrollingEnabled=true ] -->
+<html>
+<head>
+    <style>
+        .scroller {
+            direction: rtl;
+            width: 300px;
+            height: 300px;
+            border: 5px solid black;
+            padding: 2px;
+            box-sizing: border-box;
+            overflow-y: scroll;
+            overflow-x: hidden;
+        }
+        
+        .contents {
+            width: 200%;
+            height: 200%;
+            background-color: green;
+        }
+        
+        .scrollbar-hider {
+            position: absolute;
+            top: 8px;
+            left: 13px;
+            width: 16px;
+            height: 300px;
+            background-color: gray;
+        }
+    </style>
+</head>
+<body>
+    <div class="scroller">
+        <div class="contents"></div>
+    </div>
+    <div class="scrollbar-hider"></div>
+</body>
+</html>
index cd10c31..7672799 100644 (file)
@@ -1,3 +1,24 @@
+2019-06-12  Simon Fraser  <simon.fraser@apple.com>
+
+        paddingBoxRect() is wrong with RTL scrollbars on the left
+        https://bugs.webkit.org/show_bug.cgi?id=198816
+
+        Reviewed by Jon Lee.
+
+        RenderBox::paddingBoxRect() needs to offset the left side of the box for the
+        vertical scrollbar, if it's placed on the left.
+
+        Test: compositing/geometry/rtl-overflow-scroll.html
+
+        * rendering/RenderBox.cpp:
+        (WebCore::RenderBox::paddingBoxRect const):
+        * rendering/RenderBox.h:
+        (WebCore::RenderBox::paddingBoxRect const): Deleted.
+        * rendering/RenderLayerBacking.cpp:
+        (WebCore::RenderLayerBacking::updateGeometry):
+        * rendering/RenderListBox.cpp:
+        (WebCore::RenderListBox::controlClipRect const):
+
 2019-06-12  Youenn Fablet  <youenn@apple.com>
 
         Use NSURLSession for WebSocket
index c71e73c..c995bfd 100644 (file)
@@ -658,6 +658,16 @@ RoundedRect::Radii RenderBox::borderRadii() const
     return style.getRoundedBorderFor(bounds).radii();
 }
 
+LayoutRect RenderBox::paddingBoxRect() const
+{
+    auto verticalScrollbarWidth = this->verticalScrollbarWidth();
+    LayoutUnit offsetForScrollbar = shouldPlaceBlockDirectionScrollbarOnLeft() ? verticalScrollbarWidth : 0;
+
+    return LayoutRect(borderLeft() + offsetForScrollbar, borderTop(),
+        width() - borderLeft() - borderRight() - verticalScrollbarWidth,
+        height() - borderTop() - borderBottom() - horizontalScrollbarHeight());
+}
+
 LayoutRect RenderBox::contentBoxRect() const
 {
     return { contentBoxLocation(), contentSize() };
index b786508..b2afd00 100644 (file)
@@ -219,7 +219,7 @@ public:
 
     LayoutUnit paddingBoxWidth() const { return width() - borderLeft() - borderRight() - verticalScrollbarWidth(); }
     LayoutUnit paddingBoxHeight() const { return height() - borderTop() - borderBottom() - horizontalScrollbarHeight(); }
-    LayoutRect paddingBoxRect() const { return LayoutRect(borderLeft(), borderTop(), paddingBoxWidth(), paddingBoxHeight()); }
+    LayoutRect paddingBoxRect() const;
     LayoutRect paddingBoxRectIncludingScrollbar() const { return LayoutRect(borderLeft(), borderTop(), width() - borderLeft() - borderRight(), height() - borderTop() - borderBottom()); }
 
     // IE extensions. Used to calculate offsetWidth/Height.  Overridden by inlines (RenderFlow)
index e3b6694..c820832 100644 (file)
@@ -1216,11 +1216,11 @@ void RenderLayerBacking::updateGeometry()
     if (m_scrollContainerLayer) {
         ASSERT(m_scrolledContentsLayer);
         auto& renderBox = downcast<RenderBox>(renderer());
-        LayoutRect paddingBoxIncludingScrollbar = renderBox.paddingBoxRectIncludingScrollbar();
+        LayoutRect paddingBox = renderBox.paddingBoxRect();
         LayoutRect parentLayerBounds = clippingLayer() ? clippingBox : compositedBounds();
 
         // FIXME: need to do some pixel snapping here.
-        m_scrollContainerLayer->setPosition(FloatPoint(paddingBoxIncludingScrollbar.location() - parentLayerBounds.location()));
+        m_scrollContainerLayer->setPosition(FloatPoint(paddingBox.location() - parentLayerBounds.location()));
         m_scrollContainerLayer->setSize(roundedIntSize(LayoutSize(renderBox.paddingBoxWidth(), renderBox.paddingBoxHeight())));
 
         ScrollOffset scrollOffset = m_owningLayer.scrollOffset();
@@ -1230,12 +1230,12 @@ void RenderLayerBacking::updateGeometry()
 #endif
 
         FloatSize oldScrollingLayerOffset = m_scrollContainerLayer->offsetFromRenderer();
-        m_scrollContainerLayer->setOffsetFromRenderer(toFloatSize(paddingBoxIncludingScrollbar.location()));
+        m_scrollContainerLayer->setOffsetFromRenderer(toFloatSize(paddingBox.location()));
 
         if (m_childClippingMaskLayer) {
             m_childClippingMaskLayer->setPosition(m_scrollContainerLayer->position());
             m_childClippingMaskLayer->setSize(m_scrollContainerLayer->size());
-            m_childClippingMaskLayer->setOffsetFromRenderer(toFloatSize(paddingBoxIncludingScrollbar.location()));
+            m_childClippingMaskLayer->setOffsetFromRenderer(toFloatSize(paddingBox.location()));
         }
 
         bool paddingBoxOffsetChanged = oldScrollingLayerOffset != m_scrollContainerLayer->offsetFromRenderer();
@@ -1246,7 +1246,7 @@ void RenderLayerBacking::updateGeometry()
 
         m_scrolledContentsLayer->setSize(scrollSize);
         m_scrolledContentsLayer->setScrollOffset(scrollOffset, GraphicsLayer::DontSetNeedsDisplay);
-        m_scrolledContentsLayer->setOffsetFromRenderer(toLayoutSize(paddingBoxIncludingScrollbar.location()), GraphicsLayer::DontSetNeedsDisplay);
+        m_scrolledContentsLayer->setOffsetFromRenderer(toLayoutSize(paddingBox.location()), GraphicsLayer::DontSetNeedsDisplay);
         
         adjustTiledBackingCoverage();
     }
index 44c6d1f..a696bf1 100644 (file)
@@ -804,8 +804,6 @@ LayoutRect RenderListBox::controlClipRect(const LayoutPoint& additionalOffset) c
     // Clip against the padding box, to give <option>s and overlay scrollbar some extra space
     // to get painted.
     LayoutRect clipRect = paddingBoxRect();
-    if (shouldPlaceBlockDirectionScrollbarOnLeft())
-        clipRect.move(m_vBar->occupiedWidth(), 0);
     clipRect.moveBy(additionalOffset);
     return clipRect;
 }