1590894 - Need to support CSS4 system colors

Status: RESOLVED FIXED

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

Description

[almaher]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/79.0.3945.0 Safari/537.36 Edg/79.0.313.0

Steps to reproduce:

Support is needed for new CSS4 system color keywords: https://drafts.csswg.org/css-color-4/#css-system-colors

Missing keywords include:

• ActiveText
• Canvas
• Field
• FieldText
• LinkText
• Text
• VisitedText

Comment 1

[Emilio Cobos Álvarez (:emilio)]

At least five of them we already have -moz- prefixed variants of: https://searchfox.org/mozilla-central/rev/088e2cf29c59d733d57af43903eb0267dbf72e2a/servo/components/style/values/specified/color.rs#557

I think the remaining would be Field and FieldText, which we also have apparently!

https://searchfox.org/mozilla-central/rev/088e2cf29c59d733d57af43903eb0267dbf72e2a/servo/components/style/values/specified/color.rs#142

So this should be pretty easy. Mostly renaming and adding #[parse(aliases)] declarations like https://searchfox.org/mozilla-central/rev/088e2cf29c59d733d57af43903eb0267dbf72e2a/servo/components/style/values/computed/length.rs#875.

If someone is interested in giving this a shot, I'd be happy to mentor or help if something is unclear or if they get stuck!

Comment 2

[Sam Mauldin (:Sam)]

Hi, I'm new to both Rust and contributing to Servo, and I'm interested in picking this up.

Could you help me figure out what exactly needs to be done for this?

My best guess for starting is that I need to remove the moz prefix from Mozfield and Mozfieldtext and add aliases for the moz prefixed version

e.g.

pub enum Color {
    #[parse(aliases = "Mozfield")]
    Field
}

However, I'm not really sure how this interacts with the rest of the codebase - where it'd need to be renamed and where this color is defined in the first place.

Thanks for your help.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Hi! The aliases field is not camel-case, so should be something like #[parse(aliases = "-moz-field")].

But yeah, generally that sums it up. After changing the name you'll get a few build errors, that will tell you next steps. The Rust enum automagically generates a C++ enum at build time, with the same variants, so for example these uses of MozField need to be renamed to just Field:

• https://searchfox.org/mozilla-central/rev/74cc0f4dce444fe0757e2a6b8307d19e4d0e0212/layout/generic/nsTextFrame.cpp#4081

There are a couple others, but generally, yeah, you also need to rename the ColorID::MozField to be ColorID::Field in that example, same with the other variants.

These colors are queried to the system via the various implementtions of LookAndFeel for different platforms.

Comment 4

[Sam Mauldin (:Sam)]

Attachment: Bug 1590894 - Remove Moz prefix from MozField and MozFieldtext, add aliases for missing keywords

Removing the Moz prefix to support the CSS4 System Colors Draft
An alias has been added to keep support for the -moz-field(text) variant

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Hmm, actually, I have a few questions, and I suspect David will have opinions for them so I want him to take a look... And I suspect that the patch may need revisions / followup work to deal with some of these.

Thinking a bit harder about this, we should probably actually expose the system color, even if the default text and background are overriden by user preferences.

That is:

• linktext should probably map to -moz-nativehyperlinktext, not -moz-hyperlinktext.
• canvas / text should probably map to WindowBackground / WindowForeground, not -moz-default-background-color / -moz-default-color.
• ActiveLinkText / VisitedLinkText need some new implementation that pokes at the system, probably.
• We should probably use the "standins" mechanism to provide the privacy-preserving alternative to these.

Davide, what's your thought? should we override the system with the user preference? or the other way around?

In any case this is probably worth landing, and workign on top, or at least we should split the field / fieldtext changes as those are straight renames even with these changes.

Comment 6

[Bill Goldstein [:aja] (UTC-6)]

Note renaming in css-color-4 of new system color Text to CanvasText
https://github.com/w3c/csswg-drafts/issues/4465#issuecomment-547994485

Comment 7

[j.j.]

Setting keyword "dev-doc-needed" (should be set ideally together with reporting this kind of bugs). Thanks!

Comment 8

[David Baron :dbaron:]

Sorry for the delay responding here.

I filed https://github.com/w3c/csswg-drafts/issues/4533 to ask the spec to clarify. I think my preference is probably leaning slightly the other way: that it probably makes more sense for the keywords to be Gecko's preference, since pages would likely want to use them to reset things to the defaults for links, or to style things that aren't links to look like the browser's defaults for links, etc. (I think some of the use cases for the existing -moz- variants are in browser chrome, which is somewhat different since it wants to fit in to the system.)

I agree that the Text->CanvasText should be fixed, and the anti-fingerprinting preference honored.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

I'm ok with that, I think that argument makes sense, but let's wait for the WG to clarify.

Comment 10

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

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:Sam, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 11

[Sam Mauldin (:Sam)]

Are we still waiting for clarification?

Comment 12

[Frederik Braun [:freddy]]

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

I'm ok with that, I think that argument makes sense, but let's wait for the WG to clarify.

(In reply to Sam Mauldin (:Sam) from comment #11)

Are we still waiting for clarification?

Emilio, I think the assignee meant to flag a needinfo to you. See above for the question.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Yes, as other vendors haven't replied... I've added it to the agenda so it gets discussed.

Comment 14

[David Baron :dbaron:]

There's a resolution now in that issue that it should reflect the browser settings.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Sam, I think your patch is alright based on that, do you mind rebasing it and update the test expectations? I suspect at least https://searchfox.org/mozilla-central/source/testing/web-platform/tests/css/css-color/color-initial-canvastext.html and https://searchfox.org/mozilla-central/source/testing/web-platform/tests/css/css-color/parsing/system-color-valid.html should have unexpected passes.

If you don't have time let me know and I'm happy to do that.

Comment 16

[Sam Mauldin (:Sam)]

Attachment: Bug 1590894: Unprefix existing CSS4 system colors

Comment 17

[Sam Mauldin (:Sam)]

Hey!

I've rebased this patch, changed Text to Canvastext, and update test expectations. I added expected fails for some forced-colors-mode-backplate, which I suspect were only passing because the backplate of Canvastext wasn't actually being applied since we didn't have that color, but I'd appreciate if you could confirm that this is expected.

Comment 18

[Emilio Cobos Álvarez (:emilio)]

I replied in phab, but yeah those tests are passing for the wrong reasons so it is fine to annotate them as such with your patch.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a2030f611422
Unprefix existing CSS4 system colors r=emilio

Comment 20

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

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

Comment 21

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

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 22

[Daniel Varga [:dvarga]]

Backed out changeset a2030f611422 (Bug 1590894) for causing mochitest failure at layout/style/test/test_value_computation.html

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&collapsedPushes=659858&selectedJob=293236916&resultStatus=testfailed%2Cbusted%2Cexception&revision=a2030f611422814034b8c2ddf7014aea8b05bb56

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=293236916&repo=autoland&lineNumber=6709

Backout link: https://treeherder.mozilla.org/#/jobs?repo=autoland&collapsedPushes=659858&resultStatus=testfailed%2Cbusted%2Cexception&revision=f14f678590b7b71de62740c3ea540b48eb17397b

[task 2020-03-15T20:35:08.977Z] 20:35:08     INFO -  3570 INFO TEST-PASS | layout/style/test/test_value_computation.html | should not get empty value for 'color:canvastext'
[task 2020-03-15T20:35:08.977Z] 20:35:08     INFO -  Buffered messages finished
[task 2020-03-15T20:35:08.986Z] 20:35:08  WARNING -  3571 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_computation.html | should not get initial value for 'color:canvastext' on elementn. - didn't expect "rgb(0, 0, 0)", but got it
[task 2020-03-15T20:35:08.986Z] 20:35:08     INFO -      SimpleTest.isnot@SimpleTest/SimpleTest.js:402:14
[task 2020-03-15T20:35:08.986Z] 20:35:08     INFO -      test_value@layout/style/test/test_value_computation.html:174:73
[task 2020-03-15T20:35:08.986Z] 20:35:08     INFO -      test_property@layout/style/test/test_value_computation.html:206:15
[task 2020-03-15T20:35:08.986Z] 20:35:08     INFO -      do_one@layout/style/test/test_value_computation.html:221:18
[task 2020-03-15T20:35:08.987Z] 20:35:08  WARNING -  3572 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_value_computation.html | should not get initial value for 'color:canvastext' on elementf. - didn't expect "rgb(0, 0, 0)", but got it
[task 2020-03-15T20:35:08.987Z] 20:35:08     INFO -      SimpleTest.isnot@SimpleTest/SimpleTest.js:402:14
[task 2020-03-15T20:35:08.987Z] 20:35:08     INFO -      test_value@layout/style/test/test_value_computation.html:178:86
[task 2020-03-15T20:35:08.987Z] 20:35:08     INFO -      test_property@layout/style/test/test_value_computation.html:206:15
[task 2020-03-15T20:35:08.987Z] 20:35:08     INFO -      do_one@layout/style/test/test_value_computation.html:221:18

Comment 23

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

Upstream PR was closed without merging

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Hmm, that's a bit annoying. I guess canvastext should be in the initial_values array.

Comment 25

[Sam Mauldin (:Sam)]

Commented in Phabricator, I think this should be good to try and land again if everything's alright with you.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=21765b7681e14d88c93f9dae515f183cb75d116f

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Yeah, agreed. Sorry I missed that comment. Thank you!

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3f2a791d050a
Unprefix existing CSS4 system colors r=emilio

Comment 28

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

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 29

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/3f2a791d050a

Comment 30

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

Upstream PR merged by moz-wptsync-bot

Comment 31

[Janet Swisher]

Am I understanding correctly that Firefox does not yet support forced-colors (per bug 1591204)? And that therefore the dev-doc-needed here is for documenting the standard keywords, but not for Firefox doing something with them?

Comment 32

[Janet Swisher]

Updated the System colors section of the color data type page, and moved the old values into a "deprecated" subsection. Linked to the system colors section from the forced-colors page.

Comment 33

[Emilio Cobos Álvarez (:emilio)]

Hi Janet,

(In reply to Janet Swisher from comment #31)

Am I understanding correctly that Firefox does not yet support forced-colors (per bug 1591204)? And that therefore the dev-doc-needed here is for documenting the standard keywords, but not for Firefox doing something with them?

That's right. We do support some of forced-colors (as in, windows high-contrast mode does most if not all of that). But we don't support the media feature or the forced-color-adjust css property.

The documentation is for the new CSS color keywords.

(In reply to Janet Swisher from comment #32)

Updated the System colors section of the color data type page, and moved the old values into a "deprecated" subsection. Linked to the system colors section from the forced-colors page.

Thanks! That sounds great.