Summary: | Controls never hide in full screen after user stops moving mouse | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brian Weinstein <bweinstein> | ||||||||
Component: | Media | Assignee: | 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
Brian Weinstein
2011-05-29 17:43:25 PDT
Created attachment 95310 [details]
[PATCH] Fix
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 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 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. (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. Created attachment 95314 [details]
[PATCH] Fix v2
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 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 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. Created attachment 95316 [details]
Patch
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 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 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 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. Committed r87661: <http://trac.webkit.org/changeset/87661> Why can't this be done with just CSS? (In reply to comment #16) > Why can't this be done with just CSS? It probably can be. Revision r87661 cherry-picked into qtwebkit-2.2 with commit ec7a9b6 <http://gitorious.org/webkit/qtwebkit/commit/ec7a9b6> |