Closed Bug 1402877 Opened 2 years ago Closed 2 years ago

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

Categories

(Toolkit :: Video/Audio Controls, defect)

57 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- affected
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: jya, Assigned: ralin)

Details

Attachments

(3 files)

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
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.
Flags: needinfo?(jaws)
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.
Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(jaws)
Resolution: --- → INVALID
Actually, this has nothing to do with bug 693014, but that the site has an onClick handler that will pause the content.

my bad.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
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?
Flags: needinfo?(jyavenard)
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).
Status: REOPENED → NEW
Summary: Seeking using the time slider will pause the video. → Clicking on the time slider to seek dispatches a "click" event to the video element
(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
Flags: needinfo?(jyavenard)
(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?
Flags: needinfo?(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.
Assignee: nobody → ralin
Flags: needinfo?(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
No, I don't think this is a regression, I see the same issue on 52 as well.
Keywords: regression
Status: NEW → ASSIGNED
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.
Attachment #8917767 - Flags: review?(jaws) → review+
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?
Attachment #8912135 - Flags: review?(jaws) → review+
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 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.
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
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
Flags: needinfo?(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.
Flags: needinfo?(ralin)
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
Attachment #8921782 - Flags: review?(surkov.alexander) → review+
Thank you :surkov
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
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.
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!
Flags: needinfo?(bugs)
So Chrome is doing something against the specs?

We want to hide some click events?
HTMLMediaElement::GetEventTargetParent could set aVisitor.mCanHandle = false;
Flags: needinfo?(bugs)
You need to log in before you can comment on or make changes to this bug.