Closed Bug 1554433 Opened 6 months ago Closed 6 months ago

Move system colors outside of mako

Categories

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

task

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox68 --- fixed
firefox69 --- fixed

People

(Reporter: emilio, Assigned: emilio)

Details

Attachments

(4 files)

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.

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 :)

This prevents exposing the value to web content.

Depends on D32610

Keywords: leave-open
Priority: -- → P3
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3415f556123b
Move system colors to values::specified::color. r=xidorn
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8855dd7a2aa8
Hide -moz-gtk-buttonactivetext from content. r=stransky

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

Type: defect → task

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

Flags: needinfo?(emilio)

Ugh, that test sucks quite a bit :(

Fixed it.

Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c50e727f9245
Hide -moz-gtk-buttonactivetext from content. r=stransky
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED

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

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
Attachment #9067592 - Flags: approval-mozilla-beta?

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:
Attachment #9067593 - Flags: approval-mozilla-beta?
Target Milestone: --- → mozilla69

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

Attachment #9067592 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9067593 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

You need to log in before you can comment on or make changes to this bug.