Bug 208204 – [GTK][WPE] Support color-schemes CSS property

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Michael Catanzaro]

Now that GTK port no longer uses foreign drawing to render widgets, since r257299, it's finally possible to support the color-schemes CSS property. (See bug #197947 for discussion of why this was not previously possible.)

Overview of how dark mode works currently:

Firefox and Chrome and WebKitGTK since r255342: indicate to the webpage whether user prefers a dark theme using prefers-color-scheme, but always render controls using light theme. (GOOD)

Safari only: indicates to the webpage whether user prefers a dark theme using prefers-color-scheme, renders light controls usually EXCEPT if website opts-in to dark form controls using color-schemes CSS property. (BEST)

Downsides: this would complicate the new built-in Adwaita theme, so we might not want to do it.

[Carlos Garcia Campos]

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 :-)

[Michael Catanzaro]

*** Bug 226627 has been marked as a duplicate of this bug. ***

[Michael Catanzaro]

There's some justification for why this is important in bug #226627.

[Michael Catanzaro]

I think this would also fix bug #227862.

[Alice Mikhaylenko]

*** Bug 227862 has been marked as a duplicate of this bug. ***

[Alice Mikhaylenko]

This requires flipping OS_DARK_MODE_SUPPORT as well, do we want to do that for WPE as well?

[Adrian Perez]

(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!

[Alice Mikhaylenko]

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. :)

[Alice Mikhaylenko]

Created attachment 433572 [details]
Patch

This took longer than expected.

[Michael Catanzaro]

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.

[Alice Mikhaylenko]

> 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)
```

[Michael Catanzaro]

Comment on attachment 433572 [details]
Patch

Ah OK, the list *should* be sorted, but it doesn't matter. Don't worry about it then.

[Alice Mikhaylenko]

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.

[Alice Mikhaylenko]

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.

[Michael Catanzaro]

I wondered about that too. I would say go ahead and expose it, indeed.

[Alice Mikhaylenko]

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.

[Alice Mikhaylenko]

Created attachment 433588 [details]
Patch

Ok, so I was wrong. It's mac || ios || os_dark_mode_support now.

[Alice Mikhaylenko]

Created attachment 433589 [details]
Patch

So I was wrong and that color is used outside of OS_DARK_MODE_SUPPORT as well.

[Alice Mikhaylenko]

Great, so bugzilla showed me an error the first time but still attached the patch.

[Adrian Perez]

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.

[Adrian Perez]

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.

[Alice Mikhaylenko]

Created attachment 433609 [details]
Patch

> TIL about CSS mask-image ^_^

Yeah, I noticed the Apple style was doing it and it works nicely. :)

[Alice Mikhaylenko]

Created attachment 433677 [details]
Patch

Michael said I can reupload it with Adrian's r+ intact

[EWS]

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].

[Carlos Alberto Lopez Perez]

(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