RESOLVED FIXED 61717
REGRESSION (r87622): Scrubbing a Vimeo movie when in fullscreen stops playback; no way to make it start again
https://bugs.webkit.org/show_bug.cgi?id=61717
Summary REGRESSION (r87622): Scrubbing a Vimeo movie when in fullscreen stops playbac...
Darin Adler
Reported 2011-05-29 20:08:50 PDT
REGRESSION (r87622): Scrubbing a Vimeo movie when in fullscreen stops playback; no way to make it start again
Attachments
Patch (21.90 KB, patch)
2011-05-29 20:11 PDT, Darin Adler
no flags
Patch (19.68 KB, patch)
2011-05-29 20:24 PDT, Darin Adler
no flags
Patch (37.39 KB, patch)
2011-05-29 21:53 PDT, Jer Noble
no flags
Patch (39.46 KB, patch)
2011-05-29 23:10 PDT, Jer Noble
darin: review+
Darin Adler
Comment 1 2011-05-29 20:11:23 PDT
Darin Adler
Comment 2 2011-05-29 20:24:38 PDT
Jer Noble
Comment 3 2011-05-29 21:53:55 PDT
Darin Adler
Comment 4 2011-05-29 21:55:41 PDT
Comment on attachment 95324 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95324&action=review > Source/WebCore/ChangeLog:102 > +2011-05-29 Brian Weinstein <bweinstein@apple.com> > + > + Reviewed by Darin Adler. > + > + Controls never hide in full screen after user stops moving mouse > + https://bugs.webkit.org/show_bug.cgi?id=61715 > + <rdar://problem/9522182> > + > + When we get a mouse move event in HTMLMediaElement::defaultEventHandler, and we are in full screen, > + show the media controls, and then start a timer. > + > + The timer fires 3 seconds after the user's last mouse movement (timer is restarted on every mouse > + move), and hides the controls. > + > + * html/HTMLMediaElement.cpp: > + (WebCore::HTMLMediaElement::HTMLMediaElement): Initialize our new timer. > + (WebCore::HTMLMediaElement::play): If we are in full screen mode, start our timer to hide the full screen > + controls. We don't want the user to have to move the mouse to hide them when they use the spacebar > + to play. > + (WebCore::HTMLMediaElement::startHideFullscreenControlsTimer): Starts a oneshot timer 3 seconds in the future > + if we are in full screen. > + (WebCore::HTMLMediaElement::hideFullscreenControlsTimerFired): Make sure that we are currently playing, and > + we are in full screen, and hide the controls. We don't want to hide the controls if we are paused. > + (WebCore::HTMLMediaElement::stopHideFullscreenControlsTimer): Stops the timer. > + (WebCore::HTMLMediaElement::defaultEventHandler): If we get a mouse move event and are in full screen, show the > + controls and start a timer to hide them. > + (WebCore::HTMLMediaElement::enterFullscreen): Start a timer to hide the full screen controls. The user shouldn't > + have the move the mouse once they enter full screen to hide the controls. > + (WebCore::HTMLMediaElement::exitFullscreen): Stop the timer to hide the full screen controls. > + * html/HTMLMediaElement.h: > + * html/shadow/MediaControls.h: Added pure virtual shouldHideControls() method. > + * html/shadow/MediaControlRootElement.cpp: > + (WebCore::MediaControlRootElement::playbackStopped): Stop the timer to hide the full screen controls. > + (WebCore::MediaControlRootElement::shouldHideControls): Added, only report that > + the caller should hide the controls if the panel is not hovered. > + * html/shadow/MediaControlRootElement.h: > + Extra change log entry here. Don't land that!
Simon Fraser (smfr)
Comment 5 2011-05-29 22:39:52 PDT
Comment on attachment 95324 [details] Patch Is this really fixing just bug 61717, or are there changes related to bug 61715 here too?
Jer Noble
Comment 6 2011-05-29 23:10:55 PDT
Simon Fraser (smfr)
Comment 7 2011-05-29 23:21:35 PDT
Comment on attachment 95326 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95326&action=review > Source/WebCore/html/shadow/MediaControlElements.cpp:653 > > + if (event->type() == eventNames().mouseupEvent) This could be an else
Darin Adler
Comment 8 2011-05-30 09:30:01 PDT
Comment on attachment 95326 [details] Patch We know this is a step in the right direction, but we’re having a lot of problems with the patch and so we can’t land this one. Jer will post a new one once he’s made more progress.
Darin Adler
Comment 9 2011-05-30 09:43:35 PDT
Comment on attachment 95326 [details] Patch My mistake. This is a new version of the patch. Adding back the review+
Darin Adler
Comment 10 2011-05-30 10:05:26 PDT
Comment on attachment 95326 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95326&action=review > Source/WebCore/dom/EventDispatcher.cpp:377 > + if (element->isMediaElement() && shadowRoot && shadowRoot->shadowHost() == element) There is no need to null check shadowRoot here.
Darin Adler
Comment 11 2011-05-30 10:09:54 PDT
Jer Noble
Comment 12 2011-05-30 10:30:08 PDT
Comment on attachment 95326 [details] Patch I'm away from my computer for a few hours. If this patch looks committable as-is, or if another developer wants to land it with changes, that's fine with me.
Jer Noble
Comment 13 2011-05-30 10:31:31 PDT
Comment on attachment 95326 [details] Patch Whoops, missed that commit message. Clearing cq.
Dimitri Glazkov (Google)
Comment 14 2011-05-31 09:09:00 PDT
This seems like something that should be fixed with the full-screen API spec? Shouldn't it be the case that any element that's full-screen acts in the same way?
Darin Adler
Comment 15 2011-05-31 14:40:48 PDT
(In reply to comment #14) > This seems like something that should be fixed with the full-screen API spec? Shouldn't it be the case that any element that's full-screen acts in the same way? Currently, making a media element full-screen is used to implement the existing full-screen feature for <video>. We need to distinguish this from using the full-screen API on a <video> element; the current behavior is a bug. Once we can distinguish these cases, the full-screen API version won’t want this quirk -- it's specific to the medial-element-specific full-screen. But for now we don't yet distinguish the cases. Not sure that's clear enough. You're right to say there is a bug. But this should not change in the full-screen API spec because this is used to implement something separate from that spec.
Ademar Reis
Comment 16 2011-06-03 14:01:47 PDT
Revision r87692 cherry-picked into qtwebkit-2.2 with commit 65741cd <http://gitorious.org/webkit/qtwebkit/commit/65741cd>
Note You need to log in before you can comment on or make changes to this bug.