Bug 208204

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