Bug 204468 – A video element cannot enter fullscreen from PiP mode

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Peng Liu]

A video element cannot enter fullscreen from PiP mode

[Peng Liu]

<rdar://problem/57325932>

[Peng Liu]

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).

[Peng Liu]

Created attachment 384092 [details]
Patch

[Jer Noble]

Comment on attachment 384092 [details]
Patch

This needs an API test before we can land.

[Eric Carlson]

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)"

[Peng Liu]

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.

[Eric Carlson]

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.

[Peng Liu]

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.

[Peng Liu]

Created attachment 387615 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 387615 [details]
Patch for landing

Clearing flags on attachment: 387615

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