45188 2010-09-03 11:54:38 -0700 Setting innerHTML to a video element does not respect autoplay 2014-10-22 10:31:16 -0700 1 1 1 Unclassified WebKit Media 528+ (Nightly build) PC OS X 10.5 UNCONFIRMED P2 Normal --- 0 vrk webkit-unassigned eric.carlson perrinhouse vrk oldest_to_newest 274085 0 66527 vrk 2010-09-03 11:54:38 -0700 Created attachment 66527 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. 276258 1 66987 vrk 2010-09-08 18:42:24 -0700 Created attachment 66987 Patch 276259 2 vrk 2010-09-08 18:44:28 -0700 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. 279369 3 scherkus 2010-09-14 21:19:09 -0700 friendly ping for a code review! 279377 4 66987 eric.carlson 2010-09-14 21:50:15 -0700 Comment on attachment 66987 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? 279709 5 vrk 2010-09-15 14:16:14 -0700 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? 294183 6 eric.carlson 2010-10-14 10:33:33 -0700 *** Bug 46897 has been marked as a duplicate of this bug. *** 294863 7 eric.carlson 2010-10-15 12:41:01 -0700 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 294882 8 eric.carlson 2010-10-15 13:08:37 -0700 http://www.w3.org/Bugs/Public/show_bug.cgi?id=11068 294890 9 vrk 2010-10-15 13:19:28 -0700 @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! 66527 2010-09-03 11:54:38 -0700 2010-09-03 11:54:38 -0700 autoplay testcase vid.html text/html 1109 vrk PGh0bWw+DQo8c2NyaXB0Pg0KDQpmdW5jdGlvbiBTZXRJbm5lckhUTUwoKSB7DQogIHZhciBjb250 YWluZXIgPSBkb2N1bWVudC5nZXRFbGVtZW50QnlJZCgiY29udGFpbmVyIik7DQogIGNvbnRhaW5l ci5pbm5lckhUTUwgPSAnJzsNCiAgY29udGFpbmVyLmlubmVySFRNTCA9ICc8dmlkZW8gd2lkdGg9 NjQwIGhlaWdodD0zNjAgYXV0b3BsYXkgbG9vcCBjb250cm9scyBpZD0idmlkZW8iIHNyYz0iaHR0 cDovL2ZiYXJjaGFyZDAtdy5hZC5jb3JwLmdvb2dsZS5jb20vdGVzdG1hdHJpeC9iZWFyL2JlYXIy Lm9ndiI+IDwvdmlkZW8+JzsNCn0NCg0KZnVuY3Rpb24gQXBwZW5kQ2hpbGQoKSB7DQogIHZhciBj b250YWluZXIgPSBkb2N1bWVudC5nZXRFbGVtZW50QnlJZCgiY29udGFpbmVyIik7DQogIGNvbnRh aW5lci5pbm5lckhUTUwgPSAnJzsNCiAgdmFyIHZpZCA9IGRvY3VtZW50LmNyZWF0ZUVsZW1lbnQo J3ZpZGVvJyk7DQogIHZpZC5zZXRBdHRyaWJ1dGUoImF1dG9wbGF5IiwgIiIpOw0KICB2aWQuc2V0 QXR0cmlidXRlKCJsb29wIiwgIiIpOw0KICB2aWQuc2V0QXR0cmlidXRlKCJjb250cm9scyIsICIi KTsNCiAgdmlkLnNldEF0dHJpYnV0ZSgiaWQiLCAidmlkZW8iKTsNCiAgdmlkLnNldEF0dHJpYnV0 ZSgic3JjIiwgImh0dHA6Ly9mYmFyY2hhcmQwLXcuYWQuY29ycC5nb29nbGUuY29tL3Rlc3RtYXRy aXgvYmVhci9iZWFyMi5vZ3YiKTsNCiAgdmlkLnNldEF0dHJpYnV0ZSgid2lkdGgiLCAiNjQwIik7 DQogIHZpZC5zZXRBdHRyaWJ1dGUoImhlaWdodCIsICIzNjAiKTsNCiAgY29udGFpbmVyLmFwcGVu ZENoaWxkKHZpZCk7DQp9DQoNCjwvc2NyaXB0Pg0KPGJvZHk+DQogIDxkaXYgaWQ9ImNvbnRhaW5l ciI+VmlkZW8gR29lcyBIZXJlPC9kaXY+DQoNCiAgPGRpdiBpZD0iY29udHJvbHMiPg0KICAgIDxp bnB1dCB0eXBlPSJidXR0b24iIG9uY2xpY2s9IlNldElubmVySFRNTCgpOyIgdmFsdWU9IlNldCBJ bm5lciBIVE1MIi8+IChwcm9ibGVtKSA8YnIvPg0KICAgIDxpbnB1dCB0eXBlPSJidXR0b24iIG9u Y2xpY2s9IkFwcGVuZENoaWxkKCk7IiB2YWx1ZT0iQXBwZW5kIENoaWxkIi8+ICh3b3JrcykNCiAg PC9kaXY+DQo8L2JvZHk+DQo8L2h0bWw+DQo= 66987 2010-09-08 18:42:24 -0700 2010-09-14 21:50:15 -0700 Patch bug-45188-20100908184223.patch text/plain 4502 vrk ZGlmZiAtLWdpdCBhL0xheW91dFRlc3RzL0NoYW5nZUxvZyBiL0xheW91dFRlc3RzL0NoYW5nZUxv ZwppbmRleCA4ZmM4MTdjMjhmMDY0MjUyNGU3Y2Y3MDc3ZGE0Y2RmZjE1YjI4NmRkLi4xOTI4YTVl NWI3MDQ3Zjc2MjU2YjU1YTU5YWI1NDg4ZWE4ZTljZTMxIDEwMDY0NAotLS0gYS9MYXlvdXRUZXN0 cy9DaGFuZ2VMb2cKKysrIGIvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTUgQEAK KzIwMTAtMDktMDggIFZpY3RvcmlhIEtpcnN0ICA8dnJrQGdvb2dsZS5jb20+CisKKyAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQWRkaW5nIHRlc3RzIHRvIGNo ZWNrIGF1dG9wbGF5IG9uIHZpZGVvcyBjcmVhdGVkIGluIHRoZSBkb20gYW5kCisgICAgICAgIGNy ZWF0ZWQgdGhyb3VnaCBtYW5pcHVsYXRpbmcgaW5uZXJIVE1MLgorCisgICAgICAgICogbWVkaWEv dmlkZW8tYXV0b3BsYXktc2V0aW5uZXJodG1sLWV4cGVjdGVkLnR4dDogQWRkZWQuCisgICAgICAg ICogbWVkaWEvdmlkZW8tYXV0b3BsYXktc2V0aW5uZXJodG1sLmh0bWw6IEFkZGVkLgorICAgICAg ICAqIG1lZGlhL3ZpZGVvLWRvbS1hdXRvcGxheTItZXhwZWN0ZWQudHh0OiBBZGRlZC4KKyAgICAg ICAgKiBtZWRpYS92aWRlby1kb20tYXV0b3BsYXkyLmh0bWw6IEFkZGVkLgorCiAyMDEwLTA5LTA4 ICBGdW1pdG9zaGkgVWthaSAgPHVrYWlAY2hyb21pdW0ub3JnPgogCiAgICAgICAgIFVucmV2aWV3 ZWQuCmRpZmYgLS1naXQgYS9MYXlvdXRUZXN0cy9tZWRpYS92aWRlby1hdXRvcGxheS1zZXRpbm5l cmh0bWwtZXhwZWN0ZWQudHh0IGIvTGF5b3V0VGVzdHMvbWVkaWEvdmlkZW8tYXV0b3BsYXktc2V0 aW5uZXJodG1sLWV4cGVjdGVkLnR4dApuZXcgZmlsZSBtb2RlIDEwMDY0NAppbmRleCAwMDAwMDAw MDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwLi40NzFkZDc4MDU2YmM4MTI5OWFhNGNk NmQzZjNjOTQxYjAwZTNmZGE5Ci0tLSAvZGV2L251bGwKKysrIGIvTGF5b3V0VGVzdHMvbWVkaWEv dmlkZW8tYXV0b3BsYXktc2V0aW5uZXJodG1sLWV4cGVjdGVkLnR4dApAQCAtMCwwICsxLDMgQEAK K0VWRU5UKHBsYXkpCitFWFBFQ1RFRCAodmlkZW8ucGF1c2VkID09ICdmYWxzZScpIE9LCitFTkQg T0YgVEVTVApkaWZmIC0tZ2l0IGEvTGF5b3V0VGVzdHMvbWVkaWEvdmlkZW8tYXV0b3BsYXktc2V0 aW5uZXJodG1sLmh0bWwgYi9MYXlvdXRUZXN0cy9tZWRpYS92aWRlby1hdXRvcGxheS1zZXRpbm5l cmh0bWwuaHRtbApuZXcgZmlsZSBtb2RlIDEwMDY0NAppbmRleCAwMDAwMDAwMDAwMDAwMDAwMDAw MDAwMDAwMDAwMDAwMDAwMDAwMDAwLi5lYmYxMjI4YzNhZjVhYmFiZGU0M2E1NTkzYzZmMjdjNjIy MTU2ODZlCi0tLSAvZGV2L251bGwKKysrIGIvTGF5b3V0VGVzdHMvbWVkaWEvdmlkZW8tYXV0b3Bs YXktc2V0aW5uZXJodG1sLmh0bWwKQEAgLTAsMCArMSwxNiBAQAorPGRpdiBpZD0iY29udGFpbmVy Ij48L2Rpdj4KKzxzY3JpcHQgc3JjPW1lZGlhLWZpbGUuanM+PC9zY3JpcHQ+Cis8c2NyaXB0IHNy Yz12aWRlby10ZXN0LmpzPjwvc2NyaXB0PgorCis8c2NyaXB0PgorZnVuY3Rpb24gaXNQbGF5aW5n KCkgeworICAgIGNvbnNvbGVXcml0ZSgiRVZFTlQocGxheSkiKTsKKyAgICBmaW5kTWVkaWFFbGVt ZW50KCk7CisgICAgdGVzdEV4cGVjdGVkKCJ2aWRlby5wYXVzZWQiLCBmYWxzZSk7CisgICAgZW5k VGVzdCgpOworfQorICAgIHZhciBzcmMgPSBmaW5kTWVkaWFGaWxlKCJ2aWRlbyIsICJjb250ZW50 L3Rlc3QiKTsKKyAgICB2YXIgY29udGFpbmVyID0gZG9jdW1lbnQuZ2V0RWxlbWVudEJ5SWQoImNv bnRhaW5lciIpOworICAgIGNvbnRhaW5lci5pbm5lckhUTUwgPSAnPHZpZGVvIGF1dG9wbGF5IHNy Yz0iJyArIHNyYyArICciIG9ucGxheT0iaXNQbGF5aW5nKCk7Ij48L3ZpZGVvPic7Cis8L3Njcmlw dD4KKwpkaWZmIC0tZ2l0IGEvTGF5b3V0VGVzdHMvbWVkaWEvdmlkZW8tZG9tLWF1dG9wbGF5Mi1l eHBlY3RlZC50eHQgYi9MYXlvdXRUZXN0cy9tZWRpYS92aWRlby1kb20tYXV0b3BsYXkyLWV4cGVj dGVkLnR4dApuZXcgZmlsZSBtb2RlIDEwMDY0NAppbmRleCAwMDAwMDAwMDAwMDAwMDAwMDAwMDAw MDAwMDAwMDAwMDAwMDAwMDAwLi40NzFkZDc4MDU2YmM4MTI5OWFhNGNkNmQzZjNjOTQxYjAwZTNm ZGE5Ci0tLSAvZGV2L251bGwKKysrIGIvTGF5b3V0VGVzdHMvbWVkaWEvdmlkZW8tZG9tLWF1dG9w bGF5Mi1leHBlY3RlZC50eHQKQEAgLTAsMCArMSwzIEBACitFVkVOVChwbGF5KQorRVhQRUNURUQg KHZpZGVvLnBhdXNlZCA9PSAnZmFsc2UnKSBPSworRU5EIE9GIFRFU1QKZGlmZiAtLWdpdCBhL0xh eW91dFRlc3RzL21lZGlhL3ZpZGVvLWRvbS1hdXRvcGxheTIuaHRtbCBiL0xheW91dFRlc3RzL21l ZGlhL3ZpZGVvLWRvbS1hdXRvcGxheTIuaHRtbApuZXcgZmlsZSBtb2RlIDEwMDY0NAppbmRleCAw MDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwLi4zMDQxOGE2YzQ5NjIwYjcz M2U0N2U5OTNlMTg3NTliOWNlODdjMzAyCi0tLSAvZGV2L251bGwKKysrIGIvTGF5b3V0VGVzdHMv bWVkaWEvdmlkZW8tZG9tLWF1dG9wbGF5Mi5odG1sCkBAIC0wLDAgKzEsMTUgQEAKKzxkaXYgaWQ9 ImNvbnRhaW5lciI+PC9kaXY+Cis8c2NyaXB0IHNyYz1tZWRpYS1maWxlLmpzPjwvc2NyaXB0Pgor PHNjcmlwdCBzcmM9dmlkZW8tdGVzdC5qcz48L3NjcmlwdD4KKzxzY3JpcHQ+CisgICAgdmFyIGNv bnRhaW5lciA9IGRvY3VtZW50LmdldEVsZW1lbnRCeUlkKCJjb250YWluZXIiKTsKKyAgICB2YXIg dmlkZW8gPSBkb2N1bWVudC5jcmVhdGVFbGVtZW50KCd2aWRlbycpOworICAgIHZpZGVvLnNldEF0 dHJpYnV0ZSgiYXV0b3BsYXkiLCAiIik7CisgICAgdmlkZW8uc2V0QXR0cmlidXRlKCJzcmMiLCBm aW5kTWVkaWFGaWxlKCJ2aWRlbyIsICJjb250ZW50L3Rlc3QiKSk7CisgICAgdmlkZW8uYWRkRXZl bnRMaXN0ZW5lcigncGxheScsIGZ1bmN0aW9uIChldmVudCkgeworICAgICAgICBjb25zb2xlV3Jp dGUoIkVWRU5UKHBsYXkpIik7CisgICAgICAgIHRlc3RFeHBlY3RlZCgidmlkZW8ucGF1c2VkIiwg ZmFsc2UpOworICAgICAgICBlbmRUZXN0KCk7CisgICAgfSApOworICAgIGNvbnRhaW5lci5hcHBl bmRDaGlsZCh2aWRlbyk7Cis8L3NjcmlwdD4KZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9n IGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZDZlNjU3ZTY4Yzk1NWU5MmVmY2UzMmE0MzFjYzVl YWFiOGNiZTMwNi4uZjVhMDRhZjM5NjMyZTk5MTU1ODlmZWJiYmFjNjYyMWIzYmVkYjc3ZSAxMDA2 NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2ViQ29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNyBAQAorMjAxMC0wOS0wOCAgVmljdG9yaWEgS2lyc3QgIDx2cmtAZ29vZ2xlLmNvbT4K KworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBGaXggdG8g cmVzcGVjdCBhdXRvcGxheSBhdHRyaWJ1dGUgb24gdmlkZW8gdGFncyBpbmplY3RlZCBpbnRvIGEg cGFnZQorICAgICAgICBieSBzZXR0aW5nIGlubmVySFRNTC4KKyAgICAgICAgaHR0cHM6Ly9idWdz LndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTQ1MTg4CisKKyAgICAgICAgVGVzdHM6IG1lZGlh L3ZpZGVvLWF1dG9wbGF5LXNldGlubmVyaHRtbC5odG1sCisgICAgICAgICAgICAgICBtZWRpYS92 aWRlby1kb20tYXV0b3BsYXkyLmh0bWwKKworICAgICAgICAqIGh0bWwvSFRNTE1lZGlhRWxlbWVu dC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpIVE1MTWVkaWFFbGVtZW50OjpyZW1vdmVkRnJvbURv Y3VtZW50KToKKwogMjAxMC0wOS0wOCAgQWRhbSBCYXJ0aCAgPGFiYXJ0aEB3ZWJraXQub3JnPgog CiAgICAgICAgIFJldmlld2VkIGJ5IEVyaWMgU2VpZGVsLgpkaWZmIC0tZ2l0IGEvV2ViQ29yZS9o dG1sL0hUTUxNZWRpYUVsZW1lbnQuY3BwIGIvV2ViQ29yZS9odG1sL0hUTUxNZWRpYUVsZW1lbnQu Y3BwCmluZGV4IDljMmMxNWM4OWJkODliZDFjZmI1NDY0ZmM0YzZlMDNhN2E3NDY2ZmUuLjQ1MWY3 MWRkODM5NWQzOWZlOTMwZmIyNmIwMWNhOWEzYjYwNWE2NTggMTAwNjQ0Ci0tLSBhL1dlYkNvcmUv aHRtbC9IVE1MTWVkaWFFbGVtZW50LmNwcAorKysgYi9XZWJDb3JlL2h0bWwvSFRNTE1lZGlhRWxl bWVudC5jcHAKQEAgLTMwOSw3ICszMDksNyBAQCB2b2lkIEhUTUxNZWRpYUVsZW1lbnQ6Omluc2Vy dGVkSW50b0RvY3VtZW50KCkKIAogdm9pZCBIVE1MTWVkaWFFbGVtZW50OjpyZW1vdmVkRnJvbURv Y3VtZW50KCkKIHsKLSAgICBpZiAobV9uZXR3b3JrU3RhdGUgPiBORVRXT1JLX0VNUFRZKQorICAg IGlmIChtX25ldHdvcmtTdGF0ZSAhPSBORVRXT1JLX0VNUFRZICYmIG1fbmV0d29ya1N0YXRlICE9 IE5FVFdPUktfTk9fU09VUkNFKQogICAgICAgICBwYXVzZShwcm9jZXNzaW5nVXNlckdlc3R1cmUo KSk7CiAgICAgaWYgKG1faXNGdWxsc2NyZWVuKQogICAgICAgICBleGl0RnVsbHNjcmVlbigpOwo=