Bug 61717 – REGRESSION (r87622): Scrubbing a Vimeo movie when in fullscreen stops playback; no way to make it start again

• Component: New Bugs
• Status: RESOLVED FIXED

[Darin Adler]

REGRESSION (r87622): Scrubbing a Vimeo movie when in fullscreen stops playback; no way to make it start again

[Darin Adler]

Created attachment 95318 [details]
Patch

[Darin Adler]

Created attachment 95319 [details]
Patch

[Jer Noble]

Created attachment 95324 [details]
Patch

[Darin Adler]

Comment on attachment 95324 [details]
Patch

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

> Source/WebCore/ChangeLog:102
> [phone]  Brian Weinstein  <[e-mail]>
> +
> +        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 on attachment 95324 [details]
Patch

Is this really fixing just bug 61717, or are there changes related to bug 61715 here too?

[Jer Noble]

Created attachment 95326 [details]
Patch

[Simon Fraser (smfr)]

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 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 on attachment 95326 [details]
Patch

My mistake. This is a new version of the patch. Adding back the review+

[Darin Adler]

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]

Committed r87692: <http://trac.webkit.org/changeset/87692>

[Jer Noble]

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 on attachment 95326 [details]
Patch

Whoops, missed that commit message. Clearing cq.

[Dimitri Glazkov (Google)]

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]

(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]

Revision r87692 cherry-picked into qtwebkit-2.2 with commit 65741cd <http://gitorious.org/webkit/qtwebkit/commit/65741cd>