Bug 61715

Summary: Controls never hide in full screen after user stops moving mouse
Product: WebKit Reporter: Brian Weinstein <bweinstein>
Component: MediaAssignee: Brian Weinstein <bweinstein>
Status: RESOLVED FIXED    
Severity: Normal CC: ademar, darin, dglazkov, jer.noble, mjs, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
[PATCH] Fix
none
[PATCH] Fix v2
none
Patch darin: review+

Description Brian Weinstein 2011-05-29 17:43:25 PDT
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>
Comment 1 Brian Weinstein 2011-05-29 17:46:35 PDT
Created attachment 95310 [details]
[PATCH] Fix
Comment 2 Jer Noble 2011-05-29 17:48:45 PDT
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).
Comment 3 Darin Adler 2011-05-29 17:48:51 PDT
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.
Comment 4 Brian Weinstein 2011-05-29 17:53:38 PDT
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.
Comment 5 Brian Weinstein 2011-05-29 17:56:28 PDT
(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.
Comment 6 Brian Weinstein 2011-05-29 19:21:10 PDT
Created attachment 95314 [details]
[PATCH] Fix v2
Comment 7 Darin Adler 2011-05-29 19:39:09 PDT
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.
Comment 8 Jer Noble 2011-05-29 19:45:24 PDT
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.
Comment 9 Brian Weinstein 2011-05-29 19:53:10 PDT
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.
Comment 10 Jer Noble 2011-05-29 19:55:51 PDT
Created attachment 95316 [details]
Patch
Comment 11 Darin Adler 2011-05-29 19:59:07 PDT
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.
Comment 12 Jer Noble 2011-05-29 20:09:38 PDT
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.
Comment 13 Darin Adler 2011-05-29 20:14:26 PDT
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?
Comment 14 Jer Noble 2011-05-29 20:36:04 PDT
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.
Comment 15 Jer Noble 2011-05-29 20:37:23 PDT
Committed r87661: <http://trac.webkit.org/changeset/87661>
Comment 16 Dimitri Glazkov (Google) 2011-05-31 09:08:10 PDT
Why can't this be done with just CSS?
Comment 17 Darin Adler 2011-05-31 14:38:17 PDT
(In reply to comment #16)
> Why can't this be done with just CSS?

It probably can be.
Comment 18 Ademar Reis 2011-06-03 14:01:58 PDT
Revision r87661 cherry-picked into qtwebkit-2.2 with commit ec7a9b6 <http://gitorious.org/webkit/qtwebkit/commit/ec7a9b6>