Link drag image is inconsistently unreadable in dark mode
Created attachment 342382 [details] Patch
Comment on attachment 342382 [details] Patch Attachment 342382 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/8110746 New failing tests: http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html
Created attachment 342383 [details] Archive of layout-test-results from ews201 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews201 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 342382 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342382&action=review I’m nervous about just removing the check in default appearance. Can we write a test for this? > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:5019 > NSAppearanceName appearance = [[m_view effectiveAppearance] bestMatchFromAppearancesWithNames:@[ NSAppearanceNameAqua, NSAppearanceNameDarkAqua ]]; Are we sure that we can do this without leaking dark mode to parts where we are sure we *don’t* want it to be used? > Source/WebKitLegacy/mac/WebView/WebView.mm:5281 > NSAppearanceName appearance = [[self effectiveAppearance] bestMatchFromAppearancesWithNames:@[ NSAppearanceNameAqua, NSAppearanceNameDarkAqua ]]; Ditto
Comment on attachment 342382 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342382&action=review > Source/WebCore/platform/mac/DragImageMac.mm:306 > + LocalDefaultSystemAppearance localAppearance(true, page ? page->defaultAppearance() : true); We need a better way to allow getting the true effective appearance for places like this that need it. This does not feel right as-is, considering the changes to defaultAppearance(). >> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:5019 >> NSAppearanceName appearance = [[m_view effectiveAppearance] bestMatchFromAppearancesWithNames:@[ NSAppearanceNameAqua, NSAppearanceNameDarkAqua ]]; > > Are we sure that we can do this without leaking dark mode to parts where we are sure we *don’t* want it to be used? Megan is right, we cannot do this here. It will cause Safari to use dark mode colors and form controls for pages.
Comment on attachment 342382 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342382&action=review >> Source/WebCore/platform/mac/DragImageMac.mm:306 >> + LocalDefaultSystemAppearance localAppearance(true, page ? page->defaultAppearance() : true); > > We need a better way to allow getting the true effective appearance for places like this that need it. This does not feel right as-is, considering the changes to defaultAppearance(). I think this is fine now. Sorry for the confusion. I think I want to make a patch that flips defaultAppearance to be prefersDarkInterface, so it is easier to read everywhere. >>> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:5019 >>> NSAppearanceName appearance = [[m_view effectiveAppearance] bestMatchFromAppearancesWithNames:@[ NSAppearanceNameAqua, NSAppearanceNameDarkAqua ]]; >> >> Are we sure that we can do this without leaking dark mode to parts where we are sure we *don’t* want it to be used? > > Megan is right, we cannot do this here. It will cause Safari to use dark mode colors and form controls for pages. Actually, I think this is fine now! I forgot LocalDefaultSystemAppearance now checks for both useSystemAppearance and useDefaultAppearance. In the Safari case, useSystemAppearance will still be false.
(In reply to Timothy Hatcher from comment #6) > Comment on attachment 342382 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=342382&action=review > > >> Source/WebCore/platform/mac/DragImageMac.mm:306 > >> + LocalDefaultSystemAppearance localAppearance(true, page ? page->defaultAppearance() : true); > > > > We need a better way to allow getting the true effective appearance for places like this that need it. This does not feel right as-is, considering the changes to defaultAppearance(). > > I think this is fine now. Sorry for the confusion. I think I want to make a > patch that flips defaultAppearance to be prefersDarkInterface, so it is > easier to read everywhere. Yes please! > >>> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:5019 > >>> NSAppearanceName appearance = [[m_view effectiveAppearance] bestMatchFromAppearancesWithNames:@[ NSAppearanceNameAqua, NSAppearanceNameDarkAqua ]]; > >> > >> Are we sure that we can do this without leaking dark mode to parts where we are sure we *don’t* want it to be used? > > > > Megan is right, we cannot do this here. It will cause Safari to use dark mode colors and form controls for pages. > > Actually, I think this is fine now! I forgot LocalDefaultSystemAppearance > now checks for both useSystemAppearance and useDefaultAppearance. In the > Safari case, useSystemAppearance will still be false. Yeah, that’s what I was banking on. OK!
Comment on attachment 342382 [details] Patch Clearing flags on attachment: 342382 Committed r232731: <https://trac.webkit.org/changeset/232731>
All reviewed patches have been landed. Closing bug.
<rdar://problem/41019148>