Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LibWeb/CSS: Implement the color-scheme CSS property #3146

Merged
merged 4 commits into from
Jan 8, 2025

Conversation

Gingeh
Copy link
Contributor

@Gingeh Gingeh commented Jan 5, 2025

First, some beauty shots:
image
image

Now that you've been thoroughly dazzled, I need to mention a few issues with this implementation:

  • System color keywords and the light-dark function aren't computed when cascading or when serialized.
  • Styles aren't correctly invalidated when the preferred color scheme changes.
  • The theme-color and color-scheme meta tags are only evaluated on insertion. (resolved)

I believe the first two are fundamental problems with our style computation system, it can't handle cases where the computed value of a property is meaningfully distinct from the specified value. It isn't enough to just serialize things differently, the computed values are also needed for style inheritance and invalidation.

Copy link
Member

@AtkinsSJ AtkinsSJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool! I really like the parsing code, which probably means I need to get out more. 😆

Libraries/LibWeb/CSS/Default.css Show resolved Hide resolved
Libraries/LibWeb/CSS/StyleValues/ColorSchemeStyleValue.cpp Outdated Show resolved Hide resolved
Libraries/LibWeb/HTML/HTMLInputElement.cpp Outdated Show resolved Hide resolved
Libraries/LibWeb/HTML/HTMLProgressElement.cpp Outdated Show resolved Hide resolved
Libraries/LibWeb/DOM/Document.cpp Outdated Show resolved Hide resolved
Libraries/LibWeb/CSS/Parser/Parser.cpp Show resolved Hide resolved
Libraries/LibWeb/CSS/Parser/Parser.cpp Outdated Show resolved Hide resolved
Libraries/LibWeb/CSS/StyleValues/CSSLightDark.h Outdated Show resolved Hide resolved

String CSSLightDark::to_string(SerializationMode mode) const
{
// FIXME: We don't have enough information to determine the computed value here.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an actual problem, or has the light-dark() function been replaced with an actual color value by then? (Genuine question; this stuff is very confusing. 😅)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the light-dark style value is actually "replaced" at any point, this is relevant to my point in the PR description that our style computation system can't reasonably handle this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried this PR out with http://wpt.live/css/css-color/light-dark-inheritance.html and we currently fail it. From that it seems we're supposed to resolve light-dark() before inheriting it. That's related to what I mean: the computed value can't be light-dark() itself, but should be whatever light-dark() has been resolved to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's what should happen, unfortunately (as I have said) I am unable to find any way to make the style computer actually compute the style value. This is the same problem we had with PositionStyleValue which was "solved" with the SerializationMode workaround, that simply isn't enough here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, I was a bit confused about what you meant exactly. That's a tricky one. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider this PR blocked on that issue or would it be fine to merge as is?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, it's good as-is!

@Gingeh Gingeh force-pushed the color-scheme branch 3 times, most recently from 0a10a7e to 9f7ad01 Compare January 6, 2025 04:48
@Gingeh Gingeh requested a review from AtkinsSJ January 6, 2025 04:51
Copy link
Member

@AtkinsSJ AtkinsSJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Libraries/LibWeb/DOM/Document.cpp Show resolved Hide resolved
Libraries/LibWeb/HTML/HTMLMetaElement.cpp Outdated Show resolved Hide resolved

String CSSLightDark::to_string(SerializationMode mode) const
{
// FIXME: We don't have enough information to determine the computed value here.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried this PR out with http://wpt.live/css/css-color/light-dark-inheritance.html and we currently fail it. From that it seems we're supposed to resolve light-dark() before inheriting it. That's related to what I mean: the computed value can't be light-dark() itself, but should be whatever light-dark() has been resolved to.

@Gingeh Gingeh force-pushed the color-scheme branch 3 times, most recently from cb0228b to 04b33c6 Compare January 7, 2025 08:11
@AtkinsSJ AtkinsSJ merged commit 8e56109 into LadybirdBrowser:master Jan 8, 2025
8 checks passed
@Gingeh Gingeh deleted the color-scheme branch January 8, 2025 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants