UNCONFIRMED 45188
Setting innerHTML to a video element does not respect autoplay 
https://bugs.webkit.org/show_bug.cgi?id=45188
Summary Setting innerHTML to a video element does not respect autoplay
Victoria Kirst
Reported 2010-09-03 11:54:38 PDT
Created attachment 66527 [details] autoplay testcase When you write JavaScript to inject a video into the page by setting the innerHTML of the element that will contain the video, the video does not autoplay even if the tag is set. This was the only scenario I could find where autoplay did not work; if one writes the video tag in HTML it works fine, if one creates the video node in JS using the DOM and injects it in the page with appendChild, it works fine, or if there is an existing video element in the DOM to which you add the autoplay attribute and a src, it also works fine. I think this is the change list that broke it: http://trac.webkit.org/changeset/65170/trunk/WebCore/html/HTMLMediaElement.cpp But I'm not familiar enough with the code to know why exactly it's doing this.
Attachments
autoplay testcase (1.08 KB, text/html)
2010-09-03 11:54 PDT, Victoria Kirst 		no flags
Details
Patch (4.40 KB, patch)
2010-09-08 18:42 PDT, Victoria Kirst 		eric.carlson: review-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Victoria Kirst
Comment 1 2010-09-08 18:42:24 PDT
Created attachment 66987 [details] Patch
Victoria Kirst
Comment 2 2010-09-08 18:44:28 PDT
I added a patch that fixes the bug, but I am not sure it makes sense :) I am just using my best guess from what I think should happen after tracing through the execution of code in a debugger. I also added a few more layout tests to catch this bug in the future.
Andrew Scherkus
Comment 3 2010-09-14 21:19:09 PDT
friendly ping for a code review!
Eric Carlson
Comment 4 2010-09-14 21:50:15 PDT
Comment on attachment 66987 [details] Patch > + * media/video-dom-autoplay2-expected.txt: Added. > + * media/video-dom-autoplay2.html: Added. These check to make sure WebKit behaves the same in two slightly different situations, and have exactly the same event handler test so I would prefer if they were combined into a single test. > +<div id="container"></div> > +<script src=media-file.js></script> > +<script src=video-test.js></script> > + > +<script> > +function isPlaying() { > + consoleWrite("EVENT(play)"); > + findMediaElement(); > + testExpected("video.paused", false); > + endTest(); > +} > + var src = findMediaFile("video", "content/test"); > + var container = document.getElementById("container"); The function should be indented to match the rest of the <scrip> content. In general I *greatly* pefer fully formed test files, eg. please add an HTML5 doctype, <html>, <body>, etc. > void HTMLMediaElement::removedFromDocument() > { > - if (m_networkState > NETWORK_EMPTY) > + if (m_networkState != NETWORK_EMPTY && m_networkState != NETWORK_NO_SOURCE) > pause(processingUserGesture()); > if (m_isFullscreen) > exitFullscreen(); This is incorrect according to the spec, which in 4.8.9.8 says: When a media element is removed from a Document, if the media element's  networkState attribute has a value other than NETWORK_EMPTYthen the user agent must act as if the pause() method had been invoked. What actually happens to prevent autoplay from happening? Is the problem that m_player is NULL so we call scheduleLoad?
Victoria Kirst
Comment 5 2010-09-15 14:16:14 PDT
Hi Eric! (In reply to comment #4) > What actually happens to prevent autoplay from happening? Is the problem that m_player is NULL so we call scheduleLoad? Short version: When HTMLMediaElement::setReadyState(HAVE_ENOUGH_DATA) is called, the play event for an autoplaying video will trigger if the condition m_autoplaying && m_paused && autoplay() is true. When a video element is injected via setInnerHTML, m_autoplaying is false here. Long version: So this is my understanding of what's happening: - When setInnerHTML is called, the HTML element calls createFragmentFromSource, which parses the innerHTML and builds the tree fragment representing it. - The way this works is, a DocumentFragment is created that will hold the final fragment tree, but the HTMLTreeBuilder stores in work-in-progress tree in m_dummyDocumentForFragmentParsing. Then when the dummy tree is completely built, the contents of the dummy document is transferred to the DocumentFragment in a the call m_fragment->takeAllChildrenFrom(root); (from HTMLTreeBuilder::FragmentParsingContext::finished()). - When takeAllChildrenFrom(root) is called, HTMLMediaElement::removedFromDocument() is called, and because m_networkState is set to NETWORK_NO_SOURCE, pause() is also called. (Before your change, at this point m_networkState would be set to NETWORK_EMPTY and pause() would *not* be called.) - When pause() --> pauseInternal() is called, m_autoplaying is set from true (default value) to false. - Now when setReadyState(HAVE_ENOUGH_DATA) is called, m_autoplaying is false so the playEvent is never scheduled. Aaand there's where I'm at! It seems like the problem is m_autoplaying is false when it should be true, but I don't know where exactly the state goes wrong (i.e. should m_autoplaying be left alone in the pause() method? should it be reset to true again somewhere? etc. My guess was that perhaps pause() shouldn't have been called at all-- turns out my guess was wrong :) ) Thoughts?
Eric Carlson
Comment 6 2010-10-14 10:33:33 PDT
*** Bug 46897 has been marked as a duplicate of this bug. ***
Eric Carlson
Comment 7 2010-10-15 12:41:01 PDT
The current behavior is "correct" in as much as both the media element and the tree parser do exactly what the spec requires. I chatted with Hixie about this and he will be updating the spec: Hixie: eric_carlson: ah the problem is that innerHTML technically removes an element from a document and so it blocks autoplay? ... Hixie: maybe the solution is to only pause() the resource if the element is removed from a document and not reinserted in the same task? Hixie: actually that makes sense in general Hixie: forget innerHTML, consider the case of someone moving a video from an iframe to another, or from a window to another Hixie: you don't want the autoplay to stop working suddenly ... Hixie: k, i'll file a bug on the spec and get that fixed soon
Eric Carlson
Comment 8 2010-10-15 13:08:37 PDT
http://www.w3.org/Bugs/Public/show_bug.cgi?id=11068
Victoria Kirst
Comment 9 2010-10-15 13:19:28 PDT
@eric.carlson: Interesting! Yeah, this is definitely a corner case of removing an element from a document... Thanks so much for digging into this and filing the bug!
Note You need to log in before you can comment on or make changes to this bug.