Closed Bug 493508 Opened 16 years ago Closed 16 years ago

video <scale> thumb can jump around while it's being dragged.

Categories

(Core :: XUL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: Dolske, Assigned: Dolske)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 1 obsolete file)

The video scrubber thumb can jump around while it's being dragged, due to the interaction between where the using is dragging the thumb to and the video being slow to seek to new positions. As a video plays, we set the <scale> value to indicate the current playback position. And when the user drags the thumb, we kick off seek requests to the video. The problem is that when the current playback position updates after a seek, the user may still be dragging the thumb to some other position. So when we set the <scale>'s .value, you get the unsettling effect of the thumb jumping to some other location, and then jumping back when you move the mouse some more. I can think of 2 different ways do solve this: 1) Add code to nsSliderFrame (::AttributeChanged ?) to suppress changing the displayed position of the thumb while it's being dragged. This seems like the more general solution to the issue. The mouseup handling would also probably need updated to update .value, lest script have changed it between the last mousemove event and the mouseup? 2) Have the video controls suppress changing the <scale>'s value while the user is dragging the thumb. This avoids having script and the user fighting for control of the thumb, but still needs help from nsSliderFrame... It's grabbing control of mouse events, which seems to result in mouseup listeners never being called (I could only get mousedown working, when I tried to get this working). Maybe add another callback to nsISliderListener, so the controls can know what the drag state is, and thus not change the <scale>'s value during a drag? But I think there's still tricky mouseup logic to figure out: eg, if I start a drag, hold the mouse still, allow the video to resume play for a while, and then mouseup -- what should happen? So, I'm going to suggest #1. Neil, what do you think?
Won't the video eventually seek to the last dragged position anyway? I have a slight preference for #2, since I think it would be clearer if the slider's display always matched its attribute.
(In reply to comment #1) > Won't the video eventually seek to the last dragged position anyway? Yes, but in the interm the thumb will likely have jumped back to some previous spot in the drag, and then it will jump forward to the final drag position. So, the end result is correct, but the intermediate states are poor UX. > I have a slight preference for #2, since I think it would be clearer if the > slider's display always matched its attribute. I think I need that anyway for bug 493681, so that's probably the way to go. Does extending nsISliderListener seem like the right way?
Sounds good to me.
Attached patch Patch v.1 (obsolete) (deleted) — Splinter Review
Patch as discussed above. Also fixed bug 493681 here. I'd very much like to get this into 191, if you could make a review pass today that would be great.
Assignee: nobody → dolske
Attachment #378552 - Flags: review?(neil)
Attached patch Patch v.2 (deleted) — Splinter Review
Got review comments from Neil on IRC and made changes. Mentioned looking to see if DragThumb() should only do the callback if the GetView() bits succeed, but I *think* it's ok as is. <NeilAway> ok, so the rest looks sane to me So over to mconnor for a final official review.
Attachment #378552 - Attachment is obsolete: true
Attachment #378775 - Flags: review?(mconnor)
Attachment #378552 - Flags: review?(neil)
Comment on attachment 378775 [details] [diff] [review] Patch v.2 a191=beltzner, usual yadda yadda about trunk first
Attachment #378775 - Flags: approval1.9.1+
Comment on attachment 378775 [details] [diff] [review] Patch v.2 mconnor still for the toolkit bits.
Attachment #378775 - Flags: review?(mconnor)
Comment on attachment 378775 [details] [diff] [review] Patch v.2 yeah, looks good to go, with a comment in the idl explaining dragStateChanged
Attachment #378775 - Flags: superreview+
Attachment #378775 - Flags: review?(mconnor)
Attachment #378775 - Flags: review+
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Comment on attachment 378775 [details] [diff] [review] Patch v.2 rs=me
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: