If an EMBED is part of an OBJECT's fallback content, WebKit should only render it if the OBJECT fails to load.
In the case where a OBJECT has an EMBED in its fallback content, WebKit has traditionally loaded the EMBED instead of the OBJECT. For instance, in WebKit: <object data="movie.swf"> <p>Fallback content</p> <embed src="othermovie.mov"></embed> </object> was equivalent to writing: <embed src="othermovie.mov"></embed> The presence of a child <embed> causes WebKit to not attempt to load the <object>, even though they reference different resources. Furthermore, WebKit ignores the non-embed fallback content of the <object> that other browsers would render if the <object> fails to load. This behavior causes WebKit-based browsers to behave differently from other modern browsers (Firefox, Opera and IE all understand both <object> and <embed> at this point), and it diverges from how HTML5 specifies <object> (http://dev.w3.org/html5/spec/Overview.html#the-object-element).
<rdar://problem/7699852>
Created attachment 65357 [details] Patch
Created attachment 65474 [details] Patch (v2) Added additional test coverage.
Comment on attachment 65474 [details] Patch (v2) > + If an EMBED is part of an OBJECT's fallback content, WebKit should only > + render the EMBED if the OBJECT fails to load. Those capitalized tag names are so old school! I think <embed> and <object> are so much more 2010. > Node* p = parentNode(); > if (p && p->hasTagName(objectTag)) { > ASSERT(p->renderer()); > - return false; > + if (!static_cast<HTMLObjectElement*>(p)->useFallbackContent()) > + return false; > } A "why" comment would be so great here. > + bool useFallbackContent() const { return m_useFallbackContent; } My only concern about exposing this is that I don't know when the value is set up. For example, might HTMLEmbedElement::rendererIsNeeded call this function before the <object> has decided whether to use fallback content? When is it safe to call this? Maybe only when we have made a renderer? Can we add some kind of assertion? > - url = p->value(); > + url = deprecatedParseURL(p->value()); This looks like a good bug fix, but perhaps one that is separate from the main change here. Did you add a test case to cover this fix? Should we land this fix separately, first? > + // Turn the attributes of the OBJECT tag into arrays, but don't override PARAM values. I think "OBJECT tag" and "PARAM" should be either "object element" or "<object> element" and "<param> values". > + * accessibility/plugin.html: Simplified this test by removing the > + unnecessary <object> around the <embed>. But don't we also want test coverage for the common case where an <object> is around an <embed>?
(In reply to comment #5) > (From update of attachment 65474 [details]) > > + If an EMBED is part of an OBJECT's fallback content, WebKit should only > > + render the EMBED if the OBJECT fails to load. > > Those capitalized tag names are so old school! I think <embed> and <object> are so much more 2010. I'll change this to get with the times! > > > Node* p = parentNode(); > > if (p && p->hasTagName(objectTag)) { > > ASSERT(p->renderer()); > > - return false; > > + if (!static_cast<HTMLObjectElement*>(p)->useFallbackContent()) > > + return false; > > } > > A "why" comment would be so great here. Will do. > > > + bool useFallbackContent() const { return m_useFallbackContent; } > > My only concern about exposing this is that I don't know when the value is set up. For example, might HTMLEmbedElement::rendererIsNeeded call this function before the <object> has decided whether to use fallback content? I don't think so. HTMLObjectElement::renderFallbackContent() sets m_useFallbackContent to true and then calls attach(), which ends up calling HTMLEmbedElement::rendererIsNeeded(). HTMLEmbedElement::rendererIsNeeded() might be called prior to this, but this will be before we've made the decision to render fallback content, so we will not create a renderer for the <embed>, which is correct behavior. > > When is it safe to call this? Maybe only when we have made a renderer? Can we add some kind of assertion? We can probably assert that the <object> element's renderer is a RenderInline instead of a RenderEmbeddedObject. That'll ensure that m_useFallbackContent wasn't set to true without also tearing down the old renderer and replacing it with a RenderInline. > > > - url = p->value(); > > + url = deprecatedParseURL(p->value()); > > This looks like a good bug fix, but perhaps one that is separate from the main change here. Did you add a test case to cover this fix? Should we land this fix separately, first? You're right. This change broke a layout test in xssAuditor which exposed this bug, but it's logically separate. I'll submit a patch with this by itself. > > > + // Turn the attributes of the OBJECT tag into arrays, but don't override PARAM values. > > I think "OBJECT tag" and "PARAM" should be either "object element" or "<object> element" and "<param> values". Will do. > > > + * accessibility/plugin.html: Simplified this test by removing the > > + unnecessary <object> around the <embed>. > > But don't we also want test coverage for the common case where an <object> is around an <embed>? I don't know why I did this. The expected results changed so I decided to update the test rather than update the results, but I can do it the other way around. Thanks for the review!
(In reply to comment #5) > > - url = p->value(); > > + url = deprecatedParseURL(p->value()); > > This looks like a good bug fix, but perhaps one that is separate from the main change here. Did you add a test case to cover this fix? Should we land this fix separately, first? Fixed in http://trac.webkit.org/changeset/66137.
Committed http://trac.webkit.org/changeset/66156.