1554433 - Move system colors outside of mako

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

When they were first implemented in stylo, we couldn't derive Parse and other traits, so they were put in a mako template, a bit of a hack.

Now we can make this cleaner, which allows us to hide some recently-introduced values from content.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Script to generate the enum

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Script to fix usage of the tree (after running `$ rg 'eColorID_' | cut -d : -f 1 | sort | uniq >files`)

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1554433 - Move system colors to values::specified::color. r=#style,stransky

This should be an idempotent patch. The way to come up with this patch has been:

• Run the first script attached to the bug and pipe it to xclip, then paste it
  in color.rs
• Add the relevant #[derive] annotations and remove the color.mako.rs
  definition.
• Reorder the values to match the ColorID definition, on which some widget
  prefs and caching stuff relies on.
• Manually port some documentation from nsLookAndFeel.h
• Run rg 'eColorID_' | cut -d : -f 1 | sort | uniq >files
• Run the second script attached to the bug.
• Manually fix usage of LAST_COLOR (adding the End variant), and adding
  casts to integer as needed.
• Add an static assert so that people remember to update the prefs, rather than
  a comment on the definition :)

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1554433 - Hide -moz-gtk-buttonactivetext from content. r=stransky

This prevents exposing the value to web content.

Depends on D32610

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3415f556123b
Move system colors to values::specified::color. r=xidorn

Comment 6

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/3415f556123b

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8855dd7a2aa8
Hide -moz-gtk-buttonactivetext from content. r=stransky

Comment 8

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

Bugbug thinks this bug is a task, but please change it back in case of error.

Comment 9

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out changeset 8855dd7a2aa8 (Bug 1554433) for browser_parsable_css.js failures

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8855dd7a2aa85c0480d180b97d2916b7da06c002&selectedJob=248614518

Backout link: https://hg.mozilla.org/integration/autoland/rev/63fabcfd50cd8e06ac376e0d44d23ab535eef229

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

19:09:31 INFO - TEST-START | browser/base/content/test/static/browser_parsable_css.js
19:09:32 INFO - TEST-INFO | started process screencapture
19:09:32 INFO - TEST-INFO | screencapture: exit 0
19:09:32 INFO - <snipped 144 output lines - if you need more context, please use SimpleTest.requestCompleteLog() in your test>
19:09:32 INFO - Buffered messages logged at 19:09:31
19:09:32 INFO - Ignored error "Unknown property ‘-moz-math-display’. Declaration dropped." on resource://gre-resources/mathml.css because of whitelist item {"sourceName":"/\b(html|mathml|ua)\.css$/i","errorMessage":"/Unknown property.-moz-/i","isFromDevTools":false,"used":true}
...
19:09:32 INFO - Ignored error "Unknown pseudo-class or pseudo-element ‘-moz-svg-marker-anon-child’. Ruleset ignored due to bad selector." on resource://gre/res/svg.css because of whitelist item {"sourceName":"/\b(contenteditable|EditorOverride|svg|forms|html|mathml|ua|pluginproblem)\.css$/i","errorMessage":"/Unknown pseudo-class.-moz-/i","isFromDevTools":false,"used":true}
19:09:32 INFO - TEST-PASS | browser/base/content/test/static/browser_parsable_css.js | All the styles (219) loaded without errors. -
19:09:32 INFO - Buffered messages finished
19:09:32 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_parsable_css.js | Unused whitelist item: {"sourceName":"/(?:res|gre-resources)\/forms\.css$/i","errorMessage":"/Expected color but found \u2018-moz.*/i","isFromDevTools":false} -
19:09:32 INFO - Stack trace:
19:09:32 INFO - chrome://mochikit/content/browser-test.js:test_ok:1314
19:09:32 INFO - chrome://mochitests/content/browser/browser/base/content/test/static/browser_parsable_css.js:checkWhitelist:464
19:09:32 INFO - chrome://mochitests/content/browser/browser/base/content/test/static/browser_parsable_css.js:checkAllTheCSS:468
19:09:32 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1116
19:09:32 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1144
19:09:32 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:1005
19:09:32 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803
19:09:32 INFO - Leaving test bound checkAllTheCSS
19:09:32 INFO - GECKO(992) | MEMORY STAT | vsize 6091MB | residentFast 968MB | heapAllocated 712MB
19:09:32 INFO - TEST-OK | browser/base/content/test/static/browser_parsable_css.js | took 1002ms

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Ugh, that test sucks quite a bit :(

Fixed it.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c50e727f9245
Hide -moz-gtk-buttonactivetext from content. r=stransky

Comment 12

[Dorel Luca [:dluca]]

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

Comment 13

[Mats Palmgren (inactive)]

Thanks for fixing this Emilio, this is great. We now have the tool to eventually start unshipping some of these values...

Comment 14

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 9067592 [details]
Bug 1554433 - Move system colors to values::specified::color. r=#style,stransky

Beta/Release Uplift Approval Request

• User impact if declined: Systems colors are exposed to Web content.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It's a pretty automatic refactoring, and it only hides the value that was just introduced so it's not web-observable (and thus not really risky).
• String changes made/needed: none

Comment 15

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 9067593 [details]
Bug 1554433 - Hide -moz-gtk-buttonactivetext from content. r=stransky

Beta/Release Uplift Approval Request

• User impact if declined: System colors are exposed to web content.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It's a pretty automatic refactoring, and it only hides the value that was just introduced so it's not web-observable (and thus not really risky).
• String changes made/needed:

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 9067592 [details]
Bug 1554433 - Move system colors to values::specified::color. r=#style,stransky

sounds like a good thing to do before esr68 branches; approved for 68.0b9

Comment 17

[Razvan Maries]

I tried to uplift the requested attachment but I got an conflict while merging on: widget/nsXPLookAndFeel.cpp

Comment 18

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/570065ea87fc
https://hg.mozilla.org/releases/mozilla-beta/rev/82341a825fbc