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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

This at least affects bug966992-{1,2,3}.html in test_reftests_with_caret.html because of overflow-clip-box.

Comment 2

[Cameron McCormack (:heycam)]

This turns out to be the root cause of bug 1365822.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Manish, any suggestion what should we do here?

Comment 5

[Manish Goregaokar [:manishearth]]

We can have the pref/flag duplicated in the templates and autogenerate tests that ensure it matches up with gecko.

Comment 6

[Cameron McCormack (:heycam)]

I'll take a look at this.

Comment 7

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fc19632b6f4e006674d180ca8282a9fde9da223

Comment 8

[Cameron McCormack (:heycam)]

Attachment: Bug 1375374 - Test expectation adjustments.

Review commit: https://reviewboard.mozilla.org/r/153154/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/153154/

Comment 9

[Manish Goregaokar [:manishearth]]

Comment on attachment 8882062 [details]
Bug 1375374 - Test expectation adjustments.

https://reviewboard.mozilla.org/r/153154/#review158452

Comment 10

[Cameron McCormack (:heycam)]

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

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8882062 [details]
Bug 1375374 - Test expectation adjustments.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153154/diff/1-2/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c38bf53df24b
Test expectation adjustments. r=manishearth

Comment 13

[Phil Ringnalda (:philor)]

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