Closed Bug 1121768 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(4 files)

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).
Blocks: 1119475
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.
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. :)
Attachment #8549348 - Flags: review?(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
Attachment #8549347 - Flags: review?(dbaron) → review+
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
Attachment #8549348 - Flags: review?(dbaron) → review+
(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! :)
Attachment #8549349 - Flags: review?(dbaron) → review+
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
Attachment #8549350 - Flags: review?(dbaron) → review+
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.
(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.
You need to log in before you can comment on or make changes to this bug.