Bug 64823

Summary:

Product:

WebKit

Reporter:

Jamie Starke <jrstarke>

Component:

SVG

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dglazkov, leviw, leviw, michaelthomas, thorton, webkit.review.bot, zimmermann

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

Unspecified

URL:

http://code.google.com/p/chromium/issues/detail?id=88742

Attachments:

Description	Flags
SVG Circle Example	none
Shot of how the example displays in different browsers.	none
Simpler sample (no JS)	none
Patch	none
Patch	none
Patch for landing	none
Patch	none

Jamie Starke

Reported 2011-07-19 14:03:17 PDT

If you generate code for an iframe and put svg in it. The size of the svg is wrong. I have attached an example where a Blue circle 100px X 100 px should be displayed. On Chrome 13.0.782.55 (using the WebKit chromium branch @89960) works as expected, where as Chrome 14.0.825.0 (which uses WebKit trunk @91062) displays the top 100 pixels of a circle that would fill the window. On Safari this displays a blank page. This functionality works properly on IE9

Attachments
SVG Circle Example (953 bytes, text/html) 2011-07-19 14:04 PDT, Jamie Starke	no flags	Details
Shot of how the example displays in different browsers. (221.42 KB, image/png) 2011-07-19 14:07 PDT, Jamie Starke	no flags	Details
Simpler sample (no JS) (1.62 KB, application/zip) 2011-08-05 11:26 PDT, Tim Horton	no flags	Details
Patch (24.10 KB, patch) 2011-10-18 17:26 PDT, Levi Weintraub	no flags	Details Formatted Diff Diff
Patch (24.01 KB, patch) 2011-10-18 17:55 PDT, Levi Weintraub	no flags	Details Formatted Diff Diff
Patch for landing (23.64 KB, patch) 2011-10-19 13:37 PDT, Levi Weintraub	no flags	Details Formatted Diff Diff
Patch (23.96 KB, patch) 2011-10-20 12:17 PDT, Levi Weintraub	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Jamie Starke

Comment 1 2011-07-19 14:04:24 PDT

Created attachment 101381 [details] SVG Circle Example Example of SVG creating a circle

Jamie Starke

Comment 2 2011-07-19 14:07:49 PDT

Created attachment 101382 [details] Shot of how the example displays in different browsers.

Tim Horton

Comment 3 2011-08-05 11:26:34 PDT

Created attachment 103089 [details] Simpler sample (no JS)

Tim Horton

Comment 4 2011-08-05 11:39:52 PDT

Looks like a regression from http://trac.webkit.org/changeset/87526

Tim Horton

Comment 5 2011-08-05 12:04:02 PDT

It looks like there's a lot going on in this code in: https://bugs.webkit.org/show_bug.cgi?id=64974 and https://bugs.webkit.org/show_bug.cgi?id=47156 so we'll have to see what happens after that.

Michael Thomas

Comment 6 2011-10-10 14:06:37 PDT

Are there any known workarounds for this issue?

Dimitri Glazkov (Google)

Comment 7 2011-10-13 14:43:14 PDT

Niko, are you working on this?

Nikolas Zimmermann

Comment 8 2011-10-14 11:29:06 PDT

(In reply to comment #7) > Niko, are you working on this? This is an area I've recently touched. I'll look at it over the weekend. It's not related to my current SVGImage work, and _should_ already work. If it doesn't there's clearly a bug with the existing size negotiation logic.

Nikolas Zimmermann

Comment 9 2011-10-14 11:32:55 PDT

(In reply to comment #6) > Are there any known workarounds for this issue? A workaround is to use <svg width="100" height="100"> instead of style="width:100px; height:100px" if that helps you in the meanwhile.

Dimitri Glazkov (Google)

Comment 10 2011-10-17 10:07:16 PDT

(In reply to comment #9) > (In reply to comment #6) > > Are there any known workarounds for this issue? > A workaround is to use <svg width="100" height="100"> instead of style="width:100px; height:100px" if that helps you in the meanwhile. This workaround doesn't work: http://jonnew.com/js/svgIframe.html Have you had a chance to look into this? The bug is blocking a good bunch of people, preventing them from delivering excellent user experience to the world.

Nikolas Zimmermann

Comment 11 2011-10-17 12:33:32 PDT

(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #6) > > > Are there any known workarounds for this issue? > > A workaround is to use <svg width="100" height="100"> instead of style="width:100px; height:100px" if that helps you in the meanwhile. This was about the attached testcase, maybe I was wrong. > > This workaround doesn't work: http://jonnew.com/js/svgIframe.html > > Have you had a chance to look into this? The bug is blocking a good bunch of people, preventing them from delivering excellent user experience to the world. Yes, just looked at it and the problem is pretty obvious: LayoutUnit RenderSVGRoot::computeReplacedLogicalWidth(bool includeMaxWidth) const { LayoutUnit replacedWidth = RenderBox::computeReplacedLogicalWidth(includeMaxWidth); Frame* frame = node() && node()->document() ? node()->document()->frame() : 0; if (!frame) return computeIntrinsicWidth(replacedWidth); // If our frame has an owner renderer, we're embedded through eg. object/embed. RenderPart* ownerRenderer = frame->ownerRenderer(); if (!ownerRenderer) return computeIntrinsicWidth(replacedWidth); ... The size negotiation in RenderSVGRoot should only be done if we're embedded standalone eg. <iframe src="foo.svg"> not for a <svg> renderer inside an embedded html! I don't have a chance to fix it today, but maybe within the next days - or someone else can look into it using that information, it's easy to fix, we just have to differentiate if we're standalone in the embedded doc or not.

Levi Weintraub

Comment 12 2011-10-18 16:50:51 PDT

I've got a working patch I'm testing. I'll take care of this.

Levi Weintraub

Comment 13 2011-10-18 17:26:59 PDT

Created attachment 111536 [details] Patch

Gyuyoung Kim

Comment 14 2011-10-18 17:50:55 PDT

Comment on attachment 111536 [details] Patch Attachment 111536 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10140170

Levi Weintraub

Comment 15 2011-10-18 17:55:11 PDT

Created attachment 111542 [details] Patch

WebKit Review Bot

Comment 16 2011-10-18 19:13:58 PDT

Comment on attachment 111542 [details] Patch Attachment 111542 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10141206 New failing tests: svg/as-object/svg-embedded-in-html-in-iframe.html

Nikolas Zimmermann

Comment 17 2011-10-19 01:39:23 PDT

Comment on attachment 111542 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111542&action=review Great patch Levi, r=me. Please fix following issues before landing: > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:133 > +static bool isEmbeddedFrameContainingSVGDocument(const Frame* frame, RenderPart* ownerRenderer) I'd name this "isEmbeddedThroughFrameContainingSVGDocument" - bug 47156 adds a similar method "isEmbeddedThroughSVGImage" and i'd like to have it consistent :-) > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:135 > + // If our frame has an owner renderer, we're embedded through eg. object/embed/iframe, I'd ASSERT(frame) here to save the reader from looking at the call-site whether this can be null. > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:136 > + // but we only negotiate if we're in an SVG document Trailing period missing. > LayoutTests/svg/as-object/svg-embedded-in-html-in-iframe.html:2 > + > +<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"> I'd suggest to turn this into a HTML5 test, aka. <!DOCTYPE html> and leaving out xmlns everywhere. > LayoutTests/svg/as-object/svg-embedded-in-html-in-iframe.html:12 > +B+="<head>\r\n"; > +B+="<title>Circle</title>\r\n"; > +B+="</head><body>\r\n"; I'd remove the head alltogether, it has no benefit here. Also the \r\n are not needed. > LayoutTests/svg/as-object/svg-embedded-in-html-in-iframe.html:14 > +B+="<ellipse cx=\"50%\" cy=\"50%\" rx=\"50%\" ry=\"50%\" fill=\"blue\" stroke=\"none\" />\r\n"; Make it a circle :-) > LayoutTests/svg/as-object/svg-embedded-in-html-in-iframe.html:22 > +<iframe src="javascript:parent.CreateCircle();" width="100%" height="100%" frameborder="0" scrolling="no"></iframe> Neat trick Levi! :-)

Levi Weintraub

Comment 18 2011-10-19 13:37:38 PDT

Created attachment 111666 [details] Patch for landing

Levi Weintraub

Comment 19 2011-10-19 13:38:23 PDT

(In reply to comment #17) > (From update of attachment 111542 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=111542&action=review > > Great patch Levi, r=me. Please fix following issues before landing: > Thanks for the review!

WebKit Review Bot

Comment 20 2011-10-19 19:31:34 PDT

Comment on attachment 111666 [details] Patch for landing Rejecting attachment 111666 [details] from commit-queue. New failing tests: svg/as-object/svg-embedded-in-html-in-iframe.html Full output: http://queues.webkit.org/results/10176181

Nikolas Zimmermann

Comment 21 2011-10-20 00:56:23 PDT

Comment on attachment 111666 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=111666&action=review I forgot to mention another possible change, given that the commit failed, I'm now doing it :-) r- because of the location of the pixel test, I didn't spot it before, I guess I was too tired :( > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:139 > +static bool isEmbeddedThroughFrameContainingSVGDocument(const Frame* frame, RenderPart* ownerRenderer) > +{ > + ASSERT(frame); > + // 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 !ownerRenderer || !frame->document()->isSVGDocument(); > +} Just realized that passing around ownerRenderer is unnecessary. static inline bool isEmbeddedThroughFrameContainingSVGDocument(const Frame* frame) { ASSERT(frame); ASSERT(frame->document()); // 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(); } > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:152 > - // If our frame has an owner renderer, we're embedded through eg. object/embed. > RenderPart* ownerRenderer = frame->ownerRenderer(); > - if (!ownerRenderer) > + if (isEmbeddedThroughFrameContainingSVGDocument(frame, ownerRenderer)) > return computeIntrinsicWidth(replacedWidth); > > RenderStyle* ownerRendererStyle = ownerRenderer->style(); if (isEmbeddedThroughFrameContainingSVGDocument(frame)) return computeIntrinsicWidth(replacedWidth); RenderPart* ownerRenderer = frame->ownerRenderer(); RenderStyle* ownerRendererStyle = ownerRenderer->style(); ... > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:193 > - // If our frame has an owner renderer, we're embedded through eg. object/embed. > RenderPart* ownerRenderer = frame->ownerRenderer(); > - if (!ownerRenderer) > + if (isEmbeddedThroughFrameContainingSVGDocument(frame, ownerRenderer)) > return computeIntrinsicHeight(replacedHeight); > > RenderStyle* ownerRendererStyle = ownerRenderer->style(); Ditto. > LayoutTests/ChangeLog:11 > + * svg/as-object/svg-embedded-in-html-in-iframe-expected.png: Added. > + * svg/as-object/svg-embedded-in-html-in-iframe-expected.txt: Added. In theory this could be a cross-platform test, but it creates problems - see the commit queue failure. Currently we don't have any pixel test results (expected.png) or render tree dumps (expected.txt) in svg/as-object. So you better move these results to platform/mac/svg/as-object, in case you generated them on a mac, which I assume.

Levi Weintraub

Comment 22 2011-10-20 11:58:28 PDT

Comment on attachment 111666 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=111666&action=review >> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:139 >> +} > > Just realized that passing around ownerRenderer is unnecessary. > > static inline bool isEmbeddedThroughFrameContainingSVGDocument(const Frame* frame) > { > ASSERT(frame); > ASSERT(frame->document()); > // 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(); > } I passed it in to save a call since ownerRenderer() has some logic and isn't inlined. Doesn't matter a whole lot though. >> LayoutTests/ChangeLog:11 >> + * svg/as-object/svg-embedded-in-html-in-iframe-expected.txt: Added. > > In theory this could be a cross-platform test, but it creates problems - see the commit queue failure. > Currently we don't have any pixel test results (expected.png) or render tree dumps (expected.txt) in svg/as-object. So you better move these results to platform/mac/svg/as-object, in case you generated them on a mac, which I assume. Yeah, no such luck. Fixing.

Levi Weintraub

Comment 23 2011-10-20 12:17:19 PDT

Created attachment 111822 [details] Patch

WebKit Review Bot

Comment 24 2011-10-20 14:10:39 PDT

Comment on attachment 111822 [details] Patch Attachment 111822 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10181600 New failing tests: svg/as-object/svg-embedded-in-html-in-iframe.html

Levi Weintraub

Comment 25 2011-10-24 11:06:44 PDT

(In reply to comment #24) > (From update of attachment 111822 [details]) > Attachment 111822 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/10181600 > > New failing tests: > svg/as-object/svg-embedded-in-html-in-iframe.html In the right place. Pinging for review. It'd be great to get this landed soon.

Dimitri Glazkov (Google)

Comment 26 2011-10-24 11:12:30 PDT

Comment on attachment 111822 [details] Patch Transferring Niko's r=him. Can you add the test to test_expectations.txt before landing to avoid red bots?

Levi Weintraub

Comment 27 2011-10-24 11:13:20 PDT

(In reply to comment #26) > (From update of attachment 111822 [details]) > Transferring Niko's r=him. Can you add the test to test_expectations.txt before landing to avoid red bots? Absolutely! Thanks Dimitri!

Levi Weintraub

Comment 28 2011-10-24 15:40:27 PDT

Comment on attachment 111822 [details] Patch Clearing flags on attachment: 111822 Committed r98263: <http://trac.webkit.org/changeset/98263>

Note You need to log in before you can comment on or make changes to this bug.