1181055 - video play button does not work if the VIDEO element has onclick handler calling play()

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect, P1)

Description

[Alice0775 White]

Chrome dev45 works as expected, but Firefox(incl Nightly42.0a1) does not work :(

Reproduced: always

Steps to reproduce:
1. Open http://himawari8.nict.go.jp/himawari8-movie.htm
2. Click a video to playback. 
   --- The video will start playback automatically as expected.
3. Wait until the play is completed
4. Seek to the head(0:00) by dragging thumb
5. Attempt to playback by clicking play button

Alternate Steps to reproduce:
1. Open http://himawari8.nict.go.jp/himawari8-movie.htm
2. Click a video to playback. 
   --- The video will start playback automatically as expected.
3. Pause the video
4. Seek to the head(0:00) by dragging thumb
5. Attempt to playback by clicking play button


Actual Results:
The video does not start.
However, "Play" command in the context menu works as expected.

Expected Results:
Play buttun should be working properly.

Comment 1

[Alice0775 White]

Attachment: log.txt

log.txt

set NSPR_LOG_MODULES=timestamp,MediaDecoder:5,MediaSource:5,MediaPromise:5,MP4Demuxer:5,nsMediaElement:5,nsMediaElementEvents:5
set MEDIA_LOG_SAMPLES=1

set NSPR_LOG_FILE=%TEMP%\log.txt

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

I found

<video class="movie_main" style="position:fixed; top:0px; right:0px; bottom:0px; left:0px; margin:auto; max-height:100%; max-width:100%; z-index:9999;" controls="controls" autoplay="autoplay" name="media" preload="none" onclick="this.play()">

in the page source.

After removing |onclick="this.play()"| using Web developer tool, it works fine again. It should just let the UI controls handle UI events for play/pause/seek instead of listening to click event and calling play() manually since it breaks how UI handle events somehow.

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Btw, it is also bad to specify |controls="controls" autoplay="autoplay"|. You can just say <video controls autoplay></video>. Check http://www.w3schools.com/tags/att_video_autoplay.asp for an example.

Comment 4

[Alice0775 White]

(In reply to JW Wang [:jwwang] from comment #3)
> Btw, it is also bad to specify |controls="controls" autoplay="autoplay"|.
> You can just say <video controls autoplay></video>. Check
> http://www.w3schools.com/tags/att_video_autoplay.asp for an example.

The spec[1][2][3] said that 
 The controls attribute is a boolean attribute.
 The autoplay attribute is a boolean attribute. 

 2.4.2 Boolean attributes
 If the attribute is present, its value must either be the empty string
 or a value that is an ASCII case-insensitive match for the attribute's
 canonical name, with no leading or trailing whitespace.

[1] http://www.w3.org/TR/html51/semantics.html#user-interface
[2] http://www.w3.org/TR/html51/semantics.html#attr-media-autoplay
[3] http://www.w3.org/TR/html51/infrastructure.html#boolean-attributes



Btw, |www.w3schools.com| is NOT spec.

And also the following is NOT spec, But said
https://www.w3.org/wiki/HTML/Elements/video#HTML_Attributes

> ・autoplay = "autoplay" or "" (empty string) or empty
>   Instructs the UA to automatically begin playback of the video 
>   as soon as it can do so without stopping. 

> ・controls = "controls" or "" (empty string) or empty
>   Instructs the UA to expose a user interface for controlling
>    playback of the video. 

(Both pages are no specifications)

Comment 5

[Alice0775 White]

The problem is |onclick="this.play()"| .

So, this is the site problem.

Comment 6

[Hallvord R. M. Steen [:hallvors]]

I can't reproduce this problem on the site - they fixed it by removing the onclick handler. However, I can't see why adding onclick="this.play()" should break the play button..? Especially as it works in Chrome I think this is a bug in Gecko's video UI handling..

Comment 7

[Hallvord R. M. Steen [:hallvors]]

Attachment: 1181055.htm - demo

Comment 8

[Trevor Rowbotham [:rowbot]]

This seems to be fallout from Bug 659285.  I can reproduce this only with media.autoplay.enabled = false.

Comment 9

[Trevor Rowbotham [:rowbot]]

Ignore comment 8, I was wrong.

Comment 10

[Ralph Giles (:rillian)]

This seems pretty broken. We should fix it, or pass to dom if it turns out to be an issue there.

Comment 11

[Alastor Wu [:alwu]]

I can't reproduce this issue in OSX with latest nightly (changeset:290645).

Comment 12

[Alice0775 White]

I can still reproduce the problem on Latest Nightly[1] with STR of Comment 7.

[1]
https://hg.mozilla.org/mozilla-central/rev/63be002b4a803df1122823841ef7633b7561d873
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0 ID:20160328030215

Comment 13

[Alastor Wu [:alwu]]

Oh, I can also reproduce that using the STR in comment7.
Remove the qa-flag.

Comment 14

[Alastor Wu [:alwu]]

[Root cause]
The click event always be triggered before the video control's click, and the behavior of the control interface is depending on media element's play state to decide whether need to play or pause.

Therefore, every time we click the element, the code flow is like that,
1. On MediaElement's onclick, play video
2. On video control's togglePaused, pause video (because the video is playing)

[Solution]
The best solution I think is that the onclick function should also be triggered when user is pressing media element at where is not covered by control interface. (Chrome is already done that, so this issue won't happen in Chrome)

Comment 15

[Alastor Wu [:alwu]]

However, I don't know how to block the onclick function for media element when user is pressing at the place where is covered by media element's control interface.

--

Hi, Jared,
Sorry to bother you, could you give me any comment on this issue?
Since you're working on video-control, maybe you have other good solution or you know how to implement the method I proposed.
Thanks!

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

I'm not sure what you're asking for when saying to "block the onclick function for media element". This is specifically a "tricky" problem because if controls are enabled then we want to provide a good basic environment to play videos. This includes the ability to click on the video to toggle playback. We have a specific event listener for the area on top of the video just for this feature.

Ideally, if content was adding an event listener to the element that would toggle playback they would set event.preventDefault() on the event object. We already check that in our event listener though it seems like we can't depend on content being that responsible.

What do you think about this solution:
1) Add a capturing event listener that tracks the state of the video (playing or paused)
2) In the pre-existing clickToPlayClickHandler, compare the state captured in the capturing event listener to see if the play state has changed.
3a) If it has changed, treat this the same as event.defaultPrevented
3b) If it's not changed, proceed with the togglePause() call.

Comment 17

[Alastor Wu [:alwu]]

Hi, Jaren,
Let me explain my idea in more detail.

The view of media element can be separate into two layer [1],
(1) UI layer : the control interface
(2) Content layer : the media element's video content

And I think the onclick event of the media element should only be triggered when we clicked at the content layer instead of clicked at both two layers, and that is also what Chrome did. 

Is it possible for us? and do you think is it reasonable?

---

Sorry, I don't understand about |event.preventDefault()|, is it helpful for this issue?

The onclick function of media element is always triggered before the video control's clickToPlayClickHandler, so when the togglePause() starts, the video was already been started by the onclick function of media element (if we added |video.play()| in the callback).

Thanks!

Comment 18

[Alastor Wu [:alwu]]

Sorry, forgot to add the link.

[1] https://goo.gl/D6oRcH

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: MozReview Request: Bug 1181055 - video play button does not work if the VIDEO element has onclick handler calling play(). r?gijs

Review commit: https://reviewboard.mozilla.org/r/52521/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52521/

Comment 20

[:Gijs (he/him)]

Comment on attachment 8752302 [details]
MozReview Request: Bug 1181055 - video play button does not work if the VIDEO element has onclick handler calling play(). r?gijs

https://reviewboard.mozilla.org/r/52521/#review49493

::: toolkit/content/tests/widgets/test_videocontrols_onclickplay.html:21
(Diff revision 1)
> +Cu.import("resource://testing-common/BrowserTestUtils.jsm");
> +Cu.import("resource://testing-common/ContentTask.jsm");
> +ContentTask.setTestScope(window.wrappedJSObject)

Can you clarify why we need these? I think we can just get rid of them - certainly BTU seems unused. I don't understand what the ContentTask thing accomplishes - perhaps we can nuke that too?

::: toolkit/content/tests/widgets/test_videocontrols_onclickplay.html:49
(Diff revision 1)
> +      timeupdates++;
> +
> +      if (timeupdates == 3) {

Nit: if (++timeupdates == 3)

::: toolkit/content/tests/widgets/test_videocontrols_onclickplay.html:57
(Diff revision 1)
> +        video.removeEventListener("timeupdate", timeupdate);
> +        resolve();
> +      }
> +    });
> +
> +    synthesizeMouseAtCenter(video, {}, window);

Technically this checks that clicking the video works - do we need the same test for the play button itself, or does that not have this problem?

::: toolkit/content/widgets/videocontrols.xml:1110
(Diff revision 1)
>                      // Read defaultPrevented asynchronously, since Web content
>                      // may want to consume the "click" event but will only
>                      // receive it after us.
>                      let self = this;
> +                    let previousState = this.video.paused;
>                      setTimeout(function clickToPlayCallback() {
> -                        if (!e.defaultPrevented)
> +                        if (e.defaultPrevented ||
> +                            self.video.paused != previousState) {
> +                            return;
> +                        }
> -                            self.togglePause();
> +                        self.togglePause();
>                      }, 0);

Very clever. Can you update the comment above this block? Also, while we're here, can we switch to using an arrow function and nuke the `let self = this` hack? :-)

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

https://reviewboard.mozilla.org/r/52521/#review49493

> Can you clarify why we need these? I think we can just get rid of them - certainly BTU seems unused. I don't understand what the ContentTask thing accomplishes - perhaps we can nuke that too?

This was flotsam from earlier work. I've removed it.

> Technically this checks that clicking the video works - do we need the same test for the play button itself, or does that not have this problem?

Thanks for asking this. The play button was following a different code path that wasn't fixed by this patch. I've now fixed this in the lastest patch and included a test for it.

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8752302 [details]
MozReview Request: Bug 1181055 - video play button does not work if the VIDEO element has onclick handler calling play(). r?gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52521/diff/1-2/

Comment 23

[:Gijs (he/him)]

Comment on attachment 8752302 [details]
MozReview Request: Bug 1181055 - video play button does not work if the VIDEO element has onclick handler calling play(). r?gijs

https://reviewboard.mozilla.org/r/52521/#review49652

::: toolkit/content/widgets/videocontrols.xml:1099
(Diff revision 2)
>                      }
>                      this.setFullscreenButtonState();
>                  },
>  
>                  clickToPlayClickHandler : function(e) {
> -                    if (e.button != 0)
> +                    if (e.button != undefined && e.button != 0)

When would e.button be undefined? Isn't e.button always set for click events?

::: toolkit/content/widgets/videocontrols.xml:1379
(Diff revision 2)
>                      return isDescendant(event.target) && isDescendant(event.relatedTarget);
>                  },
>  
>                  log : function (msg) {
>                      if (this.debug)
> -                        dump("videoctl: " + msg + "\n");
> +                        console.log("videoctl: " + msg + "\n");

Also flotsam? :-)

(not sure we can depend on console.log here...)

::: toolkit/content/widgets/videocontrols.xml:1521
(Diff revision 2)
>                        self.controlListeners.push({ item: elem, event: eventName, func: boundFunc });
>                        elem.addEventListener(eventName, boundFunc, false);
>                      }
>  
>                      addListener(this.muteButton, "command", this.toggleMute);
> -                    addListener(this.playButton, "command", this.togglePause);
> +                    addListener(this.playButton, "click", this.clickToPlayClickHandler);

This listener also does some things wrt error handling... is that necessary/useful/OK here?

Also, why the change away from "command"? Does using the button with the keyboard still work? Can we keep using command with the update to clickToPlayClickHandler to deal with an undefined e.button param?

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

https://reviewboard.mozilla.org/r/52521/#review49652

> When would e.button be undefined? Isn't e.button always set for click events?

I added this when the function was being called by both "click" and "command" event listeners. With the play button switched to a "click" event we can remove it.

> Also flotsam? :-)
> 
> (not sure we can depend on console.log here...)

I intentionally left it here as it made it easier to debug. Considering debug mode is a build-time switch for the video controls, I would prefer to leave it in. I'm not sure where we wouldn't be able to depend on it, but as developers we can control the environment.

> This listener also does some things wrt error handling... is that necessary/useful/OK here?
> 
> Also, why the change away from "command"? Does using the button with the keyboard still work? Can we keep using command with the update to clickToPlayClickHandler to deal with an undefined e.button param?

For the error handling, this is fine to put here. It's basically a no-op, because we hide the control bar when an error is present.

I had to change away from "command" because we need to respect the "click" event having preventDefault() called on it. We do this by listening for the "click" event, storing state, then using a setTimeout to defer our decision to toggle based on if the event was defaultPrevented. The "click" event is dispatched before the "command" event. Both events are dispatched to the video controls and outside of the video controls to the webpage. However, the "command" event is only dispatched if the user clicks on the play button, but not on the rest of the video. Therefore, it would be uncommon for web developers to rely on the "command" event, and even moreso to use preventDefault() on it.

The buttons on the control bar in the video controls are not keyboard accessible and haven't been keyboard accessible. When the element is focused, the video controls *do* listen for key events and will play/pause from "Space", seek from left/right arrow keys, and change volume from up/down arrow keys.

Comment 25

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #24)
> I had to change away from "command" because we need to respect the "click"
> event having preventDefault() called on it. We do this by listening for the
> "click" event, storing state, then using a setTimeout to defer our decision
> to toggle based on if the event was defaultPrevented. The "click" event is
> dispatched before the "command" event. Both events are dispatched to the
> video controls and outside of the video controls to the webpage. However,
> the "command" event is only dispatched if the user clicks on the play
> button, but not on the rest of the video. Therefore, it would be uncommon
> for web developers to rely on the "command" event, and even moreso to use
> preventDefault() on it.

Sure. I did think you could get the click event from the command event using "sourceEvent", so you could check defaultPrevented that way, maybe? - plus I would have expected that a preventDefault()'d event wouldn't trigger a command event...

Anyway, it sounds like the distinction shouldn't matter...

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for check-in (depends on patch in bug 1270853)

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d5acbd8b896c

Comment 28

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/0ba8107314d6 - browser_dbg_event-listeners-03.js counts the number of event listeners, and you surprised it by removing one, https://treeherder.mozilla.org/logviewer.html#?job_id=9417435&repo=fx-team

Comment 29

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/00ed91d9c2c83fec8c61565ef1f73ed7a7df724a
Bug 1181055 - Treat the toggling of playback state in the videocontrols' content "click" event listener as the same as preventDefault. r=gijs

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/00ed91d9c2c8