stylo: Match Gecko's handling of internal properties / prefed properties

RESOLVED FIXED in Firefox 56

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: xidorn, Assigned: heycam)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

In Gecko, there is a ENABLED_IN_UA_SHEETS flag for properties, combined with pref, there are four independent different states:
1. no pref, no flag: usable everywhere
2. no pref, has flag: usable in UA sheets, not usable in content sheets
3. has pref, no flag: the pref controls it everywhere
4. has pref, has flag: usable in UA sheets, the pref controls it in content sheets

(Actually we have a ENABLED_IN_UA_SHEETS_AND_CHROME which makes the property usable in chrome documents in addition to UA sheets.)

While in Stylo, we have an internal flag, which is checked independently than pref, and the relationship is:
1. no pref, no flag: usable everywhere
2. no pref, has flag: usable in UA sheets, not usable in content sheets
3. has pref, no flag: the pref controls it everywhere
4. has pref, has flag: the pref controls it in UA sheet, not usable in content


So there are two problems:
The main problem is that, in the last case, the behavior is different between Gecko and Stylo, which can cause some issues.
The other problem is that, because of the difference, we never mark properties which may be usable in content with "internal", which means there are properties which should go to (4) ends up being (2).
This at least affects bug966992-{1,2,3}.html in test_reftests_with_caret.html because of overflow-clip-box.
Blocks: 1320841
This turns out to be the root cause of bug 1365822.
Blocks: 1365822
One tricky thing is that, if we put the "enabled_in_ua_sheet" flag to property definitions in Servo code, once we remove a pref without removing the flag at the same time, the state of the property would suddenly jump from (4) to (2), which is unfortunate.

It is fine in Gecko because both pref and the flag are written in nsCSSPropList.h, so it is easier to remove them at the same time. But if they are put in separate places like in Stylo, it would be a bigger problem.

Tests would hopefully catch such change, though... just a bit painful for developer.
Manish, any suggestion what should we do here?
Flags: needinfo?(manishearth)
We can have the pref/flag duplicated in the templates and autogenerate tests that ensure it matches up with gecko.
Flags: needinfo?(manishearth)
I'll take a look at this.
Assignee: nobody → cam
Status: NEW → ASSIGNED
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fc19632b6f4e006674d180ca8282a9fde9da223
Comment hidden (mozreview-request)
Attachment #8882062 - Flags: review?(manishearth)

Comment 9

6 months ago
mozreview-review
Comment on attachment 8882062 [details]
Bug 1375374 - Test expectation adjustments.

https://reviewboard.mozilla.org/r/153154/#review158452
Attachment #8882062 - Flags: review?(manishearth) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a002d44ea562d9f78513f4624f6a7eb9e3b70abf
Comment hidden (mozreview-request)

Comment 12

6 months ago
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c38bf53df24b
Test expectation adjustments. r=manishearth

Comment 13

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c38bf53df24b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.