Closed
Bug 1328061
Opened 8 years ago
Closed 8 years ago
Video controls break if I drag scrubber to the right twice
Categories
(Toolkit :: Video/Audio Controls, defect)
Toolkit
Video/Audio Controls
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.
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)
Assignee | ||
Comment 3•8 years ago
|
||
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)
Comment 5•8 years ago
|
||
I can confirm this bug. With the STR from comment 0, I can successfully break the play button.
Assignee | ||
Comment 6•8 years ago
|
||
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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•8 years ago
|
||
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•8 years ago
|
||
(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.
Assignee | ||
Comment 10•8 years ago
|
||
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•8 years ago
|
||
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 hidden (mozreview-request) |
Comment 14•8 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 16•8 years ago
|
||
comment added, thank you :Gijs!
Status: NEW → ASSIGNED
Keywords: checkin-needed
Comment 17•8 years ago
|
||
Ray, there still one open issue in mozreview, can this be fixed so that we can use the autolander ? Thanks!
Flags: needinfo?(ralin)
Assignee | ||
Comment 18•8 years ago
|
||
(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 19•8 years ago
|
||
mozreview-review |
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+
Comment 20•8 years ago
|
||
bugherder landing |
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a4c1b6c15a2
Keywords: checkin-needed
Comment 21•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0a4c1b6c15a2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 22•8 years ago
|
||
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
Updated•8 years ago
|
Updated•8 years ago
|
status-firefox51:
--- → unaffected
status-firefox52:
--- → unaffected
You need to log in
before you can comment on or make changes to this bug.
Description
•