1121768 - shorthands enabled only in UA style sheets fail to be added to declarations when parsed

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

If we have a shorthand that is marked CSS_PROPERTY_ALWAYS_ENABLED_IN_UA_SHEETS, then using it in a UA style sheet while the pref is disabled will fail.  The property will parse correctly, but in the TransferFromBlock call we use CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES to iterate over the longhand components.  That macro calls the one-argument version of nsCSSProps::IsEnabled to check if each longhand component is enabled.  Probably we should be passing down an EnabledState value into TransferFromBlock so it can pass it to CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES (or a variant of it that takes such an argument).

Comment 1

[Cameron McCormack (:heycam)]

I think we can get away with just using either nsCSSProps::eEnabledForAllContent or nsCSSProps::eIgnoreEnabledState depending on the call site.  Specifically, we can use the latter in nsCSSExpandedDataBlock to solve this bug.  We don't really need to compute an EnabledState value based on the CSS parser's mUnsafeRulesEnabled/mIsChromeOrCertifiedApp values; it's unlikely we will have a shorthand that is e.g. CSS_PROPERTY_ALWAYS_ENABLED_IN_UA_SHEETS but whose longhand components aren't.  Maybe we should assert that's the case.

Comment 2

[Cameron McCormack (:heycam)]

Attachment: Part 1: Require that shorthands with CSS_PROPERTY_ALWAYS_ENABLED_IN_UA_SHEETS or CSS_PROPERTY_ALWAYS_ENABLED_IN_CHROME_OR_CERTIFIED_APP have those flags on all subproperties.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Part 2: Give CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES an nsCSSProps::EnabledState argument.

I was able to remove most of the monstrousness from the monstrosity I showed earlier, realising that the for loop actually lets me define variables of any number of types, as long as they are all pointers or references to the same underlying type. :)

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Part 3: Look at all subproperties (not just content-visible ones) in nsCSSExpandedDataBlock methods.

Comment 5

[Cameron McCormack (:heycam)]

Attachment: Part 4: Store token stream values in all subproperties of a shorthand with a variable reference.

Comment 6

[Cameron McCormack (:heycam)]

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

Comment 7

[David Baron :dbaron:]

Comment on attachment 8549347 [details] [diff] [review]
Part 1: Require that shorthands with CSS_PROPERTY_ALWAYS_ENABLED_IN_UA_SHEETS or CSS_PROPERTY_ALWAYS_ENABLED_IN_CHROME_OR_CERTIFIED_APP have those flags on all subproperties.

So just because you can do it using the preprocessor doesn't mean you should.  I'd really prefer that things that can operate on data tables stored elsewhere do that, rather than using more preprocessing.

Instead, you should loop from eCSSProperty_COUNT_no_shorthands to eCSSProperty_COUNT, within that loop over an array of the two flags you want to test, and within that loop (probably with for rather than while) over the subproperties (perhaps called subprop rather than p).

No preprocessing needed, other than #ifdef DEBUG ... #endif.

r=dbaron with that

Comment 8

[David Baron :dbaron:]

Comment on attachment 8549348 [details] [diff] [review]
Part 2: Give CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES an nsCSSProps::EnabledState argument.

Ah, the reason I've always preferred writing " *" rather than "* ". :-P

r=dbaron

Comment 9

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #8)
> Ah, the reason I've always preferred writing " *" rather than "* ". :-P

And conversely, the reason I prefer not to have two variables declared in the one statement when I can help it! :)

Comment 10

[David Baron :dbaron:]

Comment on attachment 8549350 [details] [diff] [review]
Part 4: Store token stream values in all subproperties of a shorthand with a variable reference.

Wouldn't it be more correct to use the enabled state that LookupEnabledProperty() uses?  Otherwise it seems like a shorthand could override something from the UA sheet (e.g., if the UA sheet needed to have a non-default value, like we had to do for object-fit before it was enabled).

I think I'd prefer if you did that; it should be easy to refactor most of LookupEnabledProperty into a function that returns enabledState.

If you agree, then r=dbaron with that; otherwise we can discuss further

Comment 11

[Cameron McCormack (:heycam)]

Turns out for part 3 I can't just use eIgnoreEnabledState, because nsCSSExpandedDataBlock::DoTransferFromBlock doesn't like being called with a property that doesn't exist on the block being transferred from.  So I think I will have to pass in an appropriate EnabledState value from the CSS parser.

Comment 12

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #10)
> Wouldn't it be more correct to use the enabled state that
> LookupEnabledProperty() uses?  Otherwise it seems like a shorthand could
> override something from the UA sheet (e.g., if the UA sheet needed to have a
> non-default value, like we had to do for object-fit before it was enabled).
> 
> I think I'd prefer if you did that; it should be easy to refactor most of
> LookupEnabledProperty into a function that returns enabledState.
> 
> If you agree, then r=dbaron with that; otherwise we can discuss further

Yes, I think you're right.  I'll use this EnabledState-computing function in part 3 and this part 4.

Comment 13

[Cameron McCormack (:heycam)]

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

Comment 14

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be159d9f6d92
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a6692c19d67
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1c3ce647646
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf403ce5f085

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/be159d9f6d92
https://hg.mozilla.org/mozilla-central/rev/9a6692c19d67
https://hg.mozilla.org/mozilla-central/rev/a1c3ce647646
https://hg.mozilla.org/mozilla-central/rev/cf403ce5f085