The bug occurs because any flags about missing components, generated during conversion of the colour (e.g. HSL->OKLCH it can produce a powerless hue) within are overridden by the source colour's flags.
The change is to make it instead by a disjunction of the source color flag and the flag of the color itself.
Details
Details
- Reviewers
tlouw emilio - Group Reviewers
Restricted Project - Commits
- rFIREFOXAUTOLAND557d0a01bb28: Bug 2001615 - Fix carry forward to not override flags from conversion. r=tlouw…
- Bugzilla Bug ID
- 2001615
Diff Detail
Diff Detail
- Repository
- rFIREFOXAUTOLAND firefox-autoland
Event Timeline
Comment Actions
Code analysis found 1 defect in diff 1168443:
- 1 defect found by file-whitespace (Mozlint)
IMPORTANT: Found 1 defect (error level) that must be fixed before landing.
You can run this analysis locally with:
- ./mach lint --warnings --outgoing
If you see a problem in this automated review, please report it here.
You can view these defects in the Diff Detail section of Phabricator diff 1168443.
Comment Actions
Code analysis found 1 defect in diff 1168450:
- 1 defect found by file-whitespace (Mozlint)
IMPORTANT: Found 1 defect (error level) that must be fixed before landing.
You can run this analysis locally with:
- ./mach lint --warnings --outgoing
If you see a problem in this automated review, please report it here.
You can view these defects in the Diff Detail section of Phabricator diff 1168450.
| servo/components/style/color/mix.rs | ||
|---|---|---|
| 258 | Why not insert()? if source.flags.contains(F::C0_IS_NONE) {
self.flags.insert(F::C0_IS_NONE);
} | |
Comment Actions
Thanks! Looks reasonable.
Yeah let's use insert for the flags that are different, and that or something like what I suggested when the components match.
Let's also simplify the test. Is there any reason it needs to go on its own test file?
| servo/components/style/color/mix.rs | ||
|---|---|---|
| 301 | For the ones with the same bit you can even do: self.flags |= other.flags & F::C1_IS_NONE; which is a bit easier to read | |
| 594 | Please fix the lint? | |
| testing/web-platform/tests/css/css-color/color-mix-missing-components-carry-forward.html | ||
| 3 ↗ | (On Diff #1168450) | Let's just add this to color-mix-computed or so rather than adding a whole new file? |
Comment Actions
Good catch! Looks good with the combined testing and .insert issues addressed.
| testing/web-platform/tests/css/css-color/color-mix-missing-components-carry-forward.html | ||
|---|---|---|
| 13 ↗ | (On Diff #1168450) | Can you add more tests? Ones that exercise all the combinations? Slightly more effort to figure out which combinations, but would be super helpful to catch these. |
Comment Actions
Hello 👋:)
I added the changes, took me a bit of attempts to get used to moz-phab (hence the extra random revision that happened x). I added the changes and appended the test to the end of the color-computed-color-mix-function.html file, under a new "subsection".
Edit: Just saw the comment about adding more tests. I will do so later today as soon as possible