Bug 64823 - <svg> fails to use explicit width and height inside <html> inside IFRAME
: <svg> fails to use explicit width and height inside <html> inside IFRAME
Status: RESOLVED FIXED
: WebKit
SVG
: 528+ (Nightly build)
: All Unspecified
: P2 Normal
Assigned To:
: http://code.google.com/p/chromium/iss...
:
:
:
  Show dependency treegraph
 
Reported: 2011-07-19 14:03 PST by
Modified: 2011-10-24 15:40 PST (History)


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


Note

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


Description From 2011-07-19 14:03:17 PST
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
------- Comment #1 From 2011-07-19 14:04:24 PST -------
Created an attachment (id=101381) [details]
SVG Circle Example

Example of SVG creating a circle
------- Comment #2 From 2011-07-19 14:07:49 PST -------
Created an attachment (id=101382) [details]
Shot of how the example displays in different browsers.
------- Comment #3 From 2011-08-05 11:26:34 PST -------
Created an attachment (id=103089) [details]
Simpler sample (no JS)
------- Comment #4 From 2011-08-05 11:39:52 PST -------
Looks like a regression from http://trac.webkit.org/changeset/87526
------- Comment #5 From 2011-08-05 12:04:02 PST -------
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.
------- Comment #6 From 2011-10-10 14:06:37 PST -------
Are there any known workarounds for this issue?
------- Comment #7 From 2011-10-13 14:43:14 PST -------
Niko, are you working on this?
------- Comment #8 From 2011-10-14 11:29:06 PST -------
(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.
------- Comment #9 From 2011-10-14 11:32:55 PST -------
(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.
------- Comment #10 From 2011-10-17 10:07:16 PST -------
(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.
------- Comment #11 From 2011-10-17 12:33:32 PST -------
(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.
------- Comment #12 From 2011-10-18 16:50:51 PST -------
I've got a working patch I'm testing. I'll take care of this.
------- Comment #13 From 2011-10-18 17:26:59 PST -------
Created an attachment (id=111536) [details]
Patch
------- Comment #14 From 2011-10-18 17:50:55 PST -------
(From update of attachment 111536 [details])
Attachment 111536 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10140170
------- Comment #15 From 2011-10-18 17:55:11 PST -------
Created an attachment (id=111542) [details]
Patch
------- Comment #16 From 2011-10-18 19:13:58 PST -------
(From update of attachment 111542 [details])
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 #17 From 2011-10-19 01:39:23 PST -------
(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:

> 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! :-)
------- Comment #18 From 2011-10-19 13:37:38 PST -------
Created an attachment (id=111666) [details]
Patch for landing
------- Comment #19 From 2011-10-19 13:38:23 PST -------
(In reply to comment #17)
> (From update of attachment 111542 [details] [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 #20 From 2011-10-19 19:31:34 PST -------
(From update of attachment 111666 [details])
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 #21 From 2011-10-20 00:56:23 PST -------
(From update of attachment 111666 [details])
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 #22 From 2011-10-20 11:58:28 PST -------
(From update of attachment 111666 [details])
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.
------- Comment #23 From 2011-10-20 12:17:19 PST -------
Created an attachment (id=111822) [details]
Patch
------- Comment #24 From 2011-10-20 14:10:39 PST -------
(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
------- Comment #25 From 2011-10-24 11:06:44 PST -------
(In reply to comment #24)
> (From update of attachment 111822 [details] [details])
> Attachment 111822 [details] [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 #26 From 2011-10-24 11:12:30 PST -------
(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?
------- Comment #27 From 2011-10-24 11:13:20 PST -------
(In reply to comment #26)
> (From update of attachment 111822 [details] [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 #28 From 2011-10-24 15:40:27 PST -------
(From update of attachment 111822 [details])
Clearing flags on attachment: 111822

Committed r98263: <http://trac.webkit.org/changeset/98263>