REGRESSION (r215613): Incorrect corners clipping with border-radius
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 20 Sep 2017 02:20:28 +0000 (02:20 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 20 Sep 2017 02:20:28 +0000 (02:20 +0000)
https://bugs.webkit.org/show_bug.cgi?id=176498
<rdar://problem/34112607>

Reviewed by Tim Horton.

Source/WebCore:

http://trac.webkit.org/r215613 introduced an optimization to bail out of repainting borders if the invalidated
rect to paint is fully contained within the inner rounded rect of the border. However, due to issues with
coordinate and intersection math in RoundedRect::contains() and ellipseContainsPoint(), this causes
RenderBoxModelObject::paintBorder to return early even in circumstances where the border requires a repaint.
This patch fixes the contains() helper in RoundedRect and adds a new API test suite for RoundedRect that covers
these changes.

Test: WebCore.RoundedRectContainsRect

* platform/graphics/GeometryUtilities.cpp:
(WebCore::ellipseContainsPoint):

This function attempts to return early if the Manhattan distance of the transformed point is less than the
radius of the circle that results from applying the same transformation to the ellipse. However, this bails and
returns true if `x + y <= R`, but this means that if x and y are negative, we'll always end up returning true.
We fix this by adding the absolute values instead, so the check becomes: |x| + |y| <= R.

* platform/graphics/RoundedRect.cpp:
(WebCore::RoundedRect::contains const):

Before this patch, otherRect's upper left location was being used to hit-test against the ellipses formed from
each of the 4 corners of the rounded rect. Instead, this should use (x, y), (maxX, y), (x, maxY), (maxX, maxY)
for the top left, top right, bottom left, and bottom right corners, respectively.

Additionally, the checks for the bottom left and bottom right to determine whether the rect corner should be
checked for intersection against the ellipse's corner are incorrect. In the bottom left corner, the check for
`otherRect.maxX() >= center.x()` should instead be `otherRect.x() <= center.x()`, and the check for
`otherRect.x() <= center.x()` should instead be `otherRect.maxX() >= center.x()`.

* platform/graphics/RoundedRect.h:

Tools:

Add WebCore API tests for RoundedRect::contains().

* TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* TestWebKitAPI/Tests/WebCore/RoundedRectTests.cpp: Added.
(TestWebKitAPI::layoutRect):
(TestWebKitAPI::TEST):

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

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/GeometryUtilities.cpp
Source/WebCore/platform/graphics/RoundedRect.cpp
Source/WebCore/platform/graphics/RoundedRect.h
Tools/ChangeLog
Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
Tools/TestWebKitAPI/Tests/WebCore/RoundedRectTests.cpp [new file with mode: 0644]

index 644638e..f73313a 100644 (file)
@@ -1,3 +1,42 @@
+2017-09-19  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        REGRESSION (r215613): Incorrect corners clipping with border-radius
+        https://bugs.webkit.org/show_bug.cgi?id=176498
+        <rdar://problem/34112607>
+
+        Reviewed by Tim Horton.
+
+        http://trac.webkit.org/r215613 introduced an optimization to bail out of repainting borders if the invalidated
+        rect to paint is fully contained within the inner rounded rect of the border. However, due to issues with
+        coordinate and intersection math in RoundedRect::contains() and ellipseContainsPoint(), this causes
+        RenderBoxModelObject::paintBorder to return early even in circumstances where the border requires a repaint.
+        This patch fixes the contains() helper in RoundedRect and adds a new API test suite for RoundedRect that covers
+        these changes.
+
+        Test: WebCore.RoundedRectContainsRect
+
+        * platform/graphics/GeometryUtilities.cpp:
+        (WebCore::ellipseContainsPoint):
+
+        This function attempts to return early if the Manhattan distance of the transformed point is less than the
+        radius of the circle that results from applying the same transformation to the ellipse. However, this bails and
+        returns true if `x + y <= R`, but this means that if x and y are negative, we'll always end up returning true.
+        We fix this by adding the absolute values instead, so the check becomes: |x| + |y| <= R.
+
+        * platform/graphics/RoundedRect.cpp:
+        (WebCore::RoundedRect::contains const):
+
+        Before this patch, otherRect's upper left location was being used to hit-test against the ellipses formed from
+        each of the 4 corners of the rounded rect. Instead, this should use (x, y), (maxX, y), (x, maxY), (maxX, maxY)
+        for the top left, top right, bottom left, and bottom right corners, respectively.
+
+        Additionally, the checks for the bottom left and bottom right to determine whether the rect corner should be
+        checked for intersection against the ellipse's corner are incorrect. In the bottom left corner, the check for
+        `otherRect.maxX() >= center.x()` should instead be `otherRect.x() <= center.x()`, and the check for
+        `otherRect.x() <= center.x()` should instead be `otherRect.maxX() >= center.x()`.
+
+        * platform/graphics/RoundedRect.h:
+
 2017-09-19  Alexey Proskuryakov  <ap@apple.com>
 
         Layering violation in Editor::createFragment
index 0276511..b33fa2a 100644 (file)
@@ -155,16 +155,29 @@ FloatSize sizeWithAreaAndAspectRatio(float area, float aspectRatio)
 
 bool ellipseContainsPoint(const FloatPoint& center, const FloatSize& radii, const FloatPoint& point)
 {
+    if (radii.width() <= 0 || radii.height() <= 0)
+        return false;
+
+    // First, offset the query point so that the ellipse is effectively centered at the origin.
     FloatPoint transformedPoint(point);
     transformedPoint.move(-center.x(), -center.y());
-    transformedPoint.scale(radii.height(), radii.width());
-    float radius = radii.width() * radii.height();
 
-    if (transformedPoint.x() > radius || transformedPoint.y() > radius)
+    // If the point lies outside of the bounding box determined by the radii of the ellipse, it can't possibly
+    // be contained within the ellipse, so bail early.
+    if (transformedPoint.x() < -radii.width() || transformedPoint.x() > radii.width() || transformedPoint.y() < -radii.height() || transformedPoint.y() > radii.height())
         return false;
-    if (transformedPoint.x() + transformedPoint.y() <= radius)
-        return true;
-    return (transformedPoint.lengthSquared() <= radius * radius);
+
+    // Let (x, y) represent the translated point, and let (Rx, Ry) represent the radii of an ellipse centered at the origin.
+    // (x, y) is contained within the ellipse if, after scaling the ellipse to be a unit circle, the identically scaled
+    // point lies within that unit circle. In other words, the squared distance (x/Rx)^2 + (y/Ry)^2 of the transformed point
+    // to the origin is no greater than 1. This is equivalent to checking whether or not the point (xRy, yRx) lies within a
+    // circle of radius RxRy.
+    transformedPoint.scale(radii.height(), radii.width());
+    auto transformedRadius = radii.width() * radii.height();
+
+    // We can bail early if |xRy| + |yRx| <= RxRy to avoid additional multiplications, since that means the Manhattan distance
+    // of the transformed point is less than the radius, so the point must lie within the transformed circle.
+    return std::abs(transformedPoint.x()) + std::abs(transformedPoint.y()) <= transformedRadius || transformedPoint.lengthSquared() <= transformedRadius * transformedRadius;
 }
 
 }
index 60a451f..471d4b3 100644 (file)
@@ -239,14 +239,14 @@ bool RoundedRect::intersectsQuad(const FloatQuad& quad) const
 
 bool RoundedRect::contains(const LayoutRect& otherRect) const
 {
-    if (!rect().contains(otherRect))
+    if (!rect().contains(otherRect) || !isRenderable())
         return false;
 
     const LayoutSize& topLeft = m_radii.topLeft();
     if (!topLeft.isEmpty()) {
         FloatPoint center = { m_rect.x() + topLeft.width(), m_rect.y() + topLeft.height() };
         if (otherRect.x() <= center.x() && otherRect.y() <= center.y()) {
-            if (!ellipseContainsPoint(center, topLeft, otherRect.location()))
+            if (!ellipseContainsPoint(center, topLeft, otherRect.minXMinYCorner()))
                 return false;
         }
     }
@@ -255,7 +255,7 @@ bool RoundedRect::contains(const LayoutRect& otherRect) const
     if (!topRight.isEmpty()) {
         FloatPoint center = { m_rect.maxX() - topRight.width(), m_rect.y() + topRight.height() };
         if (otherRect.maxX() >= center.x() && otherRect.y() <= center.y()) {
-            if (!ellipseContainsPoint(center, topRight, otherRect.location()))
+            if (!ellipseContainsPoint(center, topRight, otherRect.maxXMinYCorner()))
                 return false;
         }
     }
@@ -263,8 +263,8 @@ bool RoundedRect::contains(const LayoutRect& otherRect) const
     const LayoutSize& bottomLeft = m_radii.bottomLeft();
     if (!bottomLeft.isEmpty()) {
         FloatPoint center = { m_rect.x() + bottomLeft.width(), m_rect.maxY() - bottomLeft.height() };
-        if (otherRect.maxX() >= center.x() && otherRect.maxY() >= center.y()) {
-            if (!ellipseContainsPoint(center, bottomLeft, otherRect.location()))
+        if (otherRect.x() <= center.x() && otherRect.maxY() >= center.y()) {
+            if (!ellipseContainsPoint(center, bottomLeft, otherRect.minXMaxYCorner()))
                 return false;
         }
     }
@@ -272,8 +272,8 @@ bool RoundedRect::contains(const LayoutRect& otherRect) const
     const LayoutSize& bottomRight = m_radii.bottomRight();
     if (!bottomRight.isEmpty()) {
         FloatPoint center = { m_rect.maxX() - bottomRight.width(), m_rect.maxY() - bottomRight.height() };
-        if (otherRect.x() <= center.x() && otherRect.maxY() >= center.y()) {
-            if (!ellipseContainsPoint(center, bottomRight, otherRect.location()))
+        if (otherRect.maxX() >= center.x() && otherRect.maxY() >= center.y()) {
+            if (!ellipseContainsPoint(center, bottomRight, otherRect.maxXMaxYCorner()))
                 return false;
         }
     }
index 3f2f7eb..a2852eb 100644 (file)
@@ -80,7 +80,7 @@ public:
 
     explicit RoundedRect(const LayoutRect&, const Radii& = Radii());
     RoundedRect(const LayoutUnit&, const LayoutUnit&, const LayoutUnit& width, const LayoutUnit& height);
-    RoundedRect(const LayoutRect&, const LayoutSize& topLeft, const LayoutSize& topRight, const LayoutSize& bottomLeft, const LayoutSize& bottomRight);
+    WEBCORE_EXPORT RoundedRect(const LayoutRect&, const LayoutSize& topLeft, const LayoutSize& topRight, const LayoutSize& bottomLeft, const LayoutSize& bottomRight);
 
     const LayoutRect& rect() const { return m_rect; }
     const Radii& radii() const { return m_radii; }
@@ -106,7 +106,7 @@ public:
     // Tests whether the quad intersects any part of this rounded rectangle.
     // This only works for convex quads.
     bool intersectsQuad(const FloatQuad&) const;
-    bool contains(const LayoutRect&) const;
+    WEBCORE_EXPORT bool contains(const LayoutRect&) const;
 
     FloatRoundedRect pixelSnappedRoundedRectForPainting(float deviceScaleFactor) const;
 
index 3a52d74..f0b326c 100644 (file)
@@ -1,3 +1,18 @@
+2017-09-19  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        REGRESSION (r215613): Incorrect corners clipping with border-radius
+        https://bugs.webkit.org/show_bug.cgi?id=176498
+        <rdar://problem/34112607>
+
+        Reviewed by Tim Horton.
+
+        Add WebCore API tests for RoundedRect::contains().
+
+        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
+        * TestWebKitAPI/Tests/WebCore/RoundedRectTests.cpp: Added.
+        (TestWebKitAPI::layoutRect):
+        (TestWebKitAPI::TEST):
+
 2017-09-19  Youenn Fablet  <youenn@apple.com>
 
         Allow WTF::map to use any class that is iterable and has a size getter
index 2455d99..f1db8e0 100644 (file)
                ECA680CE1E68CC0900731D20 /* StringUtilities.mm in Sources */ = {isa = PBXBuildFile; fileRef = ECA680CD1E68CC0900731D20 /* StringUtilities.mm */; };
                F407FE391F1D0DFC0017CF25 /* enormous.svg in Copy Resources */ = {isa = PBXBuildFile; fileRef = F407FE381F1D0DE60017CF25 /* enormous.svg */; };
                F415086D1DA040C50044BE9B /* play-audio-on-click.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = F415086C1DA040C10044BE9B /* play-audio-on-click.html */; };
+               F418BE151F71B7DC001970E6 /* RoundedRectTests.cpp in Sources */ = {isa = PBXBuildFile; fileRef = F418BE141F71B7DC001970E6 /* RoundedRectTests.cpp */; };
                F4194AD11F5A320100ADD83F /* drop-targets.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = F4194AD01F5A2EA500ADD83F /* drop-targets.html */; };
                F41AB99F1EF4696B0083FA08 /* autofocus-contenteditable.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = F41AB9981EF4692C0083FA08 /* autofocus-contenteditable.html */; };
                F41AB9A01EF4696B0083FA08 /* background-image-link-and-input.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = F41AB9971EF4692C0083FA08 /* background-image-link-and-input.html */; };
                F3FC3EE213678B7300126A65 /* libgtest.a */ = {isa = PBXFileReference; lastKnownFileType = archive.ar; path = libgtest.a; sourceTree = BUILT_PRODUCTS_DIR; };
                F407FE381F1D0DE60017CF25 /* enormous.svg */ = {isa = PBXFileReference; lastKnownFileType = text; path = enormous.svg; sourceTree = "<group>"; };
                F415086C1DA040C10044BE9B /* play-audio-on-click.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = "play-audio-on-click.html"; sourceTree = "<group>"; };
+               F418BE141F71B7DC001970E6 /* RoundedRectTests.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = RoundedRectTests.cpp; sourceTree = "<group>"; };
                F4194AD01F5A2EA500ADD83F /* drop-targets.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = "drop-targets.html"; sourceTree = "<group>"; };
                F41AB9931EF4692C0083FA08 /* image-and-textarea.html */ = {isa = PBXFileReference; lastKnownFileType = text.html; path = "image-and-textarea.html"; sourceTree = "<group>"; };
                F41AB9941EF4692C0083FA08 /* prevent-operation.html */ = {isa = PBXFileReference; lastKnownFileType = text.html; path = "prevent-operation.html"; sourceTree = "<group>"; };
                                076E507E1F45031E006E9F5A /* Logging.cpp */,
                                A5B149DD1F5A19DC00C6DAFF /* MIMETypeRegistry.cpp */,
                                CD225C071C45A69200140761 /* ParsedContentRange.cpp */,
+                               F418BE141F71B7DC001970E6 /* RoundedRectTests.cpp */,
                                CDCFA7A91E45122F00C2433D /* SampleMap.cpp */,
                                CE06DF9A1E1851F200E570C9 /* SecurityOrigin.cpp */,
                                41973B5C1AF22875006C7B36 /* SharedBuffer.cpp */,
                                7CCE7F111A411AE600447C4C /* RestoreSessionStateContainingFormData.cpp in Sources */,
                                835CF9671D25FCD6001A65D4 /* RestoreSessionStateWithoutNavigation.cpp in Sources */,
                                46E816F81E79E29C00375ADC /* RestoreStateAfterTermination.mm in Sources */,
+                               F418BE151F71B7DC001970E6 /* RoundedRectTests.cpp in Sources */,
                                A180C0FA1EE67DF000468F47 /* RunOpenPanel.mm in Sources */,
                                CDCFA7AA1E45183200C2433D /* SampleMap.cpp in Sources */,
                                7CCE7F121A411AE600447C4C /* ScrollPinningBehaviors.cpp in Sources */,
                        buildActionMask = 2147483647;
                        files = (
                                37E7DD671EA071F3009B396D /* AdditionalReadAccessAllowedURLsPlugin.mm in Sources */,
+                               754CEC811F6722F200D0039A /* AutoFillAvailable.mm in Sources */,
                                374B7A611DF371CF00ACCB6C /* BundleEditingDelegatePlugIn.mm in Sources */,
                                A13EBBB01B87436F00097110 /* BundleParametersPlugIn.mm in Sources */,
                                37A709AF1E3EA97E00CA5969 /* BundleRangeHandlePlugIn.mm in Sources */,
                                A14FC58B1B89927100D107EB /* ContentFilteringPlugIn.mm in Sources */,
                                A13EBBAB1B87434600097110 /* PlatformUtilitiesCocoa.mm in Sources */,
                                1A4F81CF1BDFFD53004E672E /* RemoteObjectRegistryPlugIn.mm in Sources */,
-                               754CEC811F6722F200D0039A /* AutoFillAvailable.mm in Sources */,
                                A12DDC021E837C2400CF6CAE /* RenderedImageWithOptionsPlugIn.mm in Sources */,
                                7C882E091C80C630006BF731 /* UserContentWorldPlugIn.mm in Sources */,
                                7C83E03D1D0A60D600FEBCF3 /* UtilitiesCocoa.mm in Sources */,
diff --git a/Tools/TestWebKitAPI/Tests/WebCore/RoundedRectTests.cpp b/Tools/TestWebKitAPI/Tests/WebCore/RoundedRectTests.cpp
new file mode 100644 (file)
index 0000000..963827d
--- /dev/null
@@ -0,0 +1,73 @@
+/*
+ * Copyright (C) 2017 Apple Inc. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
+ * THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include "config.h"
+
+#include <WebCore/RoundedRect.h>
+
+using namespace WebCore;
+
+namespace TestWebKitAPI {
+
+static LayoutRect layoutRect(float x, float y, float width, float height)
+{
+    return { LayoutPoint(x, y), LayoutSize(width, height) };
+}
+
+TEST(WebCore, RoundedRectContainsRect)
+{
+    auto roundedRectBounds = layoutRect(-10, -40, 40, 80);
+    RoundedRect roundedRect(roundedRectBounds, { 10, 10 }, { 10, 20 }, { 0, 0 }, { 20, 40 });
+
+    // Cases where the other rect is outside of the rounded rect's bounds.
+    EXPECT_FALSE(roundedRect.contains(layoutRect(-40, -10, 10, 10)));
+    EXPECT_FALSE(roundedRect.contains(layoutRect(-40, 10, 100, 10)));
+    EXPECT_FALSE(roundedRect.contains(layoutRect(20, 20, 60, 60)));
+    EXPECT_FALSE(roundedRect.contains(layoutRect(10, -80, 10, 200)));
+    EXPECT_FALSE(roundedRect.contains(layoutRect(-80, -80, 160, 160)));
+
+    // Cases where the other rect doesn't intersect with any corners.
+    EXPECT_TRUE(roundedRect.contains(layoutRect(-5, -5, 10, 10)));
+    EXPECT_TRUE(roundedRect.contains(layoutRect(20, -10, 5, 5)));
+    EXPECT_TRUE(roundedRect.contains(layoutRect(-5, 5, 5, 5)));
+    EXPECT_TRUE(roundedRect.contains(layoutRect(0, 0, 10, 10)));
+    EXPECT_TRUE(roundedRect.contains(layoutRect(0, -10, 10, 10)));
+
+    // Cases where the other rect intersects at one of the rounded rect's corners.
+    EXPECT_FALSE(roundedRect.contains(layoutRect(-9, -39, 20, 40)));
+    EXPECT_TRUE(roundedRect.contains(layoutRect(-1, -31, 20, 40)));
+    EXPECT_FALSE(roundedRect.contains(layoutRect(20, -30, 10, 10)));
+    EXPECT_TRUE(roundedRect.contains(layoutRect(10, -30, 11, 11)));
+    EXPECT_FALSE(roundedRect.contains(layoutRect(15, 25, 14, 14)));
+    EXPECT_TRUE(roundedRect.contains(layoutRect(15, 25, 3, 3)));
+    EXPECT_TRUE(roundedRect.contains(layoutRect(-10, 20, 10, 20)));
+
+    // The other rect is equal to the rounded rect's bounds.
+    EXPECT_FALSE(roundedRect.contains(roundedRectBounds));
+    RoundedRect nonRoundedRect(roundedRectBounds, { 0, 0 }, { 0, 0 }, { 0, 0 }, { 0, 0 });
+    EXPECT_TRUE(nonRoundedRect.contains(roundedRectBounds));
+}
+
+}