Bug 64823 - <svg> fails to use explicit width and height inside <html> inside IFRAME
Summary: <svg> fails to use explicit width and height inside <html> inside IFRAME
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Unspecified
: P2 Normal
Assignee: Nobody
URL: http://code.google.com/p/chromium/iss...
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-19 14:03 PDT by Jamie Starke
Modified: 2011-10-24 15:40 PDT (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Jamie Starke 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
Comment 1 Jamie Starke 2011-07-19 14:04:24 PDT
Created attachment 101381 [details]
SVG Circle Example

Example of SVG creating a circle
Comment 2 Jamie Starke 2011-07-19 14:07:49 PDT
Created attachment 101382 [details]
Shot of how the example displays in different browsers.
Comment 3 Tim Horton 2011-08-05 11:26:34 PDT
Created attachment 103089 [details]
Simpler sample (no JS)
Comment 4 Tim Horton 2011-08-05 11:39:52 PDT
Looks like a regression from http://trac.webkit.org/changeset/87526
Comment 5 Tim Horton 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.
Comment 6 Michael Thomas 2011-10-10 14:06:37 PDT
Are there any known workarounds for this issue?
Comment 7 Dimitri Glazkov (Google) 2011-10-13 14:43:14 PDT
Niko, are you working on this?
Comment 8 Nikolas Zimmermann 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.
Comment 9 Nikolas Zimmermann 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.
Comment 10 Dimitri Glazkov (Google) 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.
Comment 11 Nikolas Zimmermann 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.
Comment 12 Levi Weintraub 2011-10-18 16:50:51 PDT
I've got a working patch I'm testing. I'll take care of this.
Comment 13 Levi Weintraub 2011-10-18 17:26:59 PDT
Created attachment 111536 [details]
Patch
Comment 14 Gyuyoung Kim 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
Comment 15 Levi Weintraub 2011-10-18 17:55:11 PDT
Created attachment 111542 [details]
Patch
Comment 16 WebKit Review Bot 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
Comment 17 Nikolas Zimmermann 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! :-)
Comment 18 Levi Weintraub 2011-10-19 13:37:38 PDT
Created attachment 111666 [details]
Patch for landing
Comment 19 Levi Weintraub 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!
Comment 20 WebKit Review Bot 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
Comment 21 Nikolas Zimmermann 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.
Comment 22 Levi Weintraub 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.
Comment 23 Levi Weintraub 2011-10-20 12:17:19 PDT
Created attachment 111822 [details]
Patch
Comment 24 WebKit Review Bot 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
Comment 25 Levi Weintraub 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.
Comment 26 Dimitri Glazkov (Google) 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?
Comment 27 Levi Weintraub 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!
Comment 28 Levi Weintraub 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>