Bug 17688 - <embed> width and height properties propagate to parent object node
Summary: <embed> width and height properties propagate to parent object node
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Robert Hogan
URL: http://lequipe.fr/
Keywords: HasReduction
Depends on:
Blocks:
 
Reported: 2008-03-05 15:28 PST by jasneet
Modified: 2012-05-16 15:21 PDT (History)
10 users (show)

See Also:


Attachments
screenshot (256.23 KB, image/jpeg)
2008-03-05 15:28 PST, jasneet
no flags Details
reduction (787 bytes, text/html)
2008-03-05 15:29 PST, jasneet
no flags Details
test case (328 bytes, text/html)
2008-03-05 23:57 PST, Robert Blaut
no flags Details
Patch (7.61 KB, patch)
2011-08-30 14:26 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (8.45 KB, patch)
2011-08-31 10:48 PDT, Robert Hogan
eric: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jasneet 2008-03-05 15:28:14 PST
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
Comment 1 jasneet 2008-03-05 15:28:50 PST
Created attachment 19560 [details]
screenshot
Comment 2 jasneet 2008-03-05 15:29:44 PST
Created attachment 19561 [details]
reduction
Comment 3 Robert Blaut 2008-03-05 23:56:48 PST
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".
Comment 4 Robert Blaut 2008-03-05 23:57:19 PST
Created attachment 19566 [details]
test case
Comment 5 Robert Hogan 2011-08-30 14:26:34 PDT
Created attachment 105694 [details]
Patch
Comment 6 Andy Estes 2011-08-30 14:34:33 PDT
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.
Comment 7 Darin Adler 2011-08-30 15:02:47 PDT
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?
Comment 8 Robert Hogan 2011-08-30 15:39:16 PDT
(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!
Comment 9 Justin Schuh 2011-08-30 16:40:52 PDT
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?
Comment 10 Andy Estes 2011-08-30 17:16:21 PDT
(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.
Comment 11 Robert Hogan 2011-08-31 10:48:48 PDT
Created attachment 105791 [details]
Patch
Comment 12 Robert Hogan 2011-08-31 11:30:46 PDT
(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.
Comment 13 Robert Hogan 2011-08-31 11:57:55 PDT
(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?
Comment 14 Andy Estes 2011-09-02 19:16:47 PDT
(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>.
Comment 15 Andy Estes 2011-09-02 19:19:42 PDT
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.
Comment 16 Robert Hogan 2011-09-20 12:01:35 PDT
(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.
Comment 17 Eric Seidel (no email) 2012-01-09 13:22:53 PST
Comment on attachment 105791 [details]
Patch

Fantastic!  I recently saw this code and wondered why we did this. :)
Comment 18 WebKit Review Bot 2012-01-09 13:25:14 PST
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
Comment 19 Robert Hogan 2012-01-15 09:20:22 PST
Committed r105030: <http://trac.webkit.org/changeset/105030>
Comment 20 noel gordon 2012-01-15 20:55:12 PST
(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.
Comment 21 Eric Seidel (no email) 2012-05-13 14:24:27 PDT
Supposedly this caused this regression: http://code.google.com/p/chromium/issues/detail?id=127907
Comment 22 Robert Hogan 2012-05-16 15:21:39 PDT
(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