|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
58 bytes, text/x-review-board-request
|Details | Review|
Spawned from bug 1231886: "Set media.autoplay.enabled to false and straight HTML5 videos don't play when you click the large play button (overlay) at the center of the video. http://video.webmfiles.org/big-buck-bunny_trailer.webm" This bug will concentrate on straight videos (i.e., not Youtube or others).
Created attachment 8714170 [details] MozReview Request: Bug 1244592 - Fix non-autoplay click-to-play - r?cpearce When media.autoplay.enabled==false, only videos may only be started through direct user actions. Unfortunately, for straight video files, the click-to-play handler defers video.play() to a 0-timeout (to allow other scripts to consume the click if they wish), which means the play() action arrives *outside* of the user- interaction window; in which case it will be blocked. The easiest solution is to send a play() then pause() immediately in the click handler, so that the play() is not blocked; a side-effect is that the media element remembers that user interaction has started, and will not block future play()'s. // If not already started, send a quick play then pause, // so the media element thinks user interaction has happened // when calling play() again in the timeout below (otherwise // media.autoplay.enabled==false might block it). Review commit: https://reviewboard.mozilla.org/r/32963/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/32963/
(In reply to Gerald Squelart [:gerald] from comment #1) > // If not already started, send a quick play then pause, [...] Sorry for the pasted code comment in the patch comment in comment 1, I'll remove it from the patch comment. (And sorry for the overuse of the word 'comment' in this comment.)
Comment on attachment 8714170 [details] MozReview Request: Bug 1244592 - Fix non-autoplay click-to-play - r?cpearce Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32963/diff/1-2/
(In reply to Gerald Squelart [:gerald] from comment #4) > Review request updated; see interdiff: > https://reviewboard.mozilla.org/r/32963/diff/1-2/ Small change, much better way to do the sneaky play-then-pause only once per media element. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d4e9cb3bafc
Boris and/or Olli, Chris suggested that I contact you to find a better way to fix this issue. Quick summary: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#1093 The straight-file video controls' "play" button does video.play() in a timeout handler (to allow user scripts to intercept the click and set defaultPrevented on the event). But a side-effect of that is that the delayed play() call appears to come from a non-user-initiated script and therefore is blocked when media.autoplay.enabled==false. My attempt at fixing this was to immediately send a play() (then pause), so the media element thinks it's been interacted with, and won't block the play() in the timeout. It worked, but it may also introduce side-effects from the extra play() that could confuse some websites. So, would you have ideas on how else we could perform the 'play()', *after* other scripts have had a chance to intercept the click, but such that it will still appear as a user-initiated action?
The right theoretical way to do this is to use a system event listener, I would think. But I'm not sure XBL can add those in this case, since it has no access to the nsIEventListenerService. Olli, maybe we should add a way for it to be able to? For example, adding something IsChromeOrXBL on EventTarget or something?
https://reviewboard.mozilla.org/r/32963/#review30247 Closing review, will work on better patch.
Not sure where you want to add the event listener, but group="system" in xbl's <handler> element? That should work when we're dealing with native anonymous content, like we should here. You can of course check event's .originalTarget or it ancestors in handler to make sure you're dealing with the right target, if you need to add the listener somewhere higher up in the (composed) dom tree and not to the expected target.
(In reply to Boris Zbarsky [:bz] from comment #7) > Olli, maybe we should add a way for it to be able to? For example, adding > something IsChromeOrXBL on EventTarget or something? Yeah, we could add [IsChromeOrXBL] addSystemEventListener to EventTarget. I think that shouldn't be too scary.
Mass change P2 -> P3