The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla17

Status

()

Core
Audio/Video
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: padenot, Assigned: padenot)

Tracking

Trunk
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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
(Assignee)

Comment 1

5 years ago
Created attachment 647276 [details] [diff] [review]
Patch.

If the reading of the spec is correct (i.e. we don't have a media controller), here is a patch.
Attachment #647276 - Flags: review?(cpearce)
(Assignee)

Comment 2

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

Comment 3

5 years ago
Created attachment 647767 [details] [diff] [review]
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.
Attachment #647767 - Flags: review?(cpearce)
(Assignee)

Updated

5 years ago
Attachment #647276 - Attachment is obsolete: true
Attachment #647276 - Flags: review?(cpearce)
(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 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" ?
Attachment #647767 - Flags: review?(cpearce) → review+
(Assignee)

Comment 6

5 years ago
Created attachment 649387 [details] [diff] [review]
When a video finished playing, "pause" should be fired, and |paused| should be true.
(Assignee)

Updated

5 years ago
Attachment #647767 - Attachment is obsolete: true
(Assignee)

Comment 7

5 years ago
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
Attachment #649387 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/2940e190783f
Status: NEW → ASSIGNED
Keywords: checkin-needed
Target Milestone: --- → mozilla17
Version: 16 Branch → Trunk
https://hg.mozilla.org/mozilla-central/rev/2940e190783f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.