496231 - videocontrols don't properly change play button state after video has ended

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Justin Dolske [:Dolske]]

Spun off from bug 496147.

When clicking the play button after a video has ended, playback resumes from the beginning but the play button's icon does not change. It should display "||" while playing, but is instead showing "|>". If you click twice more (to pause and then unpause), the icon then changes to the expected "||".

Note that to avoid the problem in bug 496147, you'll need to seek before the video ends. Then you see the problem here.

When the playback first ends, .paused is false. When clicking play, the controls a "play" event to fire but it doesn't (the controls just see, in order: waiting, seeking, timeupdate --> 0.0, canplay, playing, canplaythrough, seeked, timeupdates...).

I'm not sure if the core is supposed to be firing "play" here or not. It sure seems like calling play() after playback has ended should result in a play event. But the video is technically still playing (.paused == false) after the end, it just can't advance the position, and so play() is effectively just a seek to 0.0 during ongoing playback (in which case a play event wouldn't be expected). Perhaps the controls should change state upon "playing" in addition to just "play"?

Comment 1

[Matthew Gregan [:kinetik]]

> Perhaps the controls should change state upon "playing" in addition
to just "play"?

Not in addition, I don't think.  You'll always get the playing event, so you can just use that.  See bug 488287 and http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2009-March/019012.html.

I think this bug and bug 490114 are the same, so we can probably close bug 490114 as a dupe.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

See http://www.whatwg.org/specs/web-apps/current-work/#dom-media-play. play() only fires "play" if we are currently paused --- but we aren't.

When we seek back to the beginning, "playing" should fire, so yes, that's what you should listen for.

So this bug is invalid as filed, I think.

Comment 3

[Justin Dolske [:Dolske]]

(In reply to comment #2)

> So this bug is invalid as filed, I think.

I'm going to morph it to be for making the videocontrols use the right event.

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Possible to fix as stability update?

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Oh, and I'd take a patch if one came along ... and all the tests still passed ...

Comment 7

[Justin Dolske [:Dolske]]

(In reply to comment #1)

> Not in addition, I don't think.  You'll always get the playing event, so you
> can just use that.  See bug 488287 and
> http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2009-March/019012.html.

Hmm, well, here's the problem. The "playing" event might not fire until long after the play button is clicked, but we want the UI (button icon) to change state immediately. The "play" and "pause" events are normally very convenient to use for this.

I guess another way of looking at the problem is that the "ended" event is convenient for detecting when the video ends (which the button icon treats as a "pause"), but there's no directly analogous event for not-ended  (to treat as a "play"). Seems like this might be useful to add to the spec...

I should be able to add a check in the timeupdate handler... It feels a bit like overkill, but there should be an immediate timeupdate event fired when a video is played (or, more generally, seeked to a playable position) after it has ended. (using "seeking" is tempting, because it's fired less often, but it the spec makes it sound like this isn't always fired when changing the playback position -- 4.8.10.10 Seeking, step 9).

Comment 8

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

Like so?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 381447 [details] [diff] [review]
Patch v.1

looks reasonable.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 381447 [details] [diff] [review]
Patch v.1

I'll take comment #6 as preemptive approval

Comment 11

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/f4a9e6f39872

Comment 12

[Justin Dolske [:Dolske]]

Pushed to 191: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/db15a52a4fd4