Bug 71673 – Zooming in SVGs in <object> is flakey

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Nikolas Zimmermann]

Zooming svg/zoom/page/zoom-img-preserveAspectRatio-support-1.html manually shows size negotiation problems with the <object>.
Reloading the image at the desired zoom level makes scrollbars go away.

[Nikolas Zimmermann]

Created attachment 114535 [details]
Patch

Finally found the culprit: overflow support. See patch for details.

[Julien Chaffraix]

Comment on attachment 114535 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114535&action=review

Just commenting on one obvious mistake and a question. I don't feel like I know SVG + scrolling enough to review this patch in depth.

> Source/WebCore/page/FrameView.cpp:544
> +    EOverflow overflowY = o->style()->overflowX();

It should be overflowY here! I wonder if any test will fail with this change. If it is not covered, it would be needed to add some testing as part of this patch.

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:145
> +    Frame* frame = node()->document()->frame();

Isn't it possible for node()->document() to be NULL here?

[Nikolas Zimmermann]

(In reply to comment #2)
> (From update of attachment 114535 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114535&action=review
> 
> Just commenting on one obvious mistake and a question. I don't feel like I know SVG + scrolling enough to review this patch in depth.
> 
> > Source/WebCore/page/FrameView.cpp:544
> > +    EOverflow overflowY = o->style()->overflowX();
> 
> It should be overflowY here! I wonder if any test will fail with this change. If it is not covered, it would be needed to add some testing as part of this patch.
Good catch, but no worries for SVG overflowX is always equal to overflowY.
It will actually report errors in fast/ with this patch. Unfortunately cr-linux EWS is hosed, so we won't see it.
I'm going to upload a patch that passes all non-SVG tests as well :-)
> 
> > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:145
> > +    Frame* frame = node()->document()->frame();
> 
> Isn't it possible for node()->document() to be NULL here?
Nope, document() is never 0.

[Nikolas Zimmermann]

Created attachment 114562 [details]
Patch v2

Fix layout test failures, due the typo Julien discovered.

[Nikolas Zimmermann]

CC'ing Dirk & Zoltan & Rob for review, could any of you have a look?

[Zoltan Herczeg]

Comment on attachment 114562 [details]
Patch v2

Nice job. I like the long description about the issue.

[Nikolas Zimmermann]

Landed in r99937. This may need rebaselines, if so I'm going to take care. Yay, finally all known flakiness with SVG-as-image/object is gone!

[Stephen Chenney]

Committed r113470: <http://trac.webkit.org/changeset/113470>