716171 - mouseout should hide controls immediately, not after a delay

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Justin Dolske [:Dolske]]

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.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: 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).

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: 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.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Alternate patch for bug 716171

This patch provides an alternate fix for the bug by halving the delay down to 1 second.

Comment 4

[Justin Dolske [:Dolske]]

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

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

(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

Comment 6

[Justin Dolske [:Dolske]]

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.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: 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.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

review ping?

Comment 10

[Justin Dolske [:Dolske]]

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. :-)

Comment 11

[Justin Dolske [:Dolske]]

Attachment: 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?

Comment 12

[Justin Dolske [:Dolske]]

Patch ping? :)

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: 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.

Comment 16

[Alex Keybl [:akeybl]]

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.

Comment 17

[Justin Dolske [:Dolske]]

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?

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51f25cd0e1b4

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/51f25cd0e1b4