Bug 197086 - Layout of YouTube videos is wrong in fullscreen after the first time
Summary: Layout of YouTube videos is wrong in fullscreen after the first time
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified macOS 10.14
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-04-18 18:19 PDT by Remy Demarest
Modified: 2019-05-02 16:21 PDT (History)
8 users (show)

See Also:


Attachments
Patch (6.51 KB, patch)
2019-04-18 18:34 PDT, Remy Demarest
no flags Details | Formatted Diff | Diff
Fix compilation error and ensure the titlebar shows in fullscreen (6.51 KB, patch)
2019-04-18 19:35 PDT, Remy Demarest
no flags Details | Formatted Diff | Diff
Patch including feedback changes (6.86 KB, patch)
2019-04-22 16:57 PDT, Remy Demarest
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Remy Demarest 2019-04-18 18:19:55 PDT
View layout is wrong after entering fullscreen mode from the video using WebKit API.
Comment 1 Remy Demarest 2019-04-18 18:21:10 PDT
<rdar://problem/47733671>
Comment 2 Remy Demarest 2019-04-18 18:34:46 PDT
Created attachment 367780 [details]
Patch
Comment 3 Remy Demarest 2019-04-18 19:35:55 PDT
Created attachment 367784 [details]
Fix compilation error and ensure the titlebar shows in fullscreen
Comment 5 Darin Adler 2019-04-21 12:37:00 PDT
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.
Comment 6 Remy Demarest 2019-04-22 01:24:18 PDT
>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.
Comment 7 Darin Adler 2019-04-22 08:39:02 PDT
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?
Comment 8 Remy Demarest 2019-04-22 13:58:06 PDT
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.
Comment 9 Remy Demarest 2019-04-22 16:57:23 PDT
Created attachment 367997 [details]
Patch including feedback changes
Comment 10 WebKit Commit Bot 2019-04-23 09:25:36 PDT
Comment on attachment 367997 [details]
Patch including feedback changes

Clearing flags on attachment: 367997

Committed r244545: <https://trac.webkit.org/changeset/244545>
Comment 11 WebKit Commit Bot 2019-04-23 09:25:38 PDT
All reviewed patches have been landed.  Closing bug.