Open http://cd.pn/yt/. Click-to-play. Click on the "Change" button to load a new resource. Note that no "click to play" button appears. Also if you keep the controls visible, the "current time" doesn't reset to 0 immediately.

We believe this is affecting the Youtube app on v1.0.1.

This isn't a complete fix to all the issues that could arise when a new load happens. Maybe init() should be called again, or something like that. But this is less risky.

Comment on attachment 754413 [details] [diff] [review] fix We need review pretty much "now" for B2G.

I think setupNewLoadState will need to be called in 'loadstart' not 'loadedmetadata' to account for the preload=none case.

This moves the call to setupNewLoadState to loadstart. It also makes setupNewLoadState check video.paused --- for preload=none videos, it's possible for loadstart to fire after play() has already been called, in which case we don't want to show the click-to-play button or the controls.

Comment on attachment 754479 [details] [diff] [review] fix v2, v1.0.1 version Review of attachment 754479 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/content/widgets/videocontrols.xml @@ +357,5 @@ > // Since the play button will be showing, we don't want to > // show the throbber behind it. The throbber here will > // only show if needed after the play button has been pressed. > + if (!this.clickToPlay.hidden) { > + this.startFade(this.statusOverlay, false, true); This change seems to work against the goal of the comment above it.

Comment on attachment 754479 [details] [diff] [review] fix v2, v1.0.1 version Review of attachment 754479 [details] [diff] [review]: ----------------------------------------------------------------- Random omg-definitely-not-for-this-bug ponder: I wonder if it would be useful to have each media event carry the full state of the media element as it was when the event was dispatched. The existing complexity in setupStatusFader() got me thinking about this as a way to avoid subtle bugs caused by mixing the "as it was" state implied by an event and "as it is" state returned by media.someProperty. For example, aiui a listener for the "paused" event is not assured to find that media.paused == true. But it could be assured to find that event.mediaState.paused == true. In theory, code that was entirely event-driven could then have a stronger set of invariants and be easier to test -- dispatching any single event at it would be sufficient, and every event could effectively be a full reset / initialization. The event name essentially becomes an hint for "what changed". A media.sendGenericStateEvent() method+event might be useful for first setup. I suppose the inverse would be to listen for all media events, and have the listener update the controls based on the current state. ::: toolkit/content/widgets/videocontrols.xml @@ +357,5 @@ > // Since the play button will be showing, we don't want to > // show the throbber behind it. The throbber here will > // only show if needed after the play button has been pressed. > + if (!this.clickToPlay.hidden) { > + this.startFade(this.statusOverlay, false, true); this.startFadeOut(this.statusOverlay, true); @@ +473,5 @@ > this._handleCustomEventsBound = this.handleCustomEvents.bind(this); > this.video.addEventListener("media-showStatistics", this._handleCustomEventsBound, false, true); > }, > > + setupNewLoadState : function() { Hmm. This is sorta what setupInitialState() was intended to do, although originally aimed more are the binding being attached at an arbitrary time than loading a new resource. I'd like to move towards a single "resetState()" kind of thing, but I suppose what you've got is safest for B2G's immediate needs. @@ -1392,5 @@ > - // > - // (Note: the |controls| attribute is already handled via layout/style/html.css) > - var shouldShow = (!(this.video.autoplay && this.video.mozAutoplayEnabled) || !this.dynamicControls); > - // Hide the overlay if the video time is non-zero or if an error occurred to workaround bug 718107. > - this.startFade(this.clickToPlay, shouldShow && !this.isAudioOnly && Note that this doesn't apply on m-c because this line changed. Just a gentile reminder to make sure that change carries over to setupNewLoadState(). Would be nice if diff had a way to represent moving code. And ponies.

(In reply to Jared Wein [:jaws] from comment #7) > ::: toolkit/content/widgets/videocontrols.xml > @@ +357,5 @@ > > // Since the play button will be showing, we don't want to > > // show the throbber behind it. The throbber here will > > // only show if needed after the play button has been pressed. > > + if (!this.clickToPlay.hidden) { > > + this.startFade(this.statusOverlay, false, true); > > This change seems to work against the goal of the comment above it. Then I'm confused ... This code hides the throbber if the play button is showing. Which is exactly what the comment says! (In reply to Justin Dolske [:Dolske] from comment #8) > I wonder if it would be useful to have each media event carry the full state > of the media element as it was when the event was dispatched. The existing > complexity in setupStatusFader() got me thinking about this as a way to > avoid subtle bugs caused by mixing the "as it was" state implied by an event > and "as it is" state returned by media.someProperty. Yes, I agree this is an issue. I tried to get it fixed in the HTML5 spec (so during event dispatch the element's methods would return state "as it was" when the event was triggered) but that was rejected. Your suggestion is a good one. > ::: toolkit/content/widgets/videocontrols.xml > @@ +357,5 @@ > > // Since the play button will be showing, we don't want to > > // show the throbber behind it. The throbber here will > > // only show if needed after the play button has been pressed. > > + if (!this.clickToPlay.hidden) { > > + this.startFade(this.statusOverlay, false, true); > > this.startFadeOut(this.statusOverlay, true); OK, will do. > @@ +473,5 @@ > > this._handleCustomEventsBound = this.handleCustomEvents.bind(this); > > this.video.addEventListener("media-showStatistics", this._handleCustomEventsBound, false, true); > > }, > > > > + setupNewLoadState : function() { > > Hmm. This is sorta what setupInitialState() was intended to do, although > originally aimed more are the binding being attached at an arbitrary time > than loading a new resource. > > I'd like to move towards a single "resetState()" kind of thing, but I > suppose what you've got is safest for B2G's immediate needs. Yes, I fully agree.

Oh, I meant to add: There's also the question as to if the CTP UI state should really be shown again when an element's resource is (re)set. I think there was some vague notion of CTP being mainly useful for calling attention to the media when the page _first_ loads. After which, once the user has interacted with the element, CTP is perhaps no longer so useful (which is why it's not shown again after the playback first ends). EG, a page doing a "slideshow" of videos back-to-back might not want the CTP UI flashing into view between playbacks. But mostly this falls in to the "need more usage feedback" as to what makes sense.

(In reply to Jared Wein [:jaws] from comment #7) > Comment on attachment 754479 [details] [diff] [review] > fix v2, v1.0.1 version > > Review of attachment 754479 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/content/widgets/videocontrols.xml > @@ +357,5 @@ > > // Since the play button will be showing, we don't want to > > // show the throbber behind it. The throbber here will > > // only show if needed after the play button has been pressed. > > + if (!this.clickToPlay.hidden) { > > + this.startFade(this.statusOverlay, false, true); > > This change seems to work against the goal of the comment above it. I misunderstood this code change. After changing this to use startFadeOut it will be clearer.

[Approval Request Comment] Bug caused by (feature/regressing bug #): none User impact if declined: Play button fails to appear in some apps when a new media resource is loaded into an existing media element (especially in Youtube app) Testing completed: a small amount of testing by me and Chris Double Risk to taking this patch (and alternatives if risky): similar to bug 876426. Fairly low risk, videocontrols not used much in b2g 1.0.1. String or UUID changes made by this patch: none

Hmm, on central we have var shouldShow = (!(this.video.autoplay && this.video.mozAutoplayEnabled) || !this.dynamicControls); // Hide the overlay if the video time is non-zero or if the video is already playing // or if an error occurred to workaround bug 718107. this.startFade(this.clickToPlay, shouldShow && this.video.paused && !this.isAudioOnly && this.video.currentTime == 0 && !this.hasError(), true); this.startFade(this.controlBar, shouldShow, true); Note that the "paused" check I added to shouldShow has been added to startFade(this.clickToPlay) here. I'm pretty sure we don't want to bring up the controlBar for a video that's already playing. So I'm going to carry my change forward.

Push backed out for crashtest timeouts: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=6e02333d572b&jobname=crashtest remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ef539966c3c3 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b4a23a7c47e4 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7be40b778117

Hmm...the timeout is coming from a webrtc crashtest that currently only runs on FxAndroid and Desktop Firefox. That test in particular runs a PC handshake, reloads the page, and repeats that same process 5 times in a row.

https://bugzilla.mozilla.org/show_bug.cgi?id=876426#c13 explains the issue.

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/dabb3cc21eb9 https://hg.mozilla.org/releases/mozilla-b2g18/rev/fa49cffe5575

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9) > I tried to get it fixed in the HTML5 spec (so > during event dispatch the element's methods would return state "as it was" > when the event was triggered) but that was rejected. Your suggestion is a > good one. Wouldn't implementing "as it was" be compatible with what the spec defines? I.e. if we implement "as it was" we wouldn't be failing test harnesses, but websites would work more reliably in Firefox. We'd of course need to be upfront about it and let people on the lists know. Implementation behavior weighs very heavily.

(In reply to Jonas Sicking (:sicking) from comment #20) > Wouldn't implementing "as it was" be compatible with what the spec defines? > I.e. if we implement "as it was" we wouldn't be failing test harnesses, but > websites would work more reliably in Firefox. I think there would be observable differences especially once you combine media elements with other Web platform features. > We'd of course need to be upfront about it and let people on the lists know. > Implementation behavior weighs very heavily. Justin's idea of passing state into the event makes a lot of sense to me and sidesteps the spec issues. However, we also might be able to keep the current behavior and refactor the videocontrols code to get mostly the same benefits, per Justin's comment above "I suppose the inverse would be to listen for all media events, and have the listener update the controls based on the current state." I think we should try that.

Not clear how this got uplifted to v1.0.1 without being tef+ - setting that flag now so we can keep this tracked correctly in case of backout/regressions.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13) > I'm pretty sure we don't want to bring up the controlBar for a video that's > already playing. So I'm going to carry my change forward. Yeah, and is also consistent with the existing check there to not initially show controls for autoplay videos.

Works fine on b2g18 5/31 build.