2011-11-11 Nikolas Zimmermann <nzimmermann@rim.com>
authorzimmermann@webkit.org <zimmermann@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 11 Nov 2011 09:52:29 +0000 (09:52 +0000)
committerzimmermann@webkit.org <zimmermann@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 11 Nov 2011 09:52:29 +0000 (09:52 +0000)
        Zooming in SVGs in <object> is flakey
        https://bugs.webkit.org/show_bug.cgi?id=71673

        Reviewed by Zoltan Herczeg.

        Add another minimal testcase using <object> embedding SVGs + zooming.

        * platform/qt/Skipped: Unskip zoom-img-preserveAspectRatio-support-1.html.
        * platform/mac/svg/zoom/page/zoom-svg-as-object-expected.png: Added.
        * platform/mac/svg/zoom/page/zoom-svg-as-object-expected.txt: Added.
        * svg/zoom/page/resources/zoom-svg-as-object.svg: Added.
        * svg/zoom/page/zoom-svg-as-object.html: Added.

2011-11-11  Nikolas Zimmermann  <nzimmermann@rim.com>

        Zooming in SVGs in <object> is flakey
        https://bugs.webkit.org/show_bug.cgi?id=71673

        Reviewed by Zoltan Herczeg.

        It turns out zooming in SVGs in <object> wasn't flakey in Safari at all, only in DRT/Mac. In Safari it failed 100% reproducable.
        Scrollbars would always appear when zooming in a HTML document, containing an embedded SVG by <object>/<embed>/<iframe>, even
        though the content would visually fit perfectly, also it zoomed properly. Reloading would make the scrollbars disappear again.

        If scrollbars should be created for a FrameView or not, is determined by ScrollView::updateScrollbars(), by comparing the
        visible size of the scroll view against the contents size. The contents size is propagated to the ScrollView, by
        FrameView::adjustViewSize(), which is called during FrameView::layout(). The size thats propagated is RenderView::documentRect().
        RenderView::documentRect() returns a writing-mode aware layoutOverflowRect(), computed by RenderBox::layoutOverflowRectForPropagation.

        If overflow is "visible", layoutOverflowRect() will return a union of the borderBoxRect() and the layoutOverflowRect(), which
        may exceed the visible size of the RenderView. For standalone SVG documents, the default value for the outermost <svg> renderer is
        "visible". When embedding SVGs through <object>s into a host document, the same code path is taken, and RenderView::documentRect()
        of the embedded SVG document will always return a union of the borderBoxREct() and the layoutOverflowRect().

        If that happens while zooming in a HTML document containing a SVG by <object>, scrollbars are created.
        By ensuring that overflow is treated as hidden, which is what Opera does (and makes sense!) this can't happen.

        The fix is to treat embedded SVGs as they would carry overflow="hidden" on the outermost <svg> renderer. That also makes
        sense as the embedded SVG cant paint outside an external "frame rect" thats given by the FrameView, effectively rendering
        as overflow "hidden" already.

        The fix is realized, by altering the overflow x/y values that are used in FrameView::applyOverflowToViewport(). Previously
        we never called that method for SVG, which was fine. Now we always call applyOverflowToViewport(), but only do something
        if the FrameView of the SVG is embedded in another document. If so, we force overflow to hidden.

        That fixes all zooming+<object> related flakiness seen on the chromium bots, most noticeable, without other side effects.
        All svg/overflow tests, still work as expected.

        Test: svg/zoom/page/zoom-svg-as-object.html

        * page/Frame.cpp:
        (WebCore::Frame::setPageAndTextZoomFactors): Remove unnecessary setNeedsLayout() call in SVG builds.
        * page/FrameView.cpp:
        (WebCore::FrameView::applyOverflowToViewport): Always enforce overflow=hidden, when embedding SVGs through object/embed/iframe.
                                                       Otherwise scrollbars will appear, even though the contents would fit without them.
        (WebCore::FrameView::calculateScrollbarModesForLayout): Always call applyOverflowToViewport, even for RenderSVGRoot. It only has
                                                                an effect though, when the FrameView of the SVG is embedded through <object>/etc.
        (WebCore::FrameView::layout): Remove unnecessary setChildNeedsLayout() call.
        * rendering/svg/RenderSVGRoot.cpp:
        (WebCore::RenderSVGRoot::isEmbeddedThroughFrameContainingSVGDocument): Fix this function, its meaning was reversed before!
        (WebCore::RenderSVGRoot::computeReplacedLogicalWidth): Fix logical error, by negating the result of isEmbeddedThroughFrameContainingSVGDocument.
        (WebCore::RenderSVGRoot::computeReplacedLogicalHeight): Ditto.
        * rendering/svg/RenderSVGRoot.h: Expose isEmbeddedThroughFrameContainingSVGDocument.

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

LayoutTests/ChangeLog
LayoutTests/platform/mac/svg/zoom/page/zoom-svg-as-object-expected.png [new file with mode: 0644]
LayoutTests/platform/mac/svg/zoom/page/zoom-svg-as-object-expected.txt [new file with mode: 0644]
LayoutTests/platform/qt/Skipped
LayoutTests/svg/zoom/page/resources/zoom-svg-as-object.svg [new file with mode: 0644]
LayoutTests/svg/zoom/page/zoom-svg-as-object.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/Frame.cpp
Source/WebCore/page/FrameView.cpp
Source/WebCore/rendering/svg/RenderSVGRoot.cpp
Source/WebCore/rendering/svg/RenderSVGRoot.h

index 5dde0f7..4791a91 100644 (file)
@@ -1,3 +1,18 @@
+2011-11-11  Nikolas Zimmermann  <nzimmermann@rim.com>
+
+        Zooming in SVGs in <object> is flakey
+        https://bugs.webkit.org/show_bug.cgi?id=71673
+
+        Reviewed by Zoltan Herczeg.
+
+        Add another minimal testcase using <object> embedding SVGs + zooming.
+
+        * platform/qt/Skipped: Unskip zoom-img-preserveAspectRatio-support-1.html.
+        * platform/mac/svg/zoom/page/zoom-svg-as-object-expected.png: Added.
+        * platform/mac/svg/zoom/page/zoom-svg-as-object-expected.txt: Added.
+        * svg/zoom/page/resources/zoom-svg-as-object.svg: Added.
+        * svg/zoom/page/zoom-svg-as-object.html: Added.
+
 2011-11-10  Yuta Kitamura  <yutak@chromium.org>
 
         [Chromium] Unreviewed, add Linux baselines I forgot to commit in the last rebaseline.
diff --git a/LayoutTests/platform/mac/svg/zoom/page/zoom-svg-as-object-expected.png b/LayoutTests/platform/mac/svg/zoom/page/zoom-svg-as-object-expected.png
new file mode 100644 (file)
index 0000000..86c284a
Binary files /dev/null and b/LayoutTests/platform/mac/svg/zoom/page/zoom-svg-as-object-expected.png differ
diff --git a/LayoutTests/platform/mac/svg/zoom/page/zoom-svg-as-object-expected.txt b/LayoutTests/platform/mac/svg/zoom/page/zoom-svg-as-object-expected.txt
new file mode 100644 (file)
index 0000000..2bcdbc8
--- /dev/null
@@ -0,0 +1,11 @@
+layer at (0,0) size 800x600
+  RenderView at (0,0) size 800x600
+layer at (0,0) size 800x600
+  RenderBlock {HTML} at (0,0) size 800x600
+    RenderBody {BODY} at (0,0) size 666x500
+      RenderEmbeddedObject {OBJECT} at (0,0) size 400x300
+        layer at (0,0) size 400x300
+          RenderView at (0,0) size 400x300
+        layer at (0,0) size 400x300
+          RenderSVGRoot {svg} at (0,0) size 400x300
+            RenderSVGPath {rect} at (0,0) size 400x300 [stroke={[type=SOLID] [color=#000000]}] [x=1.00] [y=1.00] [width=478.00] [height=358.00]
index f2832d5..451864a 100644 (file)
@@ -2454,7 +2454,6 @@ inspector/extensions/extensions-console.html
 
 # [Qt] 4 new tests fail introduced in r98852
 # https://bugs.webkit.org/show_bug.cgi?id=71253
-svg/zoom/page/zoom-img-preserveAspectRatio-support-1.html
 fast/backgrounds/size/contain-and-cover-zoomed.html
 
 # REGRESSION(r99195)
diff --git a/LayoutTests/svg/zoom/page/resources/zoom-svg-as-object.svg b/LayoutTests/svg/zoom/page/resources/zoom-svg-as-object.svg
new file mode 100644 (file)
index 0000000..74968a9
--- /dev/null
@@ -0,0 +1,5 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1 Tiny//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11-tiny.dtd">
+<svg xmlns="http://www.w3.org/2000/svg"  width="480" height="360">
+<rect id="test-frame" x="1" y="1" width="478" height="358" fill="none" stroke="#000000"/>
+</svg>
diff --git a/LayoutTests/svg/zoom/page/zoom-svg-as-object.html b/LayoutTests/svg/zoom/page/zoom-svg-as-object.html
new file mode 100644 (file)
index 0000000..849fa3a
--- /dev/null
@@ -0,0 +1,23 @@
+<html>
+<head>
+<style>
+body {
+  margin: 0px;
+  width: 800px;
+  height: 600px;
+}
+
+object {
+  width: 480px;
+  height: 360px;
+}
+</style>
+</head>
+<body>
+<object data="resources/zoom-svg-as-object.svg" type="image/svg+xml"/>
+</body>
+
+<script>var zoomCount = 1; window.shouldZoomOut = true;</script>
+<script src="../resources/testPageZoom.js"></script>
+
+</html>
index 9fbcc61..95a9bab 100644 (file)
@@ -1,5 +1,56 @@
 2011-11-11  Nikolas Zimmermann  <nzimmermann@rim.com>
 
+        Zooming in SVGs in <object> is flakey
+        https://bugs.webkit.org/show_bug.cgi?id=71673
+
+        Reviewed by Zoltan Herczeg.
+
+        It turns out zooming in SVGs in <object> wasn't flakey in Safari at all, only in DRT/Mac. In Safari it failed 100% reproducable.
+        Scrollbars would always appear when zooming in a HTML document, containing an embedded SVG by <object>/<embed>/<iframe>, even
+        though the content would visually fit perfectly, also it zoomed properly. Reloading would make the scrollbars disappear again.
+
+        If scrollbars should be created for a FrameView or not, is determined by ScrollView::updateScrollbars(), by comparing the 
+        visible size of the scroll view against the contents size. The contents size is propagated to the ScrollView, by
+        FrameView::adjustViewSize(), which is called during FrameView::layout(). The size thats propagated is RenderView::documentRect().
+        RenderView::documentRect() returns a writing-mode aware layoutOverflowRect(), computed by RenderBox::layoutOverflowRectForPropagation.
+
+        If overflow is "visible", layoutOverflowRect() will return a union of the borderBoxRect() and the layoutOverflowRect(), which
+        may exceed the visible size of the RenderView. For standalone SVG documents, the default value for the outermost <svg> renderer is
+        "visible". When embedding SVGs through <object>s into a host document, the same code path is taken, and RenderView::documentRect()
+        of the embedded SVG document will always return a union of the borderBoxREct() and the layoutOverflowRect().
+
+        If that happens while zooming in a HTML document containing a SVG by <object>, scrollbars are created.
+        By ensuring that overflow is treated as hidden, which is what Opera does (and makes sense!) this can't happen.
+
+        The fix is to treat embedded SVGs as they would carry overflow="hidden" on the outermost <svg> renderer. That also makes
+        sense as the embedded SVG cant paint outside an external "frame rect" thats given by the FrameView, effectively rendering
+        as overflow "hidden" already.
+
+        The fix is realized, by altering the overflow x/y values that are used in FrameView::applyOverflowToViewport(). Previously
+        we never called that method for SVG, which was fine. Now we always call applyOverflowToViewport(), but only do something
+        if the FrameView of the SVG is embedded in another document. If so, we force overflow to hidden.
+
+        That fixes all zooming+<object> related flakiness seen on the chromium bots, most noticeable, without other side effects.
+        All svg/overflow tests, still work as expected.
+
+        Test: svg/zoom/page/zoom-svg-as-object.html
+
+        * page/Frame.cpp:
+        (WebCore::Frame::setPageAndTextZoomFactors): Remove unnecessary setNeedsLayout() call in SVG builds.
+        * page/FrameView.cpp:
+        (WebCore::FrameView::applyOverflowToViewport): Always enforce overflow=hidden, when embedding SVGs through object/embed/iframe.
+                                                       Otherwise scrollbars will appear, even though the contents would fit without them.
+        (WebCore::FrameView::calculateScrollbarModesForLayout): Always call applyOverflowToViewport, even for RenderSVGRoot. It only has
+                                                                an effect though, when the FrameView of the SVG is embedded through <object>/etc.
+        (WebCore::FrameView::layout): Remove unnecessary setChildNeedsLayout() call.
+        * rendering/svg/RenderSVGRoot.cpp:
+        (WebCore::RenderSVGRoot::isEmbeddedThroughFrameContainingSVGDocument): Fix this function, its meaning was reversed before!
+        (WebCore::RenderSVGRoot::computeReplacedLogicalWidth): Fix logical error, by negating the result of isEmbeddedThroughFrameContainingSVGDocument.
+        (WebCore::RenderSVGRoot::computeReplacedLogicalHeight): Ditto.
+        * rendering/svg/RenderSVGRoot.h: Expose isEmbeddedThroughFrameContainingSVGDocument.
+
+2011-11-11  Nikolas Zimmermann  <nzimmermann@rim.com>
+
         Not reviewed. Unbreak my 32bit SL build.
 
         * rendering/RenderLayer.cpp:
index a88f185..6270310 100644 (file)
@@ -999,8 +999,6 @@ void Frame::setPageAndTextZoomFactors(float pageZoomFactor, float textZoomFactor
     if (document->isSVGDocument()) {
         if (!static_cast<SVGDocument*>(document)->zoomAndPanEnabled())
             return;
-        if (document->renderer())
-            document->renderer()->setNeedsLayout(true);
     }
 #endif
 
index 35770d0..96131ab 100644 (file)
@@ -540,7 +540,20 @@ void FrameView::applyOverflowToViewport(RenderObject* o, ScrollbarMode& hMode, S
 
     bool overrideHidden = m_frame->page() && m_frame->page()->mainFrame() == m_frame && m_frame->frameScaleFactor() > 1;
 
-    switch (o->style()->overflowX()) {
+    EOverflow overflowX = o->style()->overflowX();
+    EOverflow overflowY = o->style()->overflowY();
+
+#if ENABLE(SVG)
+    if (o->isSVGRoot()) {
+        // overflow is ignored in stand-alone SVG documents.
+        if (!toRenderSVGRoot(o)->isEmbeddedThroughFrameContainingSVGDocument())
+            return;
+        overflowX = OHIDDEN;
+        overflowY = OHIDDEN;
+    }
+#endif
+
+    switch (overflowX) {
         case OHIDDEN:
             if (overrideHidden)
                 hMode = ScrollbarAuto;
@@ -558,7 +571,7 @@ void FrameView::applyOverflowToViewport(RenderObject* o, ScrollbarMode& hMode, S
             ;
     }
     
-     switch (o->style()->overflowY()) {
+     switch (overflowY) {
         case OHIDDEN:
             if (overrideHidden)
                 vMode = ScrollbarAuto;
@@ -613,14 +626,8 @@ void FrameView::calculateScrollbarModesForLayout(ScrollbarMode& hMode, Scrollbar
                 RenderObject* o = rootRenderer->style()->overflowX() == OVISIBLE && document->documentElement()->hasTagName(htmlTag) ? body->renderer() : rootRenderer;
                 applyOverflowToViewport(o, hMode, vMode);
             }
-        } else if (rootRenderer) {
-#if ENABLE(SVG)
-            if (!documentElement->isSVGElement())
-                applyOverflowToViewport(rootRenderer, hMode, vMode);
-#else
+        } else if (rootRenderer)
             applyOverflowToViewport(rootRenderer, hMode, vMode);
-#endif
-        }
     }    
 }
 
@@ -1006,8 +1013,6 @@ void FrameView::layout(bool allowSubtree)
 
     if (!m_layoutRoot) {
         Document* document = m_frame->document();
-        Node* documentElement = document->documentElement();
-        RenderObject* rootRenderer = documentElement ? documentElement->renderer() : 0;
         Node* body = document->body();
         if (body && body->renderer()) {
             if (body->hasTagName(framesetTag) && m_frame->settings() && !m_frame->settings()->frameFlatteningEnabled()) {
@@ -1016,13 +1021,6 @@ void FrameView::layout(bool allowSubtree)
                 if (!m_firstLayout && m_size.height() != layoutHeight() && body->renderer()->enclosingBox()->stretchesToViewport())
                     body->renderer()->setChildNeedsLayout(true);
             }
-        } else if (rootRenderer) {
-#if ENABLE(SVG)
-            if (documentElement->isSVGElement()) {
-                if (!m_firstLayout && (m_size.width() != layoutWidth() || m_size.height() != layoutHeight()))
-                    rootRenderer->setChildNeedsLayout(true);
-            }
-#endif
         }
         
 #ifdef INSTRUMENT_LAYOUT_SCHEDULING
index 1195af4..5eb3c98 100644 (file)
@@ -137,13 +137,20 @@ bool RenderSVGRoot::isEmbeddedThroughSVGImage() const
     return true;
 }
 
-static inline bool isEmbeddedThroughFrameContainingSVGDocument(const Frame* frame)
+bool RenderSVGRoot::isEmbeddedThroughFrameContainingSVGDocument() const
 {
-    ASSERT(frame);
-    ASSERT(frame->document());
+    if (!node())
+        return false;
+
+    Frame* frame = node()->document()->frame();
+    if (!frame)
+        return false;
+
     // If our frame has an owner renderer, we're embedded through eg. object/embed/iframe,
     // but we only negotiate if we're in an SVG document.
-    return !frame->ownerRenderer() || !frame->document()->isSVGDocument();
+    if (!frame->ownerRenderer())
+        return false;
+    return frame->document()->isSVGDocument();
 }
 
 LayoutUnit RenderSVGRoot::computeReplacedLogicalWidth(bool includeMaxWidth) const
@@ -158,7 +165,7 @@ LayoutUnit RenderSVGRoot::computeReplacedLogicalWidth(bool includeMaxWidth) cons
     if (!frame)
         return replacedWidth;
 
-    if (isEmbeddedThroughFrameContainingSVGDocument(frame))
+    if (!isEmbeddedThroughFrameContainingSVGDocument())
         return replacedWidth;
 
     RenderPart* ownerRenderer = frame->ownerRenderer();
@@ -203,13 +210,14 @@ LayoutUnit RenderSVGRoot::computeReplacedLogicalHeight() const
     if (!frame)
         return replacedHeight;
 
-    if (isEmbeddedThroughFrameContainingSVGDocument(frame))
+    if (!isEmbeddedThroughFrameContainingSVGDocument())
         return replacedHeight;
 
     RenderPart* ownerRenderer = frame->ownerRenderer();
+    ASSERT(ownerRenderer);
+
     RenderStyle* ownerRendererStyle = ownerRenderer->style();
     ASSERT(ownerRendererStyle);
-    ASSERT(frame->contentRenderer());
 
     Length ownerHeight = ownerRendererStyle->height();
     if (ownerHeight.isAuto())
index 2e41354..b9ba0ef 100644 (file)
@@ -40,6 +40,7 @@ public:
     virtual ~RenderSVGRoot();
 
     bool isEmbeddedThroughSVGImage() const;
+    bool isEmbeddedThroughFrameContainingSVGDocument() const;
 
     virtual void computeIntrinsicRatioInformation(FloatSize& intrinsicRatio, bool& isPercentageIntrinsicSize) const;
     const RenderObjectChildList* children() const { return &m_children; }