Bug 197086 – Layout of YouTube videos is wrong in fullscreen after the first time

• Component: WebKit2
• Status: RESOLVED FIXED

[Remy Demarest]

View layout is wrong after entering fullscreen mode from the video using WebKit API.

[Remy Demarest]

<rdar://problem/47733671>

[Remy Demarest]

Created attachment 367780 [details]
Patch

[Remy Demarest]

Created attachment 367784 [details]
Fix compilation error and ensure the titlebar shows in fullscreen

[Darin Adler]

Comment on attachment 367784 [details]
Fix compilation error and ensure the titlebar shows in fullscreen

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

Have you tested this on the older versions of macOS that we need to support? Presumably the WebCoreFullScreenWindow changes affect Legacy WebKit as well; that’s why the class is in WebCore, not WebKit. How did you test Legacy WebKit?

> Source/WebCore/ChangeLog:11
> +        which ends up breaking the layout. Having that window use a style that incldues a

typo "incldues"

> Source/WebKit/ChangeLog:11
> +        which ends up breaking the layout. Having that window use a style that incldues a

typo "incldues"

> Source/WebKit/ChangeLog:15
> +        Declare a secret SPI to be used in WKFullScreenWindowController.

"an SPI", not "a secret SPI"

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:2613
> +    return [[[WebCoreFullScreenWindow alloc] initWithContentRect:[[NSScreen mainScreen] frame] styleMask:(NSWindowStyleMaskTitled | NSWindowStyleMaskUnifiedTitleAndToolbar | NSWindowStyleMaskFullSizeContentView | NSWindowStyleMaskResizable) backing:NSBackingStoreBuffered defer:NO] autorelease];

Not new, but it’s messy to have this tricky technique split across multiple files. Would be nice if more of this could be in WKFullScreenWindowController. Maybe we should add a method +[WKFulScreenWindowController createWindow] that contains this code.

I’d like to see a comment explaining why we don’t use a borderless window so future programmers don’t need to look back at change log history to figure out why.

> Source/WebKit/UIProcess/mac/WKFullScreenWindowController.mm:130
> +    window.titlebarAlphaValue = 0;

I think this needs a why comment. Not clear what we are doing with titlebar alpha here.

[Remy Demarest]

>Have you tested this on the older versions of macOS that we need to support? Presumably the WebCoreFullScreenWindow changes affect Legacy WebKit as well; that’s why the class is in WebCore, not WebKit. How did you test Legacy WebKit?

The changes I made in WebCore have no influence on the behavior of the window without the changes to the style mask that are applied by WebKit. -canBecomeMain returns NO when the window has no titlebar and -constrainFrameRect:toScreen: just returns the frameRect as well. So I did not think I needed to test this particular case.

[Darin Adler]

That answers the second question, with "I assume it has no effect so didn’t test". I suggest finding a way to test to confirm your assumption.

What about the first question: Have you tested this on the older versions of macOS that we need to support?

[Remy Demarest]

I tested WebKitLegacy using MiniBrowser and confirmed there were no changes between the two versions with and without my fix.

I'm still trying to get WebKit to build on Mojave so I can test it there.

[Remy Demarest]

Created attachment 367997 [details]
Patch including feedback changes

[WebKit Commit Bot]

Comment on attachment 367997 [details]
Patch including feedback changes

Clearing flags on attachment: 367997

Committed r244545: <https://trac.webkit.org/changeset/244545>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.