mouseout should hide controls immediately, not after a delay

RESOLVED FIXED in mozilla14

Status

()

Toolkit
Video/Audio Controls
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Dolske, Assigned: jaws)

Tracking

Trunk
mozilla14
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox11 affected, firefox12 affected, firefox13- affected)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

7 years ago
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.
Created attachment 586778 [details]
Video showing reason for delayed fade out

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).
Created attachment 586779 [details] [diff] [review]
Patch for bug 716171

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)
Created attachment 587130 [details] [diff] [review]
Alternate patch for bug 716171

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

7 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...
(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

7 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.
(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.
Created attachment 589681 [details] [diff] [review]
Patch for bug 716171

(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)
review ping?
(Reporter)

Comment 10

7 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

7 years ago
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?
Attachment #589681 - Attachment is obsolete: true
(Reporter)

Comment 12

6 years ago
Patch ping? :)
(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.
status-firefox11: --- → affected
status-firefox12: --- → affected
status-firefox13: --- → affected
tracking-firefox13: --- → ?
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.
Created attachment 605086 [details] [diff] [review]
Patch for bug

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)
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.
tracking-firefox13: ? → -
(Reporter)

Comment 17

6 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?
(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

6 years ago
Attachment #605086 - Flags: review?(dolske) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/51f25cd0e1b4
Flags: in-testsuite-
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/51f25cd0e1b4
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.