Open Bug 1294863 Opened 3 years ago Updated 11 months ago

Properties & Values API: Change in custom properties registrations should trigger @supports conditions recheck

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

People

(Reporter: jyc, Assigned: jyc, NeedInfo)

References

(Depends on 1 open bug)

Details

Attachments

(3 files)

This is part of implementing the Properties & Values API that has been split off into a separate bug to make Bug 1273706 more manageable.

With the implementation in 1273706, CSS.registerProperty/unregisterProperty will not trigger re-checking of @supports conditions. (@supports conditions previously never needed to be rechecked -- whether or not the rule group would would be used could be determined at parse-time).
Assignee: nobody → jyc
I'm worried this will cause us to do a lot of work if -- in a case I think won't be too uncommon -- we register a few properties at once.  Each call to document.registerProperty() would cause us to re-build the cascade immediately.

I think your GetRuleCascade changes can result in us getting the wrong style, since we don't invalidate the cached rule cascades when the registrations change, we just choose not to use a cached rule cascade at this time.  A following change in media feature will allow us to choose one of the old cached rule cascades.

ClearRuleCascades would be better, as it will remove the cached rule cascades, but that method currently assumes it's being called from CSSStyleSheet::ClearRuleCascades (in response to a style sheet mutation).

I feel like it might be best just to re-use the media queries mechanisms, despite the extra work that nsPresContext::MediaFeatureValuesChanged notifying responsive images and DOM MediaQueryList objects.  (They'll discover that the media features they're interested in haven't changed.)

I think the simplest way to do that would be to add a uint32_t to nsMediaQueryResultCacheKey that stores the custom property registration generation at the time a CSSSupportsRule::UseForPresentation is called.  Since we would only set this field if an @supports rule had a variable registration in it (we can have it be -1 otherwise), we wouldn't need to pay the cost of re-building the cascade if a custom property is registered but we don't have any @supports rules that could be affected by it.

Also, since we would use PostMediaFeaturesChanged we would coalesce multiple registerProperty calls before a style flush.

jyc, dbaron, do you think this sounds workable, and reasonable?  (Though I'm still not quite sure whether @supports should be affected by variable registrations, and the GitHub issue on it is still open.)
Flags: needinfo?(dbaron)
Hi heycam, thanks for looking it over. I am fine with whatever dbaron says. I considered making it a media feature, but that seemed pretty hacky.
Comment on attachment 8780719 [details]
Bug 1294863 - Part 3: Add tests for CSS variable registrations changing causing reevaluation of @supports conditions.

https://reviewboard.mozilla.org/r/71364/#review71902

r=me but like my previous comments, we might need to be more careful when relying on serialiations here.  Also please add some similar tests for CSS.supports() (both the 1-argument and 2-argument versions) with custom properties.

::: testing/web-platform/tests/css-houdini/properties-and-values.html:1000
(Diff revision 2)
> +      // Hopefully don't mess up future tests, although this itself might throw.
> +      CSS.unregisterProperty("--my-property");

Future tests will run in a new document so should have fresh registrations anyway.
Attachment #8780719 - Flags: review?(cam) → review+
Comment on attachment 8780717 [details]
Bug 1294863 - Part 1: Expose nsCSSRuleProcessor's RefreshRuleCascade as a virtual function (default no-op) on nsIStyleRuleProcessor, up through nsStyleSet and nsPresContext.

Cancelling review here for now.
Attachment #8780717 - Flags: review?(cam)
Comment on attachment 8780717 [details]
Bug 1294863 - Part 1: Expose nsCSSRuleProcessor's RefreshRuleCascade as a virtual function (default no-op) on nsIStyleRuleProcessor, up through nsStyleSet and nsPresContext.

https://reviewboard.mozilla.org/r/71360/#review120342
Comment on attachment 8780718 [details]
Bug 1294863 - Part 2: Make CSS variable registrations changing cause reevaluation of @supports conditions.

https://reviewboard.mozilla.org/r/71362/#review71658

::: layout/style/nsCSSRules.h:599
(Diff revision 2)
> +
> +  // Keep track of whether or not mCondition contains any variable references.
> +  // If it does, we will need to re-evaluate mCondition when UseForPresentation
> +  // is called, as variable registrations might change.
> +  bool mHasVariables;

Put this up near mUseGroup to pack the fields together better.
Attachment #8780718 - Flags: review?(cam)
You need to log in before you can comment on or make changes to this bug.