1402877 - Clicking on the time slider to seek dispatches a "click" event to the video element

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Jean-Yves Avenard [:jya]]

STR:
1- Open a video (like http://rebbephoto.com/test/ffwebm)
2- Wait for the video to start.
3- Move the mouse so that the media controller appears
4- Click anywhere on the timeline

Note that video seeks, but the media element is paused.

If you click anywhere on the timeline again, the video seeks again, but this time media element plays.

Seems that everytime you click on the timeline it pauses, and if you click a 2nd time it plays..

Expected:
It should seeks and continue playing where it seeked.

If we want to video to be paused (which is unlike any other web browser), the behaviour should be consistent: e.g it pauses no matter how many times you click, or it plays. Shouldn't be intermittent

Comment 1

[Ray Lin[:ralin]]

Thanks for reporting this. 

I see a onclick handler on the video trying to toggle play/pause. I would think in this way, no matter there's a custom controls or just an inline script, we should respect the click handler and don't manipulate event to ensure not break user script even  our builtin controls is present.

Jared, do you think we should do something here?  Thanks.

Comment 2

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

This goes back to https://bugzilla.mozilla.org/show_bug.cgi?id=693014

The website will need to call event.preventDefault() to disable our play/pause toggling.

Comment 3

[Jean-Yves Avenard [:jya]]

Actually, this has nothing to do with bug 693014, but that the site has an onClick handler that will pause the content.

my bad.

Comment 4

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

I guess you are asking that we not send the event to the page and only handle clicking on control buttons ourselves without the page getting an event?

Comment 5

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

It's actually more specific than that. This problem only occurs with the scrubber. Clicks on the scrubber probably shouldn't resolve as clicks on the video? I don't know how simple that will be to implement (I've tried a couple things already).

Comment 6

[Jean-Yves Avenard [:jya]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> I guess you are asking that we not send the event to the page and only
> handle clicking on control buttons ourselves without the page getting an
> event?
Nothing of the sort...
I lodged the bug wondering why it paused like that which is something I wasn't used to, and considering chrome didn't do it.


the scrubber is above the video floating, so I guess it could be argued that you're clicking on the video.

Chrome appears to ignore those click though as it doesn't pause/play the video when you click on the scrubber

Comment 7

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

(In reply to Jean-Yves Avenard [:jya] from comment #6)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> > I guess you are asking that we not send the event to the page and only
> > handle clicking on control buttons ourselves without the page getting an
> > event?
> Nothing of the sort...
> I lodged the bug wondering why it paused like that which is something I
> wasn't used to, and considering chrome didn't do it.
> 
> 
> the scrubber is above the video floating, so I guess it could be argued that
> you're clicking on the video.
> 
> Chrome appears to ignore those click though as it doesn't pause/play the
> video when you click on the scrubber

Okay, thanks for the details.

Ralin, do you think you could try implementing what I changed the summary to?

Comment 8

[Ray Lin[:ralin]]

sure, I guess not only time line but the other controls on controls should have similar problem, so I'll see if we can prevent  any click event within controls(the grey bar) dispatch to video.

Comment 9

[Ray Lin[:ralin]]

Also I made a reduced cases[1] to test on different browsers, the result:

Firefox:  affected
Chrome: unaffected
Edge:   unaffected
Safari:   affected

[1] http://raylin.cc/test-web/video-event.html 

Not surprised if any other browser has the same issue. IMO, if the web author tries to control the playback, in practice, should not use builtin video controls. Though the desired behavior isn't specced, no harm to perform like Chrome and Edge :D

Comment 10

[Ray Lin[:ralin]]

Attachment: Bug 1402877 - Part 2. Don't let click event dispatch through media controls to video element.

(WIP) lack of mochitest to verify the event propagation fix, will add
soon.

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

Comment 11

[Mike Taylor [:miketaylr]]

Is this actually a regression?

Comment 12

[Ray Lin[:ralin]]

No, I don't think this is a regression, I see the same issue on 52 as well.

Comment 13

[Ray Lin[:ralin]]

Attachment: Bug 1402877 - Part 1. Refactor mochitest test_videocontrols.html to use add_task().

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

Comment 14

[Ray Lin[:ralin]]

Comment on attachment 8912135 [details]
Bug 1402877 - Part 2. Don't let click event dispatch through media controls to video element.

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

Comment 15

[Ray Lin[:ralin]]

Comment on attachment 8917767 [details]
Bug 1402877 - Part 1. Refactor mochitest test_videocontrols.html to use add_task().

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

Comment 16

[Ray Lin[:ralin]]

Comment on attachment 8912135 [details]
Bug 1402877 - Part 2. Don't let click event dispatch through media controls to video element.

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

Comment 17

[Ray Lin[:ralin]]

Comment on attachment 8912135 [details]
Bug 1402877 - Part 2. Don't let click event dispatch through media controls to video element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183514/diff/3-4/

Comment 18

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

Comment on attachment 8917767 [details]
Bug 1402877 - Part 1. Refactor mochitest test_videocontrols.html to use add_task().

https://reviewboard.mozilla.org/r/188710/#review197292

::: toolkit/content/tests/widgets/test_videocontrols.html:66
(Diff revision 2)
> -    .getAnonymousElementByAttribute(videocontrols, aName, aValue);
> -}
>  
> -function isMuteButtonMuted() {
> -  var muteButton = getButtonByAttribute("anonid", "muteButton");
> +async function isMuteButtonMuted() {
> +  const muteButton = getAnonElementWithinVideoByAttribute(video, "anonid", "muteButton");
> +  await new Promise(SimpleTest.executeSoon);

Why is this needed now?

::: toolkit/content/tests/widgets/test_videocontrols.html:72
(Diff revision 2)
>    return muteButton.getAttribute("muted") === "true";
>  }
>  
> -function isVolumeSliderShowingCorrectVolume(expectedVolume) {
> -  var volumeControl = getButtonByAttribute("anonid", "volumeControl");
> -  is(+volumeControl.value, expectedVolume * 100,
> +async function isVolumeSliderShowingCorrectVolume(expectedVolume) {
> +  const volumeControl = getAnonElementWithinVideoByAttribute(video, "anonid", "volumeControl");
> +  await new Promise(SimpleTest.executeSoon);

Same question as above.

Comment 19

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

Comment on attachment 8912135 [details]
Bug 1402877 - Part 2. Don't let click event dispatch through media controls to video element.

https://reviewboard.mozilla.org/r/183514/#review197310

::: toolkit/content/widgets/videocontrols.xml:1815
(Diff revision 4)
>              mozSystemGroup: true
>            });
>          }
>  
> +        // Prevent any click event within controlBar from dispatching through to video.
> +        this.controlBar.addEventListener("click", e => e.stopPropagation());

Can you use the addListener function to add this event listener so it will get cleaned up with the others?

Comment 20

[Ray Lin[:ralin]]

Comment on attachment 8917767 [details]
Bug 1402877 - Part 1. Refactor mochitest test_videocontrols.html to use add_task().

https://reviewboard.mozilla.org/r/188710/#review197292

Thanks for reviewing :)

> Why is this needed now?

Because we used to do those checks within SimpleTest.executeSoon, so I think it's fine to move the timeout in the helper functions.
http://searchfox.org/mozilla-central/rev/d30462037ffea383e74c42542c820cf65b2b144e/toolkit/content/tests/widgets/test_videocontrols.html#345-346,354-355

Comment 21

[Ray Lin[:ralin]]

Comment on attachment 8917767 [details]
Bug 1402877 - Part 1. Refactor mochitest test_videocontrols.html to use add_task().

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

Comment 22

[Ray Lin[:ralin]]

Comment on attachment 8912135 [details]
Bug 1402877 - Part 2. Don't let click event dispatch through media controls to video element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183514/diff/4-5/

Comment 23

[Ray Lin[:ralin]]

Comment on attachment 8912135 [details]
Bug 1402877 - Part 2. Don't let click event dispatch through media controls to video element.

https://reviewboard.mozilla.org/r/183514/#review197310

> Can you use the addListener function to add this event listener so it will get cleaned up with the others?

fixed. Thanks for pointing this out.

Comment 24

[Ray Lin[:ralin]]

TH result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc909630433404d83a1f5116ce9a0ed2ee2e6bf9

Thanks.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aa39d335365a
Part 1. Refactor mochitest test_videocontrols.html to use add_task(). r=jaws
https://hg.mozilla.org/integration/autoland/rev/8dc86e170ef4
Part 2. Don't let click event dispatch through controlBar to video element. r=jaws

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing a11y accessible/tests/mochitest/actions/test_media.html:

https://hg.mozilla.org/integration/autoland/rev/75a8c05fb7d2519a5ba66eaa6c967d374f1dbdbe
https://hg.mozilla.org/integration/autoland/rev/c396dc1fb2c53b6dda438a6d61aef7f1513e9a42

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8dc86e170ef476bf89b48789ed133282a11ca8b7&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=139180991&repo=autoland

[task 2017-10-24T13:56:20.517Z] 13:56:20     INFO - TEST-START | accessible/tests/mochitest/actions/test_media.html
[task 2017-10-24T13:56:20.824Z] 13:56:20     INFO - GECKO(1097) | [1097, Main Thread] WARNING: g_type_add_interface_static: assertion 'g_type_parent (interface_type) == G_TYPE_INTERFACE' failed: 'glib warning', file /builds/worker/workspace/build/src/toolkit/xre/nsSigHandlers.cpp, line 141
[task 2017-10-24T13:56:20.824Z] 13:56:20     INFO - GECKO(1097) | (firefox:1097): GLib-GObject-CRITICAL **: g_type_add_interface_static: assertion 'g_type_parent (interface_type) == G_TYPE_INTERFACE' failed
[task 2017-10-24T13:56:20.865Z] 13:56:20     INFO - TEST-INFO | started process screentopng
[task 2017-10-24T13:56:21.492Z] 13:56:21     INFO - TEST-INFO | screentopng: exit 0
[task 2017-10-24T13:56:21.493Z] 13:56:21     INFO - Buffered messages logged at 13:56:20
[task 2017-10-24T13:56:21.493Z] 13:56:21     INFO - must wait for load
[task 2017-10-24T13:56:21.494Z] 13:56:21     INFO - TEST-FAIL | accessible/tests/mochitest/actions/test_media.html | Focus test are disabled until bug 494175 is fixed. 
[task 2017-10-24T13:56:21.495Z] 13:56:21     INFO - Buffered messages finished
[task 2017-10-24T13:56:21.495Z] 13:56:21     INFO - TEST-UNEXPECTED-FAIL | accessible/tests/mochitest/actions/test_media.html | uncaught exception - NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIAccessible.previousSibling] at doTest@chrome://mochitests/content/a11y/accessible/tests/mochitest/actions/test_media.html:61:11
[task 2017-10-24T13:56:21.496Z] 13:56:21     INFO - setTimeout handler*SimpleTest_setTimeoutShim@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:676:12
[task 2017-10-24T13:56:21.496Z] 13:56:21     INFO - waitForDocLoad/<@chrome://mochitests/content/a11y/accessible/tests/mochitest/common.js:165:9
[task 2017-10-24T13:56:21.497Z] 13:56:21     INFO - setTimeout handler*SimpleTest_setTimeoutShim@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:676:12
[task 2017-10-24T13:56:21.497Z] 13:56:21     INFO - waitForDocLoad@chrome://mochitests/content/a11y/accessible/tests/mochitest/common.js:156:5
[task 2017-10-24T13:56:21.501Z] 13:56:21     INFO - focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
[task 2017-10-24T13:56:21.502Z] 13:56:21     INFO - 
[task 2017-10-24T13:56:21.503Z] 13:56:21     INFO - simpletestOnerror@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1652:11
[task 2017-10-24T13:56:21.504Z] 13:56:21     INFO - OnErrorEventHandlerNonNull*@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1632:1
[task 2017-10-24T13:56:21.505Z] 13:56:21     INFO - GECKO(1097) | JavaScript error: chrome://mochitests/content/a11y/accessible/tests/mochitest/actions/test_media.html, line 61: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIAccessible.previousSibling]
[task 2017-10-24T13:56:21.507Z] 13:56:21     INFO - GECKO(1097) | MEMORY STAT | vsize 20974097MB | residentFast 1066MB
[task 2017-10-24T13:56:21.508Z] 13:56:21     INFO - TEST-OK | accessible/tests/mochitest/actions/test_media.html | took 360ms

Comment 27

[Ray Lin[:ralin]]

Thank you Sebastian.

---
I didn't know adding an event handler on div#controlBar will also modify the accessible tree. I'll try to find a way dealing with event propagation and ensure the accessible tree.

Comment 28

[Ray Lin[:ralin]]

Attachment: Bug 1402877 - Part 3. Don't wait for click event being dispatched to media when media controls is present.

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

Comment 29

[Ray Lin[:ralin]]

Comment on attachment 8917767 [details]
Bug 1402877 - Part 1. Refactor mochitest test_videocontrols.html to use add_task().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188710/diff/3-4/

Comment 30

[Ray Lin[:ralin]]

Comment on attachment 8912135 [details]
Bug 1402877 - Part 2. Don't let click event dispatch through media controls to video element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183514/diff/5-6/

Comment 31

[alexander :surkov (:asurkov)]

Comment on attachment 8921782 [details]
Bug 1402877 - Part 3. Don't wait for click event being dispatched to media when media controls is present.

https://reviewboard.mozilla.org/r/192808/#review198190

Comment 32

[Ray Lin[:ralin]]

Thank you :surkov

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6e019955fb8c
Part 1. Refactor mochitest test_videocontrols.html to use add_task(). r=jaws
https://hg.mozilla.org/integration/autoland/rev/b93c18c066e5
Part 2. Don't let click event dispatch through media controls to video element. r=jaws
https://hg.mozilla.org/integration/autoland/rev/34b74412c07d
Part 3. Don't wait for click event being dispatched to media when media controls is present. r=surkov

Comment 34

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6e019955fb8c
https://hg.mozilla.org/mozilla-central/rev/b93c18c066e5
https://hg.mozilla.org/mozilla-central/rev/34b74412c07d

Comment 35

[Sylvestre Ledru [:Sylvestre]]

Too late for 57.

Comment 36

[ccd0]

This is a subset of
https://bugzilla.mozilla.org/show_bug.cgi?id=1032246
which is only partly fixed by these changes. Still broken:
- Clicks on control bar fire default events on parent elements (e.g. video inside a link). This is perhaps the most annoying case as a user since attempting to control such a video results in a link click. It may technically be a separate bug from 1032246.
- Click event handlers on the capture phase still fire.
- The mousedown and mouseup events still fire.

Comment 37

[Ray Lin[:ralin]]

Hi :smaug

In Chrome, click over the video control intersection doesn't fire any click event in both capture/bubble phases, but I don't think we can do anything in our <videocontrols> binding to behave like Chrome, at least we aren't aware of click event in capture phase until it hit our binding. Would you mind give me some thoughts about how we can fix the broken parts mentioned in comment 36? Thanks!

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

So Chrome is doing something against the specs?

We want to hide some click events?
HTMLMediaElement::GetEventTargetParent could set aVisitor.mCanHandle = false;