778902 - When a video finished playing, "pause" should be fired, and |paused| should be true.

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Paul Adenot (:padenot)]

From the spec [1]:

> Queue a task that, if the media element does not have a current media
> controller, and the media element has still ended playback, and the direction
> of playback is still forwards, and paused is false, changes paused to true and
> fires a simple event named pause at the media element.

We don't have a MediaController, so the media element should be in paused state, and we should fire a paused event, here.

[1]: http://www.whatwg.org/html/the-video-element.html#ended-playback

Comment 1

[Paul Adenot (:padenot)]

Attachment: Patch.

If the reading of the spec is correct (i.e. we don't have a media controller), here is a patch.

Comment 2

[Paul Adenot (:padenot)]

Changed the relevant tests, green locally (including video controls tests), pushed to try at https://tbpl.mozilla.org/?tree=Try&rev=701737d85fc5

Comment 3

[Paul Adenot (:padenot)]

Attachment: Patch with tests updated.

The try push is green with this patch that has updated tests.

There is a behavioral change with this patch. If we seek to the beginning of a media after it has reached the end, it won't start automatically, i.e. the user will have to press play (or call play()) for the media to start playing again. 

Maybe we want to change that to keep the current behavior.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Paul ADENOT (:padenot) from comment #2)
> Changed the relevant tests, green locally (including video controls tests),
> pushed to try at https://tbpl.mozilla.org/?tree=Try&rev=701737d85fc5

You should do a try run with all tests enabled; <video> is used in several different test suites, not only in mochitest-1. For example the a11y tests use video, and they're not in mochitest-1.  I usually just do a try-all run when I make platform/API changes; it hard to keep track of who's added code which uses the APIs we're working on!

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 647767 [details] [diff] [review]
Patch with tests updated.

Review of attachment 647767 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with two minor changes.

You should do a try-all run before landing this, particularly once you've changed to call Pause() rather than re-duplicating the pause logic, since that may change behaviour further...

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +2771,5 @@
>      SetCurrentTime(0);
>      return;
>    }
>  
> +  mPaused = true;

I think you should call Pause() here, rather than trying to manage all the state... For example, you're not resetting mAutoPlaying here, and I'm pretty sure you should be...

::: content/media/test/test_paused_after_ended.html
@@ +14,5 @@
>  
>  function ended(evt) {
>    var v = evt.target;
> +  is(v.gotPause, true, "We should have received a \"pause\" event.")
> +  is(v.paused, true, v._name + " must not be paused after end");

Actually, you're asserting that we *are* paused after [playback has reached the] end! ;)

Did you mean:

v._name + "must have received ended after pause event" ?

Comment 6

[Paul Adenot (:padenot)]

Attachment: When a video finished playing, "pause" should be fired, and |paused| should be true.

Comment 7

[Paul Adenot (:padenot)]

Comment on attachment 649387 [details] [diff] [review]
When a video finished playing, "pause" should be fired, and |paused| should be true.

Addressed comment, green on try [1]. Carrying forward r+.

[1]: https://tbpl.mozilla.org/?tree=Try&rev=859080478900

Comment 8

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2940e190783f

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2940e190783f