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

Status: RESOLVED INVALID

(Core :: Layout, defect)

Description

[Jonathan Chan [:jyc]]

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

Comment 1

[Jonathan Chan [:jyc]]

Attachment: Bug 1294863 - Part 1: Expose nsCSSRuleProcessor's RefreshRuleCascade as a virtual function (default no-op) on nsIStyleRuleProcessor, up through nsStyleSet and nsPresContext.

RefreshRuleCascade is currently called by MediumFeaturesChanged. Refreshing the
rule cascade causes us to check which rule groups we need to apply (if media
feature values have changed). The checks for individual rule groups are
implemented in MediaRule::UseForPresentation. (MediaRule::UseForPresentation).

This patch series will change the behavior of SupportsRule::UseFromPresentation
to support custom property registrations changing, requiring re-evaluation of
the @supports condition. When registrations change, we want to refresh the rule
cascade so that SupportsRule::UseFromPresentation is re-evaluated.

We could call nsPresContext::MediaFeatureValuesChanged, but that does extra
work, and CSS variable registrations are not really media features.

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

Comment 2

[Jonathan Chan [:jyc]]

Attachment: Bug 1294863 - Part 2: Make CSS variable registrations changing cause reevaluation of @supports conditions.

We implement this as follows:

(1) When parsing @supports conditions, we keep track of whether or not we have
    seen any variables. We store this in the SupportsRule.
(2) In SupportsRule::UseForPresentation, if the condition didn't contain any
    variables, we can always just return the result of evaluating the condition
    at parse time. Otherwise, we reevaluate.
(3) After registerProperty/unregisterProperty are called, we call
    nsPresContext::RefreshRuleCascade, which was added in the previous patch.
(4) nsCSSRuleProcessor keeps track of the CSS variables registrations
    generation at which we last refreshed the rule cascade.
(5) Previously, we returned early from RefreshRuleCascade if all media feature
    values were the same. Now we also check that the variable registrations
    generation is the same as the generation we stored in (4).
    This causes us to refresh the rule cascade either if a custom property has
    been registered/unregistered or if a media feature value has changed.

This makes checking whether or not we should use supports rules more expensive
when they contain variables, but should have no other significant performance
impact. It might be possible to parse supports conditions into another format
instead of just reevaluating from the string in the future.

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

Comment 3

[Jonathan Chan [:jyc]]

Attachment: Bug 1294863 - Part 3: Add tests for CSS variable registrations changing causing reevaluation of @supports conditions.

We add another reftest and extend the Properties & Values API Web Platform Test.

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

Comment 4

[Jonathan Chan [:jyc]]

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.

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

Comment 5

[Jonathan Chan [:jyc]]

Comment on attachment 8780718 [details]
Bug 1294863 - Part 2: Make CSS variable registrations changing cause reevaluation of @supports conditions.

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

Comment 6

[Jonathan Chan [:jyc]]

Comment on attachment 8780719 [details]
Bug 1294863 - Part 3: Add tests for CSS variable registrations changing causing reevaluation of @supports conditions.

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

Comment 7

[Cameron McCormack (:heycam)]

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

Comment 8

[Jonathan Chan [:jyc]]

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 9

[Cameron McCormack (:heycam)]

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.

Comment 10

[Cameron McCormack (:heycam)]

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.

Comment 11

[Cameron McCormack (:heycam)]

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 12

[Cameron McCormack (:heycam)]

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.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

Redirect a needinfo that is pending on an inactive user to the triage owner.
:dholbert, since the bug has recent activity, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 15

[Frédéric Wang (:fredw)]

(In reply to Cameron McCormack (:heycam) from comment #7)

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

The spec was changed in 2019 and now contains this section:

As stated in § 2.2 Parse-Time Behavior, both unregistered and registered custom properties accept (almost) all possible values at parse-time. Registered custom properties only apply their syntax at computed value time.
So, all custom properties, regardless of whether they’re registered or unregistered, will test as "true" in an @supports rule, so long as you don’t violate the (very liberal) generic syntax for custom properties.

See https://drafts.css-houdini.org/css-properties-values-api-1/#conditional-rules

Relevant revisions:
https://github.com/w3c/css-houdini-drafts/commit/0eeca05a86688bdf74b6fdc8c4aea712b24cdcb2
https://github.com/w3c/css-houdini-drafts/commit/75ec921d1a842492698f2bb51a9be5ae2023b12c
https://github.com/w3c/css-houdini-drafts/commit/d3c7da48d09d50abfc065808486bd4c3249b9d74

Also, the following test was written to check that change and we currently pass it:
css/css-properties-values-api/conditional-rules.html

Closing this as INVALID.

Comment 16

[Frédéric Wang (:fredw)]

Change dependency since bug 1273706 is now used as a meta bug.