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
Created attachment 101381 [details] SVG Circle Example Example of SVG creating a circle
Created attachment 101382 [details] Shot of how the example displays in different browsers.
Created attachment 103089 [details] Simpler sample (no JS)
Looks like a regression from http://trac.webkit.org/changeset/87526
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.
Are there any known workarounds for this issue?
Niko, are you working on this?
(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.
(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.
(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.
(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.
I've got a working patch I'm testing. I'll take care of this.
Created attachment 111536 [details] Patch
Comment on attachment 111536 [details] Patch Attachment 111536 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10140170
Created attachment 111542 [details] Patch
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
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! :-)
Created attachment 111666 [details] Patch for landing
(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!
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
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.
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.
Created attachment 111822 [details] Patch
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
(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.
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?
(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!
Comment on attachment 111822 [details] Patch Clearing flags on attachment: 111822 Committed r98263: <http://trac.webkit.org/changeset/98263>