Bug 17688 – <embed> width and height properties propagate to parent object node

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[jasneet]

I Steps:
Go to
http://lequipe.fr/
Scroll towards bottom of page to the "video" section at right.

II Issue:
Notice the alignment of the text and the video. In Safari, the video is displayed at the center of the section, making the text to appear at the bottom.

III Conclusion: The width defined in the embed tag is causing the issue. FF/IE/Opera take the width defined in the object tag but Safari takes the width defined in the embed tag.

IV Other browsers:
IE7: ok
FF: ok
Opera: ok

V Nightly tested: 30236

[jasneet]

Created attachment 19560 [details]
screenshot

[jasneet]

Created attachment 19561 [details]
reduction

[Robert Blaut]

Definitely bug. <embed> without src attribute should be ignored: 

"The src attribute gives the address of the resource being embedded. The attribute must be present and contain a URI (or IRI).

If the src attribute is missing, then the embed element must be ignored." [http://www.whatwg.org/specs/web-apps/current-work/#embed]

In this case <embed> dimensions magically propagates to parent <object> element, but only if you define object dimensions with attributes width and height. The problem isn't visible if you set dimensions using styles. Check attached "test case".

[Robert Blaut]

Created attachment 19566 [details]
test case

[Robert Hogan]

Created attachment 105694 [details]
Patch

[Andy Estes]

Comment on attachment 105694 [details]
Patch

Nice patch. This looks to be a vestige of WebKit's old behavior of merging attributes and params when an <object> has a nested <embed>. That behavior has been removed over the last year but this bit was overlooked.

[Darin Adler]

Comment on attachment 105694 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105694&action=review

> Source/WebCore/ChangeLog:13
> +        WebKit seems to have inherited this behaviour from KHTML. When the width/height
> +        of an <embed> element is set, it propagates the values up to any parent <object>
> +        element. There doesn't seem to be any good reason for this and it is not consistent
> +        with the behaviour of Firefox and Opera.

If WebKit has been doing this for years, it seems likely there is some content that depends on it. What kinds of testing have we done to ensure this is a compatible change for content that has not been tested with Firefox?

[Robert Hogan]

(In reply to comment #7)
> (From update of attachment 105694 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105694&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        WebKit seems to have inherited this behaviour from KHTML. When the width/height
> > +        of an <embed> element is set, it propagates the values up to any parent <object>
> > +        element. There doesn't seem to be any good reason for this and it is not consistent
> > +        with the behaviour of Firefox and Opera.
> 
> If WebKit has been doing this for years, it seems likely there is some content that depends on it. What kinds of testing have we done to ensure this is a compatible change for content that has not been tested with Firefox?

The only examples of that kind of thing I can think of are Steam and corporate intranets. If there's anything that springs to mind that I can have a go at, please shoot!

[Justin Schuh]

Comment on attachment 105694 [details]
Patch

I can't speak to the Darin's question, but I can confirm there's no risk of this reintroducing issue fixed for bug 56690. (Also, there's one nit I've noted below.)

View in context: https://bugs.webkit.org/attachment.cgi?id=105694&action=review

> Source/WebCore/html/HTMLEmbedElement.cpp:237
>  void HTMLEmbedElement::attributeChanged(Attribute* attr, bool preserveDecls)
>  {
>      HTMLPlugInImageElement::attributeChanged(attr, preserveDecls);

Shouldn't you remove the virtual override entirely if you're just calling the parent class method?

[Andy Estes]

(In reply to comment #8)
> The only examples of that kind of thing I can think of are Steam and corporate intranets. If there's anything that springs to mind that I can have a go at, please shoot!

It's a much larger list than that. For instance, any application on Mac OS X can embed WebKit as a system framework to display HTML content. This content is likely to only be tested in WebKit and so is particularly subject to WebKit idioms. The same issue exists for iOS, and Android, and other platforms too.

[Robert Hogan]

Created attachment 105791 [details]
Patch

[Robert Hogan]

(In reply to comment #9)
> 
> Shouldn't you remove the virtual override entirely if you're just calling the parent class method?

Good spot - updated the patch.

[Robert Hogan]

(In reply to comment #10)
> 
> It's a much larger list than that. For instance, any application on Mac OS X can embed WebKit as a system framework to display HTML content. This content is likely to only be tested in WebKit and so is particularly subject to WebKit idioms. The same issue exists for iOS, and Android, and other platforms too.

Mmm, so what to do?

[Andy Estes]

(In reply to comment #13)
> (In reply to comment #10)
> > 
> > It's a much larger list than that. For instance, any application on Mac OS X can embed WebKit as a system framework to display HTML content. This content is likely to only be tested in WebKit and so is particularly subject to WebKit idioms. The same issue exists for iOS, and Android, and other platforms too.
> 
> Mmm, so what to do?

I think it's great that we're changing a bit of our idiomatic behavior to match other engines and the HTML5 spec. It looks like we're aware of at least one website that WebKit will better support after this change, which is also great. We do risk causing regressions with any behavior change, however, and unfortunately it's impossible to test all the WebKit-specific content out there. Given this, it'd be nice to show that you've done some basic due diligence, such as:

- Testing popular (and even not-so-popular) video sites to see if this patch breaks something. Some might sniff User-Agent and serve WebKit special markup.
- Testing other sites that embed the various popular plug-ins (Flash, QuickTime, Java, Silverlight, etc.).
- If you're developing on Mac OS X and feel particularly methodical, you could even sanity check a handful of the major non-browser WebKit clients that you think might use <object> or <embed>. A somewhat outdated list of Mac OS X WebKit clients is here: <http://trac.webkit.org/wiki/Applications%20using%20WebKit>.

[Andy Estes]

You can also test cases where <object> and <embed> are used for things other than plug-ins. For instance, both <object> and <embed> can also be used for images and subframes, and <embed> can even be used to load SVG documents.

[Robert Hogan]

(In reply to comment #14)
> 
> - Testing popular (and even not-so-popular) video sites to see if this patch breaks something. Some might sniff User-Agent and serve WebKit special markup.
> - Testing other sites that embed the various popular plug-ins (Flash, QuickTime, Java, Silverlight, etc.).

I tested the patch using QtTestBrowser at:

youtube.com
guardian.co.uk
vimeo.com
cnn.com
apple.com

All sites worked fine. QtTestBrowser only supports flash, so wasn't able to test QuickTime, Java etc.

[Eric Seidel (no email)]

Comment on attachment 105791 [details]
Patch

Fantastic!  I recently saw this code and wondered why we did this. :)

[WebKit Review Bot]

Comment on attachment 105791 [details]
Patch

Rejecting attachment 105791 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
/html/HTMLEmbedElement.cpp
Hunk #1 FAILED at 222.
Hunk #2 succeeded at 227 with fuzz 1 (offset -19 lines).
1 out of 2 hunks FAILED -- saving rejects to file Source/WebCore/html/HTMLEmbedElement.cpp.rej
patching file Source/WebCore/html/HTMLEmbedElement.h
Hunk #1 FAILED at 40.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/html/HTMLEmbedElement.h.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Eric Seidel', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/11186620

[Robert Hogan]

Committed r105030: <http://trac.webkit.org/changeset/105030>

[noel gordon]

(In reply to comment #17)
> Fantastic!  I recently saw this code and wondered why we did this. :)

Did you mean the <embed> width/height propagation thing?  Flash embeds were painful/problematic cross-browser until the Flash satay was developed.  

  http://www.longtailvideo.com/support/jw-player/13/embedding-flash#ObjectEmbedCombined
  http://www.alistapart.com/articles/flashsatay

and that method relied on the width/height propagation I believe.

[Eric Seidel (no email)]

Supposedly this caused this regression: http://code.google.com/p/chromium/issues/detail?id=127907

[Robert Hogan]

(In reply to comment #21)
> Supposedly this caused this regression: http://code.google.com/p/chromium/issues/detail?id=127907

Looks like it did. I've opened https://bugs.webkit.org/show_bug.cgi?id=86682