Bug 61715 – Controls never hide in full screen after user stops moving mouse

• Component: Media
• Status: RESOLVED FIXED

[Brian Weinstein]

Full screen HTML5 video at vimeo, reveal the controls by moving the mouse, they never go away again!

* STEPS TO REPRODUCE
1. Go to vimeo.com
2. Play the front page video
3. Take it full screen.
4. Notice the controls are hidden.
5. Move the mouse to reveal them.
6. They never re-hide.

* RESULTS
They should re-hide!

<rdar://problem/9522182>

[Brian Weinstein]

Created attachment 95310 [details]
[PATCH] Fix

[Jer Noble]

Is there any way to hide the mouse pointer from within WebCore?  If not, that's fine (because the user can park their mouse in the corner).

[Darin Adler]

Comment on attachment 95310 [details]
[PATCH] Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=95310&action=review

> Source/WebCore/html/HTMLMediaElement.cpp:1649
> +    // If we have a timer running to hide the controls, stop and restart it to start out three second
> +    // period over again.
> +    if (m_hideFullscreenControlsTimer.isActive())
> +        m_hideFullscreenControlsTimer.stop();

Doesn’t startOneShot already do this?

> Source/WebCore/html/HTMLMediaElement.cpp:2412
> +                    startHideFullscreenControlsTimer();

What if this timer is still running when some other code calls makeOpaque. I would expect to see more calls to stop the timer elsewhere.

[Brian Weinstein]

Comment on attachment 95310 [details]
[PATCH] Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=95310&action=review

>> Source/WebCore/html/HTMLMediaElement.cpp:1649
>> +        m_hideFullscreenControlsTimer.stop();
> 
> Doesn’t startOneShot already do this?

I wasn't 100% sure from looking at the Timer code, but I believe so on closer inspection.

>> Source/WebCore/html/HTMLMediaElement.cpp:2412
>> +                    startHideFullscreenControlsTimer();
> 
> What if this timer is still running when some other code calls makeOpaque. I would expect to see more calls to stop the timer elsewhere.

makeOpaque is only called in one other places in HTMLMediaElement - when handling mouseOver events. I can discuss with Jer other places to stop the timer, I'm not sure the other places we would want to.

[Brian Weinstein]

(In reply to comment #4)
> (From update of attachment 95310 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95310&action=review
> 
> >> Source/WebCore/html/HTMLMediaElement.cpp:1649
> >> +        m_hideFullscreenControlsTimer.stop();
> > 
> > Doesn’t startOneShot already do this?
> 
> I wasn't 100% sure from looking at the Timer code, but I believe so on closer inspection.

Testing confirms you are correct. Fixed to remove the unnecessary code.

[Brian Weinstein]

Created attachment 95314 [details]
[PATCH] Fix v2

[Darin Adler]

Comment on attachment 95314 [details]
[PATCH] Fix v2

View in context: https://bugs.webkit.org/attachment.cgi?id=95314&action=review

This is missing the mouseover and mouseout part. I see no reason to land the partial fix.

> Source/WebCore/html/HTMLMediaElement.cpp:2417
>                      mediaControls()->makeOpaque();

We should stop the timer here, when we move over the controls.

> Source/WebCore/html/HTMLMediaElement.cpp:2419
>                  m_mouseOver = false;

And we should start the timer here.

[Jer Noble]

Comment on attachment 95314 [details]
[PATCH] Fix v2

View in context: https://bugs.webkit.org/attachment.cgi?id=95314&action=review

>> Source/WebCore/html/HTMLMediaElement.cpp:2417
>>                      mediaControls()->makeOpaque();
> 
> We should stop the timer here, when we move over the controls.

There's not currently a way to determine here that the mouse is over the controls, since the MediaControls class doesn't give access to its innards, and MediaControls()->hovered() is apparently always returning true if the mouse is over the video element.  But if MediaControls had a new virtual function, say "bool shouldHideControls()", and MediaControlRootElement implemented it to return, "return !m_panel->hovered()", that should be enough to determine if the mouse is over the media control panel.

I've modified Brian's patch to add this to MediaControls (but not stopping the timer here, yet), and it seems to work well.

[Brian Weinstein]

Comment on attachment 95314 [details]
[PATCH] Fix v2

View in context: https://bugs.webkit.org/attachment.cgi?id=95314&action=review

>> Source/WebCore/html/HTMLMediaElement.cpp:2419
>>                  m_mouseOver = false;
> 
> And we should start the timer here.

I believe this represents whether or not the mouse is in the media element, not whether or not it is over the controls. As Jer mentioned in the comment, there currently isn't a way to determine if the mouse is in the controls.

[Jer Noble]

Created attachment 95316 [details]
Patch

[Darin Adler]

Comment on attachment 95316 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95316&action=review

> Source/WebCore/html/HTMLMediaElement.cpp:2420
> +                    if (mediaControls()->shouldHideControls())
> +                        startHideFullscreenControlsTimer();

Mousing over means we should *stop* the timer.

> Source/WebCore/html/HTMLMediaElement.cpp:2424
>                  m_mouseOver = false;
> +                stopHideFullscreenControlsTimer();

Mousing out means we should *start* the timer.

[Jer Noble]

Comment on attachment 95316 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95316&action=review

>> Source/WebCore/html/HTMLMediaElement.cpp:2420
>> +                        startHideFullscreenControlsTimer();
> 
> Mousing over means we should *stop* the timer.

What we are mousing over is not necessarily the controls.  We are mousing over the element here.  We may still want to hide the controls after the inactivity period if the mouse isn't over the controls.

>> Source/WebCore/html/HTMLMediaElement.cpp:2424
>> +                stopHideFullscreenControlsTimer();
> 
> Mousing out means we should *start* the timer.

And here, we are mousing out of the element entirely, so we are going to hide the controls the next time we get a playbackProgressTimerFired call.

[Darin Adler]

Comment on attachment 95316 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95316&action=review

OK, then assuming you tested enough, r=me

> Source/WebCore/html/shadow/MediaControlRootElement.h:99
> +    bool shouldHideControls();

Should this be marked virtual?

> Source/WebCore/html/shadow/MediaControls.h:71
> +    virtual bool shouldHideControls() = 0;

I see only one derived class implementing this. Will having this new pure virtual function break the build?

[Jer Noble]

Comment on attachment 95316 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95316&action=review

>> Source/WebCore/html/shadow/MediaControlRootElement.h:99
>> +    bool shouldHideControls();
> 
> Should this be marked virtual?

It should be.  None of the other functions in this file are, and they should all be as well.  (But later)

>> Source/WebCore/html/shadow/MediaControls.h:71
>> +    virtual bool shouldHideControls() = 0;
> 
> I see only one derived class implementing this. Will having this new pure virtual function break the build?

I checked, an no other class in WebCore inherits from this class except MediaControlRootElement.

[Jer Noble]

Committed r87661: <http://trac.webkit.org/changeset/87661>

[Dimitri Glazkov (Google)]

Why can't this be done with just CSS?

[Darin Adler]

(In reply to comment #16)
> Why can't this be done with just CSS?

It probably can be.

[Ademar Reis]

Revision r87661 cherry-picked into qtwebkit-2.2 with commit ec7a9b6 <http://gitorious.org/webkit/qtwebkit/commit/ec7a9b6>