Bug 204468 - A video element cannot enter fullscreen from PiP mode
Summary: A video element cannot enter fullscreen from PiP mode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: iPhone / iPad iOS 13
: P2 Normal
Assignee: Peng Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-11-21 13:21 PST by Peng Liu
Modified: 2020-01-14 19:17 PST (History)
8 users (show)

See Also:


Attachments
Patch (2.80 KB, patch)
2019-11-21 14:56 PST, Peng Liu
eric.carlson: review+
Details | Formatted Diff | Diff
Patch for landing (2.99 KB, patch)
2020-01-13 21:42 PST, Peng Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peng Liu 2019-11-21 13:21:04 PST
A video element cannot enter fullscreen from PiP mode
Comment 1 Peng Liu 2019-11-21 13:22:03 PST
<rdar://problem/57325932>
Comment 2 Peng Liu 2019-11-21 13:54:35 PST
This bug only happens on iPad. When we try to enter fullscreen from PiP, it will show the animation but the video element will be in the inline mode. Also, it will be stuck there and we cannot make further presentation mode changes (to fullscreen/PiP).
Comment 3 Peng Liu 2019-11-21 14:56:58 PST
Created attachment 384092 [details]
Patch
Comment 4 Jer Noble 2019-11-21 15:48:21 PST
Comment on attachment 384092 [details]
Patch

This needs an API test before we can land.
Comment 5 Eric Carlson 2019-11-21 16:18:39 PST
Comment on attachment 384092 [details]
Patch

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

> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1474
> -        m_videoFullscreenModel->fullscreenModeChanged(m_currentMode.mode());
> +        m_videoFullscreenModel->fullscreenModeChanged(mode);

It's probably worth adding a comment about why you are making this change, IOW why does "m_currentMode.mode()" not return "mode" immediately after calling "m_currentMode.setMode(mode)"
Comment 6 Peng Liu 2019-11-21 16:39:07 PST
Comment on attachment 384092 [details]
Patch

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

>> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1474
>> +        m_videoFullscreenModel->fullscreenModeChanged(mode);
> 
> It's probably worth adding a comment about why you are making this change, IOW why does "m_currentMode.mode()" not return "mode" immediately after calling "m_currentMode.setMode(mode)"

The setMode() implementation is
void setMode(HTMLMediaElementEnums::VideoFullscreenMode mode) { m_mode |= mode; }

Without the update, m_currentMode will be 3 (both fullscreen and picture-in-picture). It is a strange value for the HTMLVideoElement.
Comment 7 Eric Carlson 2019-11-22 10:09:01 PST
Comment on attachment 384092 [details]
Patch

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

>>> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1474
>>> +        m_videoFullscreenModel->fullscreenModeChanged(mode);
>> 
>> It's probably worth adding a comment about why you are making this change, IOW why does "m_currentMode.mode()" not return "mode" immediately after calling "m_currentMode.setMode(mode)"
> 
> The setMode() implementation is
> void setMode(HTMLMediaElementEnums::VideoFullscreenMode mode) { m_mode |= mode; }
> 
> Without the update, m_currentMode will be 3 (both fullscreen and picture-in-picture). It is a strange value for the HTMLVideoElement.

Please add a comment to the file so someone looking at the code in the future won't change it back.
Comment 8 Peng Liu 2020-01-13 21:29:33 PST
Comment on attachment 384092 [details]
Patch

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

>>>> Source/WebCore/platform/ios/VideoFullscreenInterfaceAVKit.mm:1474
>>>> +        m_videoFullscreenModel->fullscreenModeChanged(mode);
>>> 
>>> It's probably worth adding a comment about why you are making this change, IOW why does "m_currentMode.mode()" not return "mode" immediately after calling "m_currentMode.setMode(mode)"
>> 
>> The setMode() implementation is
>> void setMode(HTMLMediaElementEnums::VideoFullscreenMode mode) { m_mode |= mode; }
>> 
>> Without the update, m_currentMode will be 3 (both fullscreen and picture-in-picture). It is a strange value for the HTMLVideoElement.
> 
> Please add a comment to the file so someone looking at the code in the future won't change it back.

Done.
Comment 9 Peng Liu 2020-01-13 21:42:13 PST
Created attachment 387615 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2020-01-14 08:37:15 PST
Comment on attachment 387615 [details]
Patch for landing

Clearing flags on attachment: 387615

Committed r254512: <https://trac.webkit.org/changeset/254512>