Skip to content

Commit

Permalink
2011-11-11 Nikolas Zimmermann <nzimmermann@rim.com>
Browse files Browse the repository at this point in the history 
        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.


Canonical link: https://commits.webkit.org/88497@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@99937 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
Nikolas Zimmermann committed Nov 11, 2011
1 parent 688e36e commit 4328fdf
Show file tree
Hide file tree
Showing 11 changed files with 137 additions and 28 deletions.
15 changes: 15 additions & 0 deletions LayoutTests/ChangeLog
View 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.
Expand Down
Binary file added LayoutTests/platform/mac/svg/zoom/page/zoom-svg-as-object-expected.png
View file
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
11 changes: 11 additions & 0 deletions LayoutTests/platform/mac/svg/zoom/page/zoom-svg-as-object-expected.txt
View file
@@ -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]
1 change: 0 additions & 1 deletion LayoutTests/platform/qt/Skipped
View file
Expand Up @@ -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)
Expand Down
5 changes: 5 additions & 0 deletions LayoutTests/svg/zoom/page/resources/zoom-svg-as-object.svg
View file
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
23 changes: 23 additions & 0 deletions LayoutTests/svg/zoom/page/zoom-svg-as-object.html
View file
@@ -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>
51 changes: 51 additions & 0 deletions Source/WebCore/ChangeLog
View file
@@ -1,3 +1,54 @@
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.
Expand Down
2 changes: 0 additions & 2 deletions Source/WebCore/page/Frame.cpp
View file
Expand Up @@ -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

Expand Down
34 changes: 16 additions & 18 deletions Source/WebCore/page/FrameView.cpp
View file
Expand Up @@ -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;
Expand All @@ -558,7 +571,7 @@ void FrameView::applyOverflowToViewport(RenderObject* o, ScrollbarMode& hMode, S
;
}

switch (o->style()->overflowY()) {
switch (overflowY) {
case OHIDDEN:
if (overrideHidden)
vMode = ScrollbarAuto;
Expand Down Expand Up @@ -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
}
}
}

Expand Down Expand Up @@ -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()) {
Expand All @@ -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
Expand Down
22 changes: 15 additions & 7 deletions Source/WebCore/rendering/svg/RenderSVGRoot.cpp
View file
Expand Up @@ -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
Expand All @@ -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();
Expand Down Expand Up @@ -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())
Expand Down
1 change: 1 addition & 0 deletions Source/WebCore/rendering/svg/RenderSVGRoot.h
View file
Expand Up @@ -40,6 +40,7 @@ class RenderSVGRoot : public RenderBox {
virtual ~RenderSVGRoot();

bool isEmbeddedThroughSVGImage() const;
bool isEmbeddedThroughFrameContainingSVGDocument() const;

virtual void computeIntrinsicRatioInformation(FloatSize& intrinsicRatio, bool& isPercentageIntrinsicSize) const;
const RenderObjectChildList* children() const { return &m_children; }
Expand Down

0 comments on commit 4328fdf

Please sign in to comment.