Bug 204468

Summary: A video element cannot enter fullscreen from PiP mode
Product: WebKit Reporter: Peng Liu <peng.liu6>
Component: MediaAssignee: Peng Liu <peng.liu6>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric.carlson, ews-watchlist, glenn, jer.noble, philipj, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: iPhone / iPad   
OS: iOS 13   
Attachments:
Description Flags
Patch
eric.carlson: review+
Patch for landing none

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>