|Summary:||Layout of YouTube videos is wrong in fullscreen after the first time|
|Product:||WebKit||Reporter:||Remy Demarest <rdemarest>|
|Severity:||Normal||CC:||commit-queue, darin, eric.carlson, jasminedemeesterdhd, jer.noble, rdemarest, simon.fraser, thorton|
|Version:||WebKit Nightly Build|
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 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.