Summary: | [GTK][WPE] Support color-schemes CSS property | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||||||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | alicem, annulen, aperez, benjamin, bugs-noreply, cassidy, cdumez, cgarcia, changseok, clopez, cmarcelo, esprehn+autocc, ews-watchlist, glenn, gszymaszek, gyuyoung.kim, ie2kl43y, kondapallykalyan, macpherson, mcatanzaro, mcrha, menard, pdr, ryuan.choi, sergio | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | PC | ||||||||||||||||
OS: | Linux | ||||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=197947 https://bugs.webkit.org/show_bug.cgi?id=228160 |
||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 227862 | ||||||||||||||||
Attachments: |
|
Description
Michael Catanzaro
2020-02-25 09:12:26 PST
Well, I think we can try, but I want to give some more time to the new theme code, get feedback and improve it a bit before adding a new feature like this. We have the whole release cycle :-) *** Bug 226627 has been marked as a duplicate of this bug. *** There's some justification for why this is important in bug #226627. I think this would also fix bug #227862. *** Bug 227862 has been marked as a duplicate of this bug. *** This requires flipping OS_DARK_MODE_SUPPORT as well, do we want to do that for WPE as well? (In reply to Alexander Mikhaylenko from comment #6) > This requires flipping OS_DARK_MODE_SUPPORT as well, do we want to do that > for WPE as well? Yes, please, that would be great. It can also be done in a follow-up patch if that would be easier =) Thanks! Wouldn't be easier, it must be done in lockstep: * having color-scheme without OS_DARK_MODE_SUPPORT results in a white background peeking from under entry corners and a a dark text on dark bg in entries and similar widgets * having OS_DARK_MODE_SUPPORT without color-scheme means all of the things listed in https://bugs.webkit.org/show_bug.cgi?id=197947 The WPE API can be done later but the flag has to be flipped at the same time. :) Created attachment 433572 [details]
Patch
This took longer than expected.
Comment on attachment 433572 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433572&action=review LGTM. Carlos Garcia will probably want to review this, though. > Source/cmake/OptionsWPE.cmake:288 > +SET_AND_EXPOSE_TO_BUILD(HAVE_OS_DARK_MODE_SUPPORT 1) This should be alphabetized. > This should be alphabetized.
I would if the list was sorted in the first place. :)
```
SET_AND_EXPOSE_TO_BUILD(HAVE_ACCESSIBILITY ${ENABLE_ACCESSIBILITY})
SET_AND_EXPOSE_TO_BUILD(USE_ATK ${ENABLE_ACCESSIBILITY})
SET_AND_EXPOSE_TO_BUILD(USE_CAIRO TRUE)
SET_AND_EXPOSE_TO_BUILD(USE_EGL TRUE)
SET_AND_EXPOSE_TO_BUILD(USE_GCRYPT TRUE)
SET_AND_EXPOSE_TO_BUILD(USE_LIBEPOXY TRUE)
SET_AND_EXPOSE_TO_BUILD(USE_LIBWPE TRUE)
SET_AND_EXPOSE_TO_BUILD(USE_OPENGL_ES TRUE)
SET_AND_EXPOSE_TO_BUILD(HAVE_OPENGL_ES_3 TRUE)
SET_AND_EXPOSE_TO_BUILD(USE_WPE_RENDERER TRUE)
SET_AND_EXPOSE_TO_BUILD(USE_XDGMIME TRUE)
if (WTF_CPU_ARM OR WTF_CPU_MIPS)
SET_AND_EXPOSE_TO_BUILD(USE_CAPSTONE ${ENABLE_DEVELOPER_MODE})
endif ()
SET_AND_EXPOSE_TO_BUILD(USE_TEXTURE_MAPPER TRUE)
SET_AND_EXPOSE_TO_BUILD(USE_TEXTURE_MAPPER_GL TRUE)
SET_AND_EXPOSE_TO_BUILD(USE_TILED_BACKING_STORE TRUE)
SET_AND_EXPOSE_TO_BUILD(USE_COORDINATED_GRAPHICS TRUE)
SET_AND_EXPOSE_TO_BUILD(USE_NICOSIA TRUE)
SET_AND_EXPOSE_TO_BUILD(HAVE_OS_DARK_MODE_SUPPORT 1)
```
Comment on attachment 433572 [details]
Patch
Ah OK, the list *should* be sorted, but it doesn't matter. Don't worry about it then.
One thing I wonder about is why `-webkit-control-background` is macOS/iOS-only. If it wasn't we wouldn't have needed the html.css changes. Hm, actually looks like it was an oversight: https://bugs.webkit.org/show_bug.cgi?id=223303 Then I suppose it would be fine to again expose it for `defined(HAVE_OS_DARK_MODE_SUPPORT) && HAVE_OS_DARK_MODE_SUPPORT` and have one less thing to worry about. I wondered about that too. I would say go ahead and expose it, indeed. Created attachment 433586 [details]
Patch
Changed. I thought maybe one possibility was having the color for pre-Mojave macOS, but seems it's already hidden behind an HAVE_OS_DARK_MODE_SUPPORT check, so _should_ be fine.
Created attachment 433588 [details]
Patch
Ok, so I was wrong. It's mac || ios || os_dark_mode_support now.
Created attachment 433589 [details]
Patch
So I was wrong and that color is used outside of OS_DARK_MODE_SUPPORT as well.
Great, so bugzilla showed me an error the first time but still attached the patch. Comment on attachment 433589 [details] Patch This looks great, well done getting through all the churn to get the colors and theme rendering working with the dark color scheme \o/ View in context: https://bugs.webkit.org/attachment.cgi?id=433589&action=review > Source/WebCore/css/themeAdwaita.css:29 > + -webkit-mask-image: url("data:image/svg+xml;utf-8, \ TIL about CSS mask-image ^_^ > Source/WebCore/platform/adwaita/ThemeAdwaita.cpp:304 > + This addition of an empty line is not needed, please leave it out before landing :) > Source/WebCore/platform/adwaita/ThemeAdwaita.cpp:523 > + Ditto for this empty line. I also removed the question mark from the bug title, make sure to update the title line in the ChangeLog entries accordingly before landing as well. Created attachment 433609 [details] Patch > TIL about CSS mask-image ^_^ Yeah, I noticed the Apple style was doing it and it works nicely. :) Created attachment 433677 [details]
Patch
Michael said I can reupload it with Adrian's r+ intact
Committed r279987 (239730@main): <https://commits.webkit.org/239730@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 433677 [details]. (In reply to EWS from comment #24) > Committed r279987 (239730@main): <https://commits.webkit.org/239730@main> > > All reviewed patches have been landed. Closing bug and clearing flags on > attachment 433677 [details]. This caused 7 new unexpected test failures. See bug 228160 |