getBBox() on a SVGPathElement with curves incorrectly includes control points
authortimothy_horton@apple.com <timothy_horton@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 7 Nov 2011 20:40:24 +0000 (20:40 +0000)
committertimothy_horton@apple.com <timothy_horton@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 7 Nov 2011 20:40:24 +0000 (20:40 +0000)
https://bugs.webkit.org/show_bug.cgi?id=53512
<rdar://problem/9861154>

Reviewed by Oliver Hunt.

Split Path::boundingRect() into two, adding Path::fastBoundingRect()
for a rough estimate of the bounding rect (always equal to or larger
than boundingRect()). fastBoundingRect() currently falls back to
boundingRect() for all ports besides CG, though in most cases
(on a port-by-port basis) the current implementation of boundingRect()
will need to become fastBoundingRect(), and a new, more accurate method will
be implemented for boundingRect().

All previous callers of boundingRect() are transitioned to using fastBoundingRect()
except SVGPathElement::getBBox, which wants an accurate bounding box.

The CoreGraphics implementation of Path::boundingRect() called
CGPathGetBoundingBox, which includes the path's control points in its
calculations. Snow Leopard added CGPathGetPathBoundingBox, which
finds the bounding box of only points within the path, and does not
include control points. On Snow Leopard and above, we now use the latter.

Test: svg/custom/getBBox-path.svg

* html/HTMLAreaElement.cpp:
* html/canvas/CanvasRenderingContext2D.cpp:
* platform/graphics/Path.cpp:
* platform/graphics/Path.h:
* platform/graphics/cg/GraphicsContextCG.cpp:
* platform/graphics/cg/PathCG.cpp:
(WebCore::Path::boundingRect):
* rendering/RenderObject.h:
* rendering/svg/RenderSVGPath.cpp:
* svg/SVGPathElement.cpp:
* svg/SVGPathElement.h:

Add a test that ensures that getBBox does not include control points in the rect it returns.

* platform/chromium/test_expectations.txt:
* svg/custom/getBBox-path-expected.txt: Added.
* svg/custom/getBBox-path.svg: Added.

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

15 files changed:
LayoutTests/ChangeLog
LayoutTests/platform/chromium/test_expectations.txt
LayoutTests/svg/custom/getBBox-path-expected.txt [new file with mode: 0644]
LayoutTests/svg/custom/getBBox-path.svg [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/html/HTMLAreaElement.cpp
Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp
Source/WebCore/platform/graphics/Path.cpp
Source/WebCore/platform/graphics/Path.h
Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp
Source/WebCore/platform/graphics/cg/PathCG.cpp
Source/WebCore/rendering/RenderObject.h
Source/WebCore/rendering/svg/RenderSVGPath.cpp
Source/WebCore/svg/SVGPathElement.cpp
Source/WebCore/svg/SVGPathElement.h

index cac62e9..c283eb1 100644 (file)
@@ -1,3 +1,17 @@
+2011-11-07  Tim Horton  <timothy_horton@apple.com>
+
+        getBBox() on a SVGPathElement with curves incorrectly includes control points
+        https://bugs.webkit.org/show_bug.cgi?id=53512
+        <rdar://problem/9861154>
+
+        Reviewed by Oliver Hunt.
+        
+        Add a test that ensures that getBBox does not include control points in the rect it returns.
+
+        * platform/chromium/test_expectations.txt:
+        * svg/custom/getBBox-path-expected.txt: Added.
+        * svg/custom/getBBox-path.svg: Added.
+
 2011-11-07  vsevik@chromium.org  <vsevik@chromium.org>
 
         Web Inspector: Suggest box should consume enter key pressed event.
index d093cfe..ee6dab6 100644 (file)
@@ -3899,3 +3899,6 @@ BUGWK59552 SNOWLEOPARD DEBUG : fast/frames/iframe-double-scale-contents.html = P
 BUGWK59552 SNOWLEOPARD DEBUG : fast/frames/set-parent-src-synchronously.html = PASS CRASH
 
 BUGWK71658 MAC DEBUG : fast/frames/sandboxed-iframe-navigation-targetlink.html = PASS TIMEOUT
+
+// Introduced due to BUGWK53512, fails under Skia right now
+BUGWK65939 : svg/custom/getBBox-path.svg = TEXT
diff --git a/LayoutTests/svg/custom/getBBox-path-expected.txt b/LayoutTests/svg/custom/getBBox-path-expected.txt
new file mode 100644 (file)
index 0000000..86db678
--- /dev/null
@@ -0,0 +1 @@
+100 37.5 PASS
diff --git a/LayoutTests/svg/custom/getBBox-path.svg b/LayoutTests/svg/custom/getBBox-path.svg
new file mode 100644 (file)
index 0000000..21dca93
--- /dev/null
@@ -0,0 +1,21 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<svg xmlns="http://www.w3.org/2000/svg" onload="init()">
+  <script type="text/javascript">
+  <![CDATA[
+    function init()
+    {
+        if (window.layoutTestController)
+            layoutTestController.dumpAsText();
+        var txt = document.getElementById("text");
+        size = document.getElementById("shape").getBBox();
+        var passState = "FAIL";
+        if(size.width == 100 && size.height == 37.5)
+            passState = "PASS";
+        var textNode = document.createTextNode(size.width + " " + size.height + " " + passState);
+        txt.appendChild(textNode);
+    }
+  ]]>
+  </script>
+  <path id="shape" d="M100,100 C125,150 175,150 200,100 z" />
+  <text id="text" x="50" y="50" />
+</svg>
index 7bdcccc..d34d2c6 100644 (file)
@@ -1,3 +1,42 @@
+2011-11-07  Tim Horton  <timothy_horton@apple.com>
+
+        getBBox() on a SVGPathElement with curves incorrectly includes control points
+        https://bugs.webkit.org/show_bug.cgi?id=53512
+        <rdar://problem/9861154>
+
+        Reviewed by Oliver Hunt.
+
+        Split Path::boundingRect() into two, adding Path::fastBoundingRect()
+        for a rough estimate of the bounding rect (always equal to or larger
+        than boundingRect()). fastBoundingRect() currently falls back to
+        boundingRect() for all ports besides CG, though in most cases
+        (on a port-by-port basis) the current implementation of boundingRect()
+        will need to become fastBoundingRect(), and a new, more accurate method will
+        be implemented for boundingRect().
+
+        All previous callers of boundingRect() are transitioned to using fastBoundingRect()
+        except SVGPathElement::getBBox, which wants an accurate bounding box.
+
+        The CoreGraphics implementation of Path::boundingRect() called
+        CGPathGetBoundingBox, which includes the path's control points in its
+        calculations. Snow Leopard added CGPathGetPathBoundingBox, which
+        finds the bounding box of only points within the path, and does not
+        include control points. On Snow Leopard and above, we now use the latter.
+
+        Test: svg/custom/getBBox-path.svg
+
+        * html/HTMLAreaElement.cpp:
+        * html/canvas/CanvasRenderingContext2D.cpp:
+        * platform/graphics/Path.cpp:
+        * platform/graphics/Path.h:
+        * platform/graphics/cg/GraphicsContextCG.cpp:
+        * platform/graphics/cg/PathCG.cpp:
+        (WebCore::Path::boundingRect):
+        * rendering/RenderObject.h:
+        * rendering/svg/RenderSVGPath.cpp:
+        * svg/SVGPathElement.cpp:
+        * svg/SVGPathElement.h:
+
 2011-11-07  Vsevolod Vlasov  <vsevik@chromium.org>
 
         Web Inspector: Suggest box should be open immediately if forced by Ctrl+Space.
index 7628323..6960a92 100644 (file)
@@ -120,7 +120,7 @@ Path HTMLAreaElement::computePath(RenderObject* obj) const
     
 LayoutRect HTMLAreaElement::computeRect(RenderObject* obj) const
 {
-    return enclosingLayoutRect(computePath(obj).boundingRect());
+    return enclosingLayoutRect(computePath(obj).fastBoundingRect());
 }
 
 Path HTMLAreaElement::getRegion(const LayoutSize& size) const
index 376b606..8477914 100644 (file)
@@ -771,7 +771,7 @@ void CanvasRenderingContext2D::closePath()
     if (m_path.isEmpty())
         return;
 
-    FloatRect boundRect = m_path.boundingRect();
+    FloatRect boundRect = m_path.fastBoundingRect();
     if (boundRect.width() || boundRect.height())
         m_path.closeSubpath();
 }
@@ -956,7 +956,7 @@ void CanvasRenderingContext2D::fill()
             didDrawEntireCanvas();
         } else {
             c->fillPath(m_path);
-            didDraw(m_path.boundingRect());
+            didDraw(m_path.fastBoundingRect());
         }
     }
 
@@ -974,7 +974,7 @@ void CanvasRenderingContext2D::stroke()
         return;
 
     if (!m_path.isEmpty()) {
-        FloatRect dirtyRect = m_path.boundingRect();
+        FloatRect dirtyRect = m_path.fastBoundingRect();
         // Fast approximation of the stroke's bounding rect.
         // This yields a slightly oversized rect but is very fast
         // compared to Path::strokeBoundingRect().
@@ -1580,7 +1580,7 @@ template<class T> IntRect CanvasRenderingContext2D::calculateCompositingBufferRe
     IntRect canvasRect(0, 0, canvas()->width(), canvas()->height());
     canvasRect = canvas()->baseTransform().mapRect(canvasRect);
     Path path = transformAreaToDevice(area);
-    IntRect bufferRect = enclosingIntRect(path.boundingRect());
+    IntRect bufferRect = enclosingIntRect(path.fastBoundingRect());
     IntPoint originalLocation = bufferRect.location();
     bufferRect.intersect(canvasRect);
     if (croppedOffset)
index ff0fd22..128d92a 100644 (file)
@@ -163,4 +163,11 @@ void Path::addBeziersForRoundedRect(const FloatRect& rect, const FloatSize& topL
     closeSubpath();
 }
 
+#if !USE(CG)
+FloatRect Path::fastBoundingRect() const
+{
+    return boundingRect();
+}
+#endif
+
 }
index f51c764..3d1e7b1 100644 (file)
@@ -105,7 +105,10 @@ namespace WebCore {
 
         bool contains(const FloatPoint&, WindRule rule = RULE_NONZERO) const;
         bool strokeContains(StrokeStyleApplier*, const FloatPoint&) const;
+        // fastBoundingRect() should equal or contain boundingRect(); boundingRect()
+        // should perfectly bound the points within the path.
         FloatRect boundingRect() const;
+        FloatRect fastBoundingRect() const;
         FloatRect strokeBoundingRect(StrokeStyleApplier* = 0) const;
         
         float length() const;
index 8eeb74c..ee87f53 100644 (file)
@@ -627,7 +627,7 @@ void GraphicsContext::fillPath(const Path& path)
 
     if (m_state.fillGradient) {
         if (hasShadow()) {
-            FloatRect rect = path.boundingRect();
+            FloatRect rect = path.fastBoundingRect();
             CGLayerRef layer = CGLayerCreateWithContext(context, CGSizeMake(rect.width(), rect.height()), 0);
             CGContextRef layerContext = CGLayerGetContext(layer);
 
@@ -682,7 +682,7 @@ void GraphicsContext::strokePath(const Path& path)
 
     if (m_state.strokeGradient) {
         if (hasShadow()) {
-            FloatRect rect = path.boundingRect();
+            FloatRect rect = path.fastBoundingRect();
             float lineWidth = strokeThickness();
             float doubleLineWidth = lineWidth * 2;
             float layerWidth = ceilf(rect.width() + doubleLineWidth);
index 514444b..b36c766 100644 (file)
@@ -124,7 +124,7 @@ static CGMutablePathRef copyCGPathClosingSubpaths(CGPathRef originalPath)
 
 bool Path::contains(const FloatPoint &point, WindRule rule) const
 {
-    if (!boundingRect().contains(point))
+    if (!fastBoundingRect().contains(point))
         return false;
 
     // CGPathContainsPoint returns false for non-closed paths, as a work-around, we copy and close the path first.  Radar 4758998 asks for a better CG API to use
@@ -163,6 +163,17 @@ void Path::translate(const FloatSize& size)
 
 FloatRect Path::boundingRect() const
 {
+    // CGPathGetBoundingBox includes the path's control points, CGPathGetPathBoundingBox
+    // does not, but only exists on 10.6 and above.
+#if !defined(BUILDING_ON_LEOPARD)
+    return CGPathGetPathBoundingBox(m_path);
+#else
+    return CGPathGetBoundingBox(m_path);
+#endif
+}
+
+FloatRect Path::fastBoundingRect() const
+{
     return CGPathGetBoundingBox(m_path);
 }
 
index 9b35207..58f0341 100644 (file)
@@ -421,7 +421,7 @@ public:
     virtual void setNeedsBoundariesUpdate();
 
     // Per SVG 1.1 objectBoundingBox ignores clipping, masking, filter effects, opacity and stroke-width.
-    // This is used for all computation of objectBoundingBox relative units and by SVGLocateable::getBBox().
+    // This is used for all computation of objectBoundingBox relative units and by SVGLocatable::getBBox().
     // NOTE: Markers are not specifically ignored here by SVG 1.1 spec, but we ignore them
     // since stroke-width is ignored (and marker size can depend on stroke-width).
     // objectBoundingBox is returned local coordinates.
index b7ad7a2..a7b0df7 100644 (file)
@@ -357,7 +357,7 @@ void RenderSVGPath::updateCachedBoundaries()
     }
 
     // Cache _unclipped_ fill bounding box, used for calculations in resources
-    m_fillBoundingBox = m_path.boundingRect();
+    m_fillBoundingBox = m_path.fastBoundingRect();
 
     // Spec(11.4): Any zero length subpath shall not be stroked if the ‘stroke-linecap’ property has a value of butt
     // but shall be stroked if the ‘stroke-linecap’ property has a value of round or square
index 4ae6613..7b0a729 100644 (file)
@@ -76,6 +76,7 @@ inline SVGPathElement::SVGPathElement(const QualifiedName& tagName, Document* do
     : SVGStyledTransformableElement(tagName, document)
     , m_pathByteStream(SVGPathByteStream::create())
     , m_pathSegList(PathSegUnalteredRole)
+    , m_cachedBBoxRectIsValid(false)
 {
     ASSERT(hasTagName(SVGNames::pathTag));
     registerAnimatedPropertiesForSVGPathElement();
@@ -266,6 +267,7 @@ void SVGPathElement::svgAttributeChanged(const QualifiedName& attrName)
             SVGPathParserFactory* factory = SVGPathParserFactory::self();
             factory->buildSVGPathSegListFromByteStream(m_pathByteStream.get(), this, newList, UnalteredParsing);
             m_pathSegList.value = newList;
+            m_cachedBBoxRectIsValid = false;
         }
 
         if (renderer)
@@ -350,6 +352,8 @@ void SVGPathElement::pathSegListChanged(SVGPathSegRole role)
     }
 
     invalidateSVGAttributes();
+    
+    m_cachedBBoxRectIsValid = false;
 
     RenderSVGPath* renderer = static_cast<RenderSVGPath*>(this->renderer());
     if (!renderer)
@@ -359,6 +363,25 @@ void SVGPathElement::pathSegListChanged(SVGPathSegRole role)
     RenderSVGResource::markForLayoutAndParentResourceInvalidation(renderer);
 }
 
+FloatRect SVGPathElement::getBBox(StyleUpdateStrategy styleUpdateStrategy)
+{
+    if (styleUpdateStrategy == AllowStyleUpdate)
+        this->document()->updateLayoutIgnorePendingStylesheets();
+
+    RenderSVGPath* renderer = static_cast<RenderSVGPath*>(this->renderer());
+
+    // FIXME: Eventually we should support getBBox for detached elements.
+    if (!renderer)
+        return FloatRect();
+
+    if (!m_cachedBBoxRectIsValid) {
+        m_cachedBBoxRect = renderer->path().boundingRect();
+        m_cachedBBoxRectIsValid = true;
+    }
+    
+    return m_cachedBBoxRect;
+}
+
 }
 
 #endif // ENABLE(SVG)
index 1d37d0e..4555564 100644 (file)
@@ -97,6 +97,8 @@ public:
 
     static const SVGPropertyInfo* dPropertyInfo();
 
+    virtual FloatRect getBBox(StyleUpdateStrategy = AllowStyleUpdate);
+
 private:
     SVGPathElement(const QualifiedName&, Document*);
 
@@ -126,6 +128,8 @@ private:
     OwnPtr<SVGPathByteStream> m_pathByteStream;
     mutable SVGSynchronizableAnimatedProperty<SVGPathSegList> m_pathSegList;
     RefPtr<SVGAnimatedPathSegListPropertyTearOff> m_animatablePathSegList;
+    FloatRect m_cachedBBoxRect;
+    bool m_cachedBBoxRectIsValid;                       
 };
 
 } // namespace WebCore