1932544 - --custom-property: light-dark(...) fails if light value is the same as custom property initial value

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[atjn]

Attachment: testcase 1 (from reporter)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:132.0) Gecko/20100101 Firefox/132.0

Steps to reproduce:

• Open test-case.html or the codepen: https://codepen.io/atjn/pen/PoMrJRj
• Observe that the square is red in Firefox 132 (stable) and 134 (nightly).
• Observe that the square is green in all Chromium and WebKit browsers.

Actual results:

If you set the value of a custom CSS property with the light-dark function, and the CSS property's initial-value is the same as the light value, then Firefox bugs out and reverts to the initial-value. This is fine if the website is shown in light mode, but if it is shown in dark mode, the property will be set to the wrong color.

Expected results:

Firefox should not bug out and revert to the initial-value, it should have used the dark value which means the square should be green.

Comment 1

[atjn]

There is another bug that mentions similar issues: https://bugzilla.mozilla.org/show_bug.cgi?id=1924562
It also includes the bug that I am reporting here, but it does not seem like the reporter nor the triager is aware that they are encountering this specific bug, they are discussing something different. I think it is fine to keep both bugs open as they are discussing different issues even though the test cases overlap.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::CSS Parsing and Computation' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 3

[Daniel Holbert [:dholbert]]

Thanks for the report!

Side note: it looks like your testcase is structured as a WPT - are you in the process of submitting that to the shared WPT repo as part of another ticket or bug report somewhere? If so, let us know, so we can track the association with this bug when that testcase makes it into the WPT repository - thanks! Or if not, we can land it here whenever this bug gets fixed.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: reference case 1 (same as testcase but with a different `initial-value` which avoids the problem)

Comment 5

[Daniel Holbert [:dholbert]]

Maybe this is the same as bug 1926969?

Comment 6

[Tess Hoffman [:zrhoffman]]

(In reply to Daniel Holbert [:dholbert] from comment #5)

Maybe this is the same as bug 1926969?

In bug 1932544, the light-dark() value is not in an initial value (and the patch from bug 1926969 does not make the bug 1932544 testcase pass), so 1932544 is a separate issue. Thanks for the report and test, atjn!

Comment 7

[atjn]

(In reply to Daniel Holbert [:dholbert] from comment #3)

Side note: it looks like your testcase is structured as a WPT - are you in the process of submitting that to the shared WPT repo as part of another ticket or bug report somewhere? If so, let us know, so we can track the association with this bug when that testcase makes it into the WPT repository - thanks! Or if not, we can land it here whenever this bug gets fixed.

Yeah, I am very interested in WPT and I am hoping that a test for this bug can be added in some form or another. Unfortunately, I have not had much success trying to contribute to WPT, my questions and PRs have mostly been met with a wall of silence (no hard feelings, I'm sure everyone is just busy), so I decided to just include the test here and see what you do with it.

If you have any feedback on what I can do to find more success with WPT's I am all ears. I would love to not just write specific bug tests, but also try to write some more systematic tests suites for features that are undertested, such as light-dark which currently only has five simple WPTs. Right now I just have the feeling that any work I would do would take me hours and hours, and would then result in a PR that no one is going to review.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to atjn from comment #7)

Yeah, I am very interested in WPT [...]
If you have any feedback on what I can do to find more success with WPT's I am all ears.

Sorry to hear about your WPT-PR experiences being not-great (and it's great to hear of folks being interested to add tests)! I don't have any specific feedback/suggestions [and this bug probably isn't a great place for a discussion on WPT-PR-best-practices, since we try to keep bugs focused on the details of the issue itself and the fix], and I don't have a lot of experience working directly with the WPT repo PR process, but as a quick recommendation in general: I find that test-reviews are most-rapidly-acted-upon when you make the job as easy as possible for the reviewer to understand (1) what the test is doing, and (2) why its expectations are correct (particularly important to justify if it fails in some browsers). And if you're adding a group of similar tests, or variants of an existing test: (3) it's great to reduce unnecessary differences between the tests, so that the reviewer only needs to thoroughly review the first one, and then just skim the diffs between that one and the others.

Anyway: for the purposes of the testcase that you posted here: I just wanted to be sure we didn't end up inadvertently generating a duplicate in the WPT repo if/when we land a patch here at some point with that test included. If you do happen to submit the test elsewhere in the meantime, just mention it here and we can avoid doing that. :) Thanks!

Comment 9

[atjn]

Thanks for the feedback, much appreciated. We can focus on the bug now :)

Comment 10

[Emilio Cobos Álvarez [:emilio]]

Uh, I just found out about this, that's kind of a surprising bug. Interestingly, if the color-scheme comes from the parent rather than being specified on the element itself, then it works as expected...

Comment 11

[Emilio Cobos Álvarez [:emilio]]

I see what's going on.

Comment 12

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1932544 - Remove unsound registered property optimization. r=#style

At the time we try to figure out whether a value might change style, our
dependent properties are not applied, so we might end up with the wrong
computed value.

We could check potential dependencies, but it seems better to just not
bother computing the value, it might be more expensive than applying it,
and we have custom property de-duplication afterwards anyways now.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/73c6468cf280
https://hg.mozilla.org/integration/autoland/rev/cd357a6c3334
Remove unsound registered property optimization. r=firefox-style-system-reviewers,dshin

Comment 14

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/59989 for changes under testing/web-platform/tests

Comment 15

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/cd357a6c3334

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot