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 644638ec3177b6d5268baaa3bdf729ad8c2c06ae..f73313a68354cb3e05a94e28ce1d17cf9ff28130 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 02765115a4a5a3d37f328ad73373e4e2b6ed9c4c..b33fa2a2d308e698cae55113935f89e005ea4690 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 60a451f9ef9a48f6390e3111934a96b97cecb60d..471d4b375b6cf6e50b4e0b503ca9625abcc66dee 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 3f2f7ebc8d43a23ca8ec4eb934becb2f1357523d..a2852ebc9aa81d4a0a65dde7b16af20758391263 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 3a52d74cfb98594ceb6e80098c0d720137e8df83..f0b326c5a9ce38918d4e7bcd4776bda9a58c21b2 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 2455d99c9f58fec8e7119d4eb7a8f50a8464a64e..f1db8e0a88c92878db4c0d6812cbcfb2fbbd36a7 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));
+}
+
+}