Closed Bug 1591210 (forced-color-adjust) Opened 5 years ago Closed 1 year ago

Add forced-color-adjust property

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
112 Branch
Tracking Status
firefox112 --- fixed

People

(Reporter: aja, Assigned: fchasen)

References

(Blocks 3 open bugs, )

Details

(Keywords: access, Whiteboard: [fidefe-quality-foundation])

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:72.0) Gecko/20100101 Firefox/72.0

Expected results:

Add forced-color-adjust property, as per
https://drafts.csswg.org/css-color-adjust-1/#forced

There are still quite a few details that are being sorted out in the working group.

Alias: color-adjust-1
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
See Also: → 1591204

Note the addition of value 'preserve-parent-color' (which has UA stylesheet additions for SVGs), in addition to existing 'none' and 'auto' values.

Have details been sorted out in the working group over the past few years enough to implement?
Either with or without the newish 'preserve-parent-color' value?
It's been in Blink for quite a while.

Sure would be nice to be able to use in HCM tests, e.g. green/red pass/fail using 'none' value, consistently cross-browser.

Summary: Add forced-color-adjust propery → Add forced-color-adjust property
Blocks: 1748857

(In reply to Emilio Cobos Álvarez (:emilio) from comment #1)

There are still quite a few details that are being sorted out in the working group.

Any news?
Asking because of https://github.com/mozilla/pdf.js/issues/13230.

Keywords: access
Severity: normal → S3
Severity: normal → S3
Flags: needinfo?(emilio)
Whiteboard: [fidefe-quality-foundation]

So to add this we need:

  • To add the property in servo/components/style/longhands. This is an inherited property that affects colors, so putting it in the same struct as the color property would be the obvious choice. So that'd be around here.

  • You want a keyword type so you probably want to add an pub enum ForcedColorAdjust { ... }. Something like other keywords like this.

  • With that and the member added to nsStyleText here you should've added the CSS property.

Now you'd need to make it actually do something. This property affects colors, so you want to prioritize it on top of all the other color properties by adding it to the right cascade group.

Once you have that you'd need to tweak these checks to early-return when forced-color-adjust is none or preserve-parent-color, as needed.

Flags: needinfo?(emilio)
Assignee: nobody → fchasen
Status: NEW → ASSIGNED

Thank so much for the detailed instructions Emilio, was able to get this parsing the keywords so far with those!

I'm wondering if the PrintColorAdjust might be a good example to base this off: https://searchfox.org/mozilla-central/source/servo/components/style/values/specified/color.rs#1309-1329 since this property affects both text and background-colors?

If I mirrored that than it would then be accessible from StyleVisibility()->mForcedColorAdjust.

Yes but not quite, because we do the color forcing at styling time. So the checks will need to happen in cascade.rs (because otherwise the non-forced colors are gone already).

Adds the forced-color-adjust property and ForcedColorAdjust keywords.
Updates tweak_when_ignoring_colors to check for none and preserve-parent-color values of that property when determining if a color adjustment in needed.

Going through the WPTs in testing/web-platform/tests/forced-colors-mode/ to see what we could enable once this is supported.

Looks like the selection force adjust none isn't currently working (forced-colors-mode-14.html), so looking into that.

Also the SVG ones still fail, but not sure we currently apply forced colors on SVG?

Was able to get the selection to not adjust by checking pseudoStyle->StyleText()->mForcedColorAdjust != StyleForcedColorAdjust::None in ComputeSelectionStyle: https://searchfox.org/mozilla-central/source/layout/generic/nsIFrame.cpp#2401

Not sure that's the best way though?

Pushed by fchasen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f4fe07518400
Add forced-color-adjust property r=emilio

Backed out changeset f4fe07518400 (Bug 1591210) for web-platform failures on forced-colors-mode-14.html.
Backout link
Push with failures <--> Wr1
Failure Log
Also c3

Flags: needinfo?(fchasen)

Thanks, I've updated the unexpectedly passing test to pass in Linux now and fixed the failures with test_animation-type-longhand.html.

Flags: needinfo?(fchasen)
Pushed by fchasen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1c143c3c235b
Add forced-color-adjust property r=emilio
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 112 Branch
Blocks: 1818819
Alias: color-adjust-1 → forced-color-adjust
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: