Bug 208204 - [GTK][WPE] Support color-schemes CSS property
Summary: [GTK][WPE] Support color-schemes CSS property
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 226627 227862 (view as bug list)
Depends on:
Blocks: 227862
  Show dependency treegraph
 
Reported: 2020-02-25 09:12 PST by Michael Catanzaro
Modified: 2021-07-21 12:44 PDT (History)
24 users (show)

See Also:


Attachments
Patch (65.37 KB, patch)
2021-07-15 03:08 PDT, Alexander Mikhaylenko
no flags Details | Formatted Diff | Diff
Patch (64.58 KB, patch)
2021-07-15 08:45 PDT, Alexander Mikhaylenko
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (64.65 KB, patch)
2021-07-15 09:17 PDT, Alexander Mikhaylenko
no flags Details | Formatted Diff | Diff
Patch (64.65 KB, patch)
2021-07-15 09:18 PDT, Alexander Mikhaylenko
aperez: review+
aperez: commit-queue-
Details | Formatted Diff | Diff
Patch (64.21 KB, patch)
2021-07-15 12:24 PDT, Alexander Mikhaylenko
no flags Details | Formatted Diff | Diff
Patch (64.24 KB, patch)
2021-07-16 08:23 PDT, Alexander Mikhaylenko
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2020-02-25 09:12:26 PST
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.
Comment 1 Carlos Garcia Campos 2020-02-26 00:59:13 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 :-)
Comment 2 Michael Catanzaro 2021-07-09 05:01:20 PDT
*** Bug 226627 has been marked as a duplicate of this bug. ***
Comment 3 Michael Catanzaro 2021-07-09 05:02:05 PDT
There's some justification for why this is important in bug #226627.
Comment 4 Michael Catanzaro 2021-07-11 16:50:34 PDT
I think this would also fix bug #227862.
Comment 5 Alexander Mikhaylenko 2021-07-13 07:29:40 PDT
*** Bug 227862 has been marked as a duplicate of this bug. ***
Comment 6 Alexander Mikhaylenko 2021-07-13 07:30:19 PDT
This requires flipping OS_DARK_MODE_SUPPORT as well, do we want to do that for WPE as well?
Comment 7 Adrian Perez 2021-07-13 08:07:11 PDT
(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!
Comment 8 Alexander Mikhaylenko 2021-07-13 08:41:13 PDT
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. :)
Comment 9 Alexander Mikhaylenko 2021-07-15 03:08:32 PDT
Created attachment 433572 [details]
Patch

This took longer than expected.
Comment 10 Michael Catanzaro 2021-07-15 07:58:20 PDT
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.
Comment 11 Alexander Mikhaylenko 2021-07-15 08:01:07 PDT
> 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 12 Michael Catanzaro 2021-07-15 08:04:22 PDT
Comment on attachment 433572 [details]
Patch

Ah OK, the list *should* be sorted, but it doesn't matter. Don't worry about it then.
Comment 13 Alexander Mikhaylenko 2021-07-15 08:16:47 PDT
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.
Comment 14 Alexander Mikhaylenko 2021-07-15 08:20:37 PDT
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.
Comment 15 Michael Catanzaro 2021-07-15 08:24:10 PDT
I wondered about that too. I would say go ahead and expose it, indeed.
Comment 16 Alexander Mikhaylenko 2021-07-15 08:45:16 PDT
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.
Comment 17 Alexander Mikhaylenko 2021-07-15 09:17:02 PDT
Created attachment 433588 [details]
Patch

Ok, so I was wrong. It's mac || ios || os_dark_mode_support now.
Comment 18 Alexander Mikhaylenko 2021-07-15 09:18:56 PDT
Created attachment 433589 [details]
Patch

So I was wrong and that color is used outside of OS_DARK_MODE_SUPPORT as well.
Comment 19 Alexander Mikhaylenko 2021-07-15 09:19:27 PDT
Great, so bugzilla showed me an error the first time but still attached the patch.
Comment 20 Adrian Perez 2021-07-15 12:18:46 PDT
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.
Comment 21 Adrian Perez 2021-07-15 12:20:25 PDT
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.
Comment 22 Alexander Mikhaylenko 2021-07-15 12:24:47 PDT
Created attachment 433609 [details]
Patch

> TIL about CSS mask-image ^_^

Yeah, I noticed the Apple style was doing it and it works nicely. :)
Comment 23 Alexander Mikhaylenko 2021-07-16 08:23:26 PDT
Created attachment 433677 [details]
Patch

Michael said I can reupload it with Adrian's r+ intact
Comment 24 EWS 2021-07-16 09:39:22 PDT
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].
Comment 25 Carlos Alberto Lopez Perez 2021-07-21 12:44:50 PDT
(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