1328061 - Video controls break if I drag scrubber to the right twice

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 53, 32bit, ID 20161119030204 (2016-11-19)
STR_1:
1. Open attached "testcase 1"
2. Pause the video
3. Move mouse to the center of timeline, hold left mouse button, move mouse to the right,
   ~50px to the right from the right border of timeline, release left mouse button.
4. Repeat Step 3
5. Click Play button.
6. Wait 3 seconds, then move mouse a bit
7.(bonus) Click Play button

AR:  Step 6 - video stops, timeline looks broken. Step 7 - Play button doesn't work
ER:  Video controls shouldn't break

This is regression from bug 1271765. Regression range:
> https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=ec7fb4f14d3ec23ded7eea40ff49ebbcbec6bde1&tochange=8d2eecb7ea5a16e02862dd326ce4519082ce9901@ Ray Lin[:ralin]:
It seems that this is a regresion caused by your change. Please have a look.

Comment 1

[arni2033]

@ Ray Lin[:ralin]:
It seems that this is a regresion caused by your change. Please have a look.

Comment 3

[Ray Lin[:ralin]]

I tried the STR but I'm afraid that I could not reproduce it on my Windows10_Nightly_64bit_20170103
Could you attach a screenshot of Step 6? Thank you :)

Comment 4

[arni2033]

Attachment: 0_2017.01.04 19-21-03.webm

As you can see, the video plays a bit, but then stops once I move mouse a bit.

Comment 5

[Markus Stange [:mstange]]

I can confirm this bug. With the STR from comment 0, I can successfully break the play button.

Comment 6

[Ray Lin[:ralin]]

Thank you arni2033, :mstange :)

By watching the log, I noticed that the dragging state doesn't change correctly. We start dragging in an arbitrary position and ends up at the same place(in this case, the end of timeline), the input[type="range"] doesn't trigger `change` event, which is supposed to restore dragging state back to false.

To fix the issue, I guess we should not rely on `change` event to determine whether we'd already finished dragging. We will need another variable to save last position to be extra information for managing dragging state.

Comment 7

[Ray Lin[:ralin]]

Attachment: Bug 1328061 - restore dragging state when video seeks to the same point twice.

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

Comment 8

[Ray Lin[:ralin]]

Hi Jared,


Right now the dragging state toggle starts from `input` being triggered and end with a `change` event. I was wrong about that not every dragging commit a `change` to input, such as when the before|after dragging values are the same. In this case, play button refrained from playing due to incorrect dragging state. 

Do you think it's a good idea to add `mouseup` listener to input to deal with this case? 

Thank you :)

Comment 9

[:Gijs (he/him)]

(In reply to Ray Lin[:ralin] from comment #8)
> Hi Jared,
> 
> 
> Right now the dragging state toggle starts from `input` being triggered and
> end with a `change` event. I was wrong about that not every dragging commit
> a `change` to input, such as when the before|after dragging values are the
> same. In this case, play button refrained from playing due to incorrect
> dragging state. 
> 
> Do you think it's a good idea to add `mouseup` listener to input to deal
> with this case? 
> 
> Thank you :)

the mouseup might not occur over the video. Can you use a dragend listener instead? I believe that should fire even if the drag ends outside the element in which it started.

Comment 10

[Ray Lin[:ralin]]

I tried but `dragend` event seems not fired for input[type='range'], on which I thought it should work as well. You reminded me the issue like the drag ends outside, so I did few tests on `mouseup`:

- drag and release mouse over input element
- drag and release mouse over video element
- drag and release mouse outside video 
- drag and release mouse outside Firefox window

all of them trigger `mouseup` event, so perhaps we should use `mouseup` in this case.

Comment 11

[Markus Stange [:mstange]]

dragend only fires if you entered drag mode, i.e. the state where you get dragover events instead of mousemove events, and where there's a small preview image attached to the mouse cursor and there exists some meta information about the transfer data. So I agree that dragend is not the right event to listen for here.

mouseup fires even if you release the mouse outside of the scrubber due to mouse capturing. The mouse capturing probably happens from the nsIPresShell::SetCapturingContent call in nsSliderFrame::DragThumb but I haven't verified that.

Comment 12

[:Gijs (he/him)]

Ah, I'm sorry for getting things confused - mouseup sounds sensible then!

Comment 13

[Ray Lin[:ralin]]

Comment on attachment 8826064 [details]
Bug 1328061 - restore dragging state when video seeks to the same point twice.

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

Comment 14

[:Gijs (he/him)]

Comment on attachment 8826064 [details]
Bug 1328061 - restore dragging state when video seeks to the same point twice.

https://reviewboard.mozilla.org/r/104110/#review106262

::: toolkit/content/widgets/videocontrols.xml:1748
(Diff revision 2)
>          });
>  
>          if (!this.videocontrols.isTouchControls) {
>            addListener(this.scrubber, "input", this.onScrubberInput);
>            addListener(this.scrubber, "change", this.onScrubberChange);
> +          addListener(this.scrubber, "mouseup", this.onScrubberChange);

Nit: can you add a comment above this line indicating why we have this listener in addition to the change one?

Comment 15

[Ray Lin[:ralin]]

Comment on attachment 8826064 [details]
Bug 1328061 - restore dragging state when video seeks to the same point twice.

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

Comment 16

[Ray Lin[:ralin]]

comment added, thank you :Gijs!

Comment 17

[Carsten Book [:Tomcat]]

Ray, there still one open issue in mozreview, can this be fixed so that we can use the autolander ? Thanks!

Comment 18

[Ray Lin[:ralin]]

(In reply to Carsten Book [:Tomcat] from comment #17)
> Ray, there still one open issue in mozreview, can this be fixed so that we
> can use the autolander ? Thanks!

Sorry, I forgot again... issue closed, thanks :)

Comment 19

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

Comment on attachment 8826064 [details]
Bug 1328061 - restore dragging state when video seeks to the same point twice.

https://reviewboard.mozilla.org/r/104110/#review106702

It would be good to include a test with this. Could you do that in a follow-up?

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a4c1b6c15a2

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/0a4c1b6c15a2

Comment 22

[Hani Yacoub, Desktop QA]

Tested this issue on Ubuntu 16.04 x64, Windows 10 x 64, Mac OS X 10.12, on the latest Firefox Nightly and I can confirm that the issue is not reproducible anymore.