| 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: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | PC | ||||||||
| OS: | OS X 10.5 | ||||||||
| Attachments: |
|
||||||||
|
Description
Victoria Kirst
2010-09-03 11:54:38 PDT
Created attachment 66987 [details]
Patch
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. friendly ping for a code review! 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? 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? *** Bug 46897 has been marked as a duplicate of this bug. *** 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: 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! |