Summary: | Layout of YouTube videos is wrong in fullscreen after the first time | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Remy Demarest <rdemarest> | ||||||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, darin, eric.carlson, jasminedemeesterdhd, jer.noble, rdemarest, simon.fraser, thorton | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | macOS 10.14 | ||||||||||
Attachments: |
|
Description
Remy Demarest
2019-04-18 18:19:55 PDT
Created attachment 367780 [details]
Patch
Created attachment 367784 [details]
Fix compilation error and ensure the titlebar shows in fullscreen
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. >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.
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? 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. Created attachment 367997 [details]
Patch including feedback changes
Comment on attachment 367997 [details] Patch including feedback changes Clearing flags on attachment: 367997 Committed r244545: <https://trac.webkit.org/changeset/244545> All reviewed patches have been landed. Closing bug. |