Bug 61717

Summary: REGRESSION (r87622): Scrubbing a Vimeo movie when in fullscreen stops playback; no way to make it start again
Product: WebKit Reporter: Darin Adler <darin>
Component: New BugsAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: ademar, dglazkov, eric.carlson, jer.noble, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch darin: review+

Description Darin Adler 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
Comment 1 Darin Adler 2011-05-29 20:11:23 PDT
Created attachment 95318 [details]
Patch
Comment 2 Darin Adler 2011-05-29 20:24:38 PDT
Created attachment 95319 [details]
Patch
Comment 3 Jer Noble 2011-05-29 21:53:55 PDT
Created attachment 95324 [details]
Patch
Comment 4 Darin Adler 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!
Comment 5 Simon Fraser (smfr) 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?
Comment 6 Jer Noble 2011-05-29 23:10:55 PDT
Created attachment 95326 [details]
Patch
Comment 7 Simon Fraser (smfr) 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
Comment 8 Darin Adler 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.
Comment 9 Darin Adler 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+
Comment 10 Darin Adler 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.
Comment 11 Darin Adler 2011-05-30 10:09:54 PDT
Committed r87692: <http://trac.webkit.org/changeset/87692>
Comment 12 Jer Noble 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.
Comment 13 Jer Noble 2011-05-30 10:31:31 PDT
Comment on attachment 95326 [details]
Patch

Whoops, missed that commit message. Clearing cq.
Comment 14 Dimitri Glazkov (Google) 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?
Comment 15 Darin Adler 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.
Comment 16 Ademar Reis 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>