Closed Bug 1328061 Opened 3 years ago Closed 3 years ago

Video controls break if I drag scrubber to the right twice

Categories

(Toolkit :: Video/Audio Controls, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox51 --- unaffected
firefox52 --- unaffected
firefox53 --- verified

People

(Reporter: arni2033, Assigned: ralin)

References

Details

(Keywords: regression)

Attachments

(3 files)

>>>   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.
No longer blocks: 1277113
Component: Untriaged → Video/Audio Controls
Product: Firefox → Toolkit
@ Ray Lin[:ralin]:
It seems that this is a regresion caused by your change. Please have a look.
Flags: needinfo?(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 :)
Flags: needinfo?(ralin) → needinfo?(arni2033)
As you can see, the video plays a bit, but then stops once I move mouse a bit.
Flags: needinfo?(arni2033) → needinfo?(ralin)
I can confirm this bug. With the STR from comment 0, I can successfully break the play button.
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.
Assignee: nobody → ralin
Flags: needinfo?(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 :)
(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.
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.
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.
Ah, I'm sorry for getting things confused - mouseup sounds sensible then!
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?
Attachment #8826064 - Flags: review?(gijskruitbosch+bugs) → review+
comment added, thank you :Gijs!
Status: NEW → ASSIGNED
Keywords: checkin-needed
Ray, there still one open issue in mozreview, can this be fixed so that we can use the autolander ? Thanks!
Flags: needinfo?(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 :)
Flags: needinfo?(ralin)
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?
Attachment #8826064 - Flags: review?(jaws) → review+
https://hg.mozilla.org/mozilla-central/rev/0a4c1b6c15a2
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.