Bug 45188

Summary:

Setting innerHTML to a video element does not respect autoplay

Product:

WebKit

Reporter:

Victoria Kirst <vrk>

Component:

Media

Assignee:

Nobody <webkit-unassigned>

Status:

UNCONFIRMED

Severity:

Normal

CC:

eric.carlson, perrinhouse, vrk

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Attachments:

Description	Flags
autoplay testcase	none
Patch	eric.carlson: review-

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-	Details Formatted Diff Diff
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.