Closed
Bug 716171
Opened 13 years ago
Closed 12 years ago
mouseout should hide controls immediately, not after a delay
Categories
(Toolkit :: Video/Audio Controls, defect)
Toolkit
Video/Audio Controls
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: Dolske, Assigned: jaws)
References
Details
Attachments
(1 file, 5 obsolete files)
5.79 KB,
patch
|
Dolske
:
review+
|
Details | Diff | Splinter Review |
Bug 699719 broke this... 1) move mouse over a video 2) (controls immediately fade in) 3) move mouse out from video Expected: 4) (controls immediately fade out) Actual: 4) (controls don't fade out until the delay timer fires. From onMouseInOut changes from that bug: - if (!isMouseOver) + if (!isMouseOver) { this.adjustControlSize(); - this.startFade(this.controlBar, isMouseOver); + // Setting a timer here to handle the case where + // the mouse leaves the video from hovering over + // the controls. + this._hideControlsTimeout = + setTimeout(this._hideControlsFn, ...) + } The comment doesn't make sense to me. Even hovering over the controls should result in a timer starting upon the last mousemove over the controls. When the mouseout fires, the pointer has left the building and the controls should immediately go away.
Assignee | ||
Comment 1•13 years ago
|
||
This video shows the reason why the delayed fade out was added (see around the 5-8 second mark). (1) Mouse stops near the edge of a video (2) Controls disappear (3) Mouse moves off of the video This behavior will result in a brief flash of the controls appearing then disappearing. I think it is better to just let the controls timeout, as this is the same behavior of the YouTube video player (just that their timeout is a little shorter than ours).
Assignee | ||
Comment 2•13 years ago
|
||
This patch changes the video controls towards the behavior that you have requested, but I don't think we should take it. I would rather that we tweak the time delay on fading the controls out.
Attachment #586779 -
Flags: review?(dolske)
Assignee | ||
Comment 3•13 years ago
|
||
This patch provides an alternate fix for the bug by halving the delay down to 1 second.
Attachment #587130 -
Flags: review?(dolske)
Reporter | ||
Comment 4•13 years ago
|
||
(In reply to Jared Wein [:jwein and :jaws] from comment #1) > Created attachment 586778 [details] > Video showing reason for delayed fade out Oy, don't use this as an example again. ;-) I had to watch it a few times before I realized I was looking at the wrong mouse pointer! Also, this ironically illustrates an (uncommon) case of how the delay can be confusing... I moved the mouse out from the video after clicking play, but the long delay makes there be 2 sets of video controls visible at the same time (the real controls, and the controls recorded in the video)! > This behavior will result in a brief flash of the controls appearing then > disappearing. Ah, I see the issue now... 1) Hover over video 2) Wait for controls to fade away after timeout 3) Enjoy the movie 4) Go to move mouse to, say, click back button or URL bar 5) Controls flash into view and disappear, and are distracting because intended mouse target is actually unrelated and far away. An alternate solution might be to only have a slight delay when the mouse moves while the controls are hidden...
Assignee | ||
Comment 5•13 years ago
|
||
(In reply to Justin Dolske [:Dolske] from comment #4) > (In reply to Jared Wein [:jwein and :jaws] from comment #1) > > Created attachment 586778 [details] > > Video showing reason for delayed fade out > > Oy, don't use this as an example again. ;-) I had to watch it a few times > before I realized I was looking at the wrong mouse pointer! Yeah, it's a terrible sample video :P
Status: NEW → ASSIGNED
Reporter | ||
Comment 6•13 years ago
|
||
I think the delay is worse than the flicker. Want to try a hack/patch for the suggestion I made in comment 4? Seems like that might fix both issues.
Assignee | ||
Comment 7•13 years ago
|
||
(In reply to Justin Dolske [:Dolske] from comment #6) > I think the delay is worse than the flicker. > > Want to try a hack/patch for the suggestion I made in comment 4? Seems like > that might fix both issues. Yeah definitely.
Assignee | ||
Comment 8•12 years ago
|
||
(In reply to Justin Dolske [:Dolske] from comment #6) > I think the delay is worse than the flicker. > > Want to try a hack/patch for the suggestion I made in comment 4? Seems like > that might fix both issues. I've added a 500ms delay for showing the controls when moving the mouse on the video if the controls were hidden by the mouse stalling on the video.
Attachment #586779 -
Attachment is obsolete: true
Attachment #587130 -
Attachment is obsolete: true
Attachment #586779 -
Flags: review?(dolske)
Attachment #587130 -
Flags: review?(dolske)
Attachment #589681 -
Flags: review?(dolske)
Assignee | ||
Comment 9•12 years ago
|
||
review ping?
Reporter | ||
Comment 10•12 years ago
|
||
Comment on attachment 589681 [details] [diff] [review] Patch for bug 716171 Uhh... >+ HIDE_CONTROLS_TIMEOUT_MS: 500, ... > HIDE_CONTROLS_TIMEOUT_MS : 2000, ... >+ this._showControlsTimeout = setTimeout(this._showControlsFn, this.SHOW_CONTROLS_TIMEOUT_MS); I think you meant to add SHOW_CONTROLS_TIMEOUT_MS. :-)
Attachment #589681 -
Flags: review?(dolske) → review-
Reporter | ||
Comment 11•12 years ago
|
||
I went ahead and de-bittrotted your patch (and got it working, see last comment). Looks ok, except there's still another bug (or I screwed something up)... If I move the pointer within the video after the controls have timed out, sometimes the controls with stay visible (i.e., not fade back out after a few seconds). Seems inconsistent, maybe something timing related with the two timers?
Attachment #589681 -
Attachment is obsolete: true
Reporter | ||
Comment 12•12 years ago
|
||
Patch ping? :)
Assignee | ||
Comment 13•12 years ago
|
||
(In reply to Justin Dolske [:Dolske] from comment #11) > Created attachment 594616 [details] [diff] [review] > De-bitrotted last patch > > I went ahead and de-bittrotted your patch (and got it working, see last > comment). > > Looks ok, except there's still another bug (or I screwed something up)... If > I move the pointer within the video after the controls have timed out, > sometimes the controls with stay visible (i.e., not fade back out after a > few seconds). Seems inconsistent, maybe something timing related with the > two timers? I should have replied weeks ago... I saw the same issue as you and I didn't understand why it was happening. I will try to spend some more time looking in to this today or on Monday.
Assignee | ||
Updated•12 years ago
|
status-firefox11:
--- → affected
status-firefox12:
--- → affected
status-firefox13:
--- → affected
tracking-firefox13:
--- → ?
Assignee | ||
Comment 14•12 years ago
|
||
I'm marking for tracking-firefox 13 because it would be good to get this bug fix out to users earlier rather than later. Since the bug wasn't fixed for 11 and 12, delaying for one more release wouldn't be that bad either.
Assignee | ||
Comment 15•12 years ago
|
||
This fixes the issue what we were both seeing before. The _hideControlsTimeout was getting cleared in the startFade function when the controls were appearing.
Attachment #586778 -
Attachment is obsolete: true
Attachment #594616 -
Attachment is obsolete: true
Attachment #605086 -
Flags: review?(dolske)
Comment 16•12 years ago
|
||
We wouldn't be too concerned with shipping Firefox without this fix, but would definitely consider taking it on Aurora 13 if deemed low risk.
Reporter | ||
Comment 17•12 years ago
|
||
Comment on attachment 605086 [details] [diff] [review] Patch for bug Review of attachment 605086 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/content/widgets/videocontrols.xml @@ +853,5 @@ > // Hide the controls if the mouse cursor is left on top of the video > // but above the control bar and if the click-to-play overlay is hidden. > + if ((this._controlsHiddenByTimeout || > + event.clientY < this.controlBar.getBoundingClientRect().top) && > + this.clickToPlay.hidden) { Oh, I guess the rect is empty when it's hidden? Wonder if we should instead just precompute it (once) down at the bottom of setupInitialState(), as we do with various button widths. @@ -885,5 @@ > }, > > startFade : function (element, fadeIn, immediate) { > if (element.className == "controlBar" && fadeIn) { > - clearTimeout(this._hideControlsTimeout); Hmm. This makes me worry about breaking other cases where we might be relying on an immediate fade-in canceling any delayed fade-out. At least, I'd need to reread through to make sure that's ok. Maybe having just one visibility-change timeout would be helpful, but that also vaguely hurts my head. Thoughts?
Assignee | ||
Comment 18•12 years ago
|
||
(In reply to Justin Dolske [:Dolske] from comment #17) > Comment on attachment 605086 [details] [diff] [review] > Patch for bug > > Review of attachment 605086 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/content/widgets/videocontrols.xml > @@ +853,5 @@ > > // Hide the controls if the mouse cursor is left on top of the video > > // but above the control bar and if the click-to-play overlay is hidden. > > + if ((this._controlsHiddenByTimeout || > > + event.clientY < this.controlBar.getBoundingClientRect().top) && > > + this.clickToPlay.hidden) { > > Oh, I guess the rect is empty when it's hidden? > > Wonder if we should instead just precompute it (once) down at the bottom of > setupInitialState(), as we do with various button widths. It's a little harder to compute it as it is based on current height of the video, and so if the video changes dimensions while playing then the top of the control bar will have a different value. > @@ -885,5 @@ > > }, > > > > startFade : function (element, fadeIn, immediate) { > > if (element.className == "controlBar" && fadeIn) { > > - clearTimeout(this._hideControlsTimeout); > > Hmm. This makes me worry about breaking other cases where we might be > relying on an immediate fade-in canceling any delayed fade-out. At least, > I'd need to reread through to make sure that's ok. Maybe having just one > visibility-change timeout would be helpful, but that also vaguely hurts my > head. Thoughts? No new thoughts here. I'd pay a dollar for some automated tests in this area though.
Reporter | ||
Updated•12 years ago
|
Attachment #605086 -
Flags: review?(dolske) → review+
Assignee | ||
Comment 19•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/51f25cd0e1b4
Flags: in-testsuite-
Target Milestone: --- → mozilla14
Comment 20•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/51f25cd0e1b4
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•