Bug 71673 - Zooming in SVGs in <object> is flakey
Summary: Zooming in SVGs in <object> is flakey
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks: 63186 71226
  Show dependency treegraph
 
Reported: 2011-11-07 05:51 PST by Nikolas Zimmermann
Modified: 2012-04-06 11:56 PDT (History)
6 users (show)

See Also:


Attachments
Patch (28.86 KB, patch)
2011-11-10 12:05 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v2 (28.86 KB, patch)
2011-11-10 14:01 PST, Nikolas Zimmermann
zherczeg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2011-11-07 05:51:57 PST
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.
Comment 1 Nikolas Zimmermann 2011-11-10 12:05:10 PST
Created attachment 114535 [details]
Patch

Finally found the culprit: overflow support. See patch for details.
Comment 2 Julien Chaffraix 2011-11-10 13:32:54 PST
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?
Comment 3 Nikolas Zimmermann 2011-11-10 13:51:52 PST
(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.
Comment 4 Nikolas Zimmermann 2011-11-10 14:01:17 PST
Created attachment 114562 [details]
Patch v2

Fix layout test failures, due the typo Julien discovered.
Comment 5 Nikolas Zimmermann 2011-11-10 14:02:01 PST
CC'ing Dirk & Zoltan & Rob for review, could any of you have a look?
Comment 6 Zoltan Herczeg 2011-11-11 01:16:35 PST
Comment on attachment 114562 [details]
Patch v2

Nice job. I like the long description about the issue.
Comment 7 Nikolas Zimmermann 2011-11-11 01:55:07 PST
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!
Comment 8 Stephen Chenney 2012-04-06 11:56:06 PDT
Committed r113470: <http://trac.webkit.org/changeset/113470>