1893409 - InspectorUtils.getCSSStyleRules does not return CSSStartingStyleRule rules nor their children rules

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Nicolas Chevobbe [:nchevobbe]]

Investigating Bug 1892192, I applied the attached patches on Bug 1834876 and created a test page with a couple @starting-style rules: https://ffx-devtools-starting-style.glitch.me/

In the inspector, I'm not seeing the rules that are declared in @starting-style, although they seem to be taken into account, given I'm seeing the transition being applied.

But the inspector is not getting those rules when we call InspectorUtils.getCSSStyleRules in https://searchfox.org/mozilla-central/rev/f6e3b81aac49e602f06c204f9278da30993cdc8a/devtools/server/actors/page-style.js#751-755

const domRules = InspectorUtils.getCSSStyleRules(
  node,
  pseudo,
  CssLogic.hasVisitedState(node)
);

The InspectorUtils method is defined in InspectorUtils.cpp#232, but I can't figure out why CSSStartingStyleRule rules are not collected.

---

Boris, would you know why this is happening?

Comment 1

[Boris Chiou [:boris]]

I probably know the reason. InspectorUtils::GetCSSStyleRules() uses Servo_ComputedValues_GetStyleRuleList() to get the matched rules, by the computed values of this element. However, we don't apply @starting-style into the computed values. We resolve the rules inside @starting-style only when computing the CSS transitions.

I'm trying to figure out a way to provide an API to get all matched rules, including rules inside @starting-style.

Comment 2

[Boris Chiou [:boris]]

Attachment: Bug 1893409 - Make getCSSStyleRules() work for starting style as well.

Add a new API for Insepector: nspectorUtils.getCSSStartingStyleRules.
Its arguments are the same as getCSSStyleRules, and return the
starting style if any, for a given element.

Note: The starting style will be cleared if the transitions of this
element have been finished.

Comment 3

[Boris Chiou [:boris]]

I added a tentative patch which provides an extra API, nspectorUtils.getCSSStartingStyleRules. Its returned value should include the declaration blocks inside @starting-style. Could you please check if it meets your requirements?

Thanks.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Attachment: WIP: Bug 1893409 - [devtools] Add test for InspectorUtils.getCSSStyleRules for starting style.

Depends on D209318

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Boris Chiou [:boris] from comment #3)

I added a tentative patch which provides an extra API, nspectorUtils.getCSSStartingStyleRules. Its returned value should include the declaration blocks inside @starting-style. Could you please check if it meets your requirements?

Hello Boris, thanks a lot for adding this new method.
Unfortunately, it doesn't seem to return what I'm expecting.
I'm using it in https://phabricator.services.mozilla.com/D209329 , and inspecting the body or h1 in https://ffx-devtools-starting-style.glitch.me/ , I still can't see the starting-style rules.
I added a test for it in https://phabricator.services.mozilla.com/D209328 , and you can see that it fails, as getCSSStartingStyleRules doesn't return anything in this case, but I'm not sure what's happening.
Where you testing on a specific page?

Comment 6

[Boris Chiou [:boris]]

Basically, we resolve @starting-style only when we specify transition on this element (which means we don't have starting style if it's impossible to have transitions on the element), and it also follow the cascade order (i.e. latter one wins), so the style in the test case should be something like this:

  unknowntagname {
    color: blue;
    opacity: 1;
    transitions: all 1s; // This is necessary. We have to specify transitions with a non-zero combined duration.

    @starting-style {
      opacity: 0;
    }
  }

  // This should be after the declaration block of color:blue above because
  // we would like to let this wins, when resolving starting style.
  // Or if we intentionally ignore this, we can move this back to the top.
  @starting-style {
    unknowntagname {
      color: red;
    }
  }

And the returned value of InspectorUtils.getCSSStartingStyleRules(el) is (note: this is the rules in the rule tree and it follows the cascade order):

{
  "0":"unknowntagname {\n  color: blue; opacity: 1; transition: 1s;\n  @starting-style {\n  & { opacity: 0; }\n}\n}",
  "1":"& { opacity: 0; }",
  "2":"unknowntagname { color: red; }"
}

rules[1] is the declaration block inside the nested @starting-style, and rule[2] is the top-level starting-style rule.

Note"

1. It seems rules[0] also have the information of its nested block. It may be possible to filter it out if needed. But I expected this makes sense for CSS nesting.
2. The starting style is a very special style which is only used when creating transitions, and resolving it is not very cheap, so we try our best to avoid resolving it if we don't need it obviously. It is not like other @xxx rules, e.g. @keyframes, because we shouldn't take it into account for normal computed values of elements.

So maybe we still have to do some other things in https://phabricator.services.mozilla.com/D209329 because the returned value is a little bit different, i.e. we don't show @startring-style strings in the returned rules?

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Boris Chiou [:boris] from comment #6)

Basically, we resolve @starting-style only when we specify transition on this element (which means we don't have starting style if it's impossible to have transitions on the element), and it also follow the cascade order (i.e. latter one wins), so the style in the test case should be something like this:

  unknowntagname {
    color: blue;
    opacity: 1;
    transitions: all 1s; // This is necessary. We have to specify transitions with a non-zero combined duration.

    @starting-style {
      opacity: 0;
    }
  }

  // This should be after the declaration block of color:blue above because
  // we would like to let this wins, when resolving starting style.
  // Or if we intentionally ignore this, we can move this back to the top.
  @starting-style {
    unknowntagname {
      color: red;
    }
  }

And the returned value of InspectorUtils.getCSSStartingStyleRules(el) is (note: this is the rules in the rule tree and it follows the cascade order):

{
  "0":"unknowntagname {\n  color: blue; opacity: 1; transition: 1s;\n  @starting-style {\n  & { opacity: 0; }\n}\n}",
  "1":"& { opacity: 0; }",
  "2":"unknowntagname { color: red; }"
}

Ah right, I can see it working now, I updated the revision on phabricator

Note"

1. It seems rules[0] also have the information of its nested block. It may be possible to filter it out if needed. But I expected this makes sense for CSS nesting.

If possible, it would be nice to not return it. For now, I'm filtering the result to only include rules whose parent rule is a CSSStartingStyleRule

2. The starting style is a very special style which is only used when creating transitions, and resolving it is not very cheap, so we try our best to avoid resolving it if we don't need it obviously. It is not like other @xxx rules, e.g. @keyframes, because we shouldn't take it into account for normal computed values of elements.

Right. I wonder if that could be weird for developers because they won't see the starting-style rules until the transition is triggered, but we might follow up on this

So maybe we still have to do some other things in https://phabricator.services.mozilla.com/D209329 because the returned value is a little bit different, i.e. we don't show @startring-style strings in the returned rules?

I updated https://phabricator.services.mozilla.com/D209329 and I can see starting style rules in some cases.
It seems that there's something odd with InspectorUtils.getCSSStartingStyleRules for transitions triggered before the inspector is opened/registered?
I have this test page https://ffx-devtools-starting-style.glitch.me/ , where I have a few @starting-style applying to the body, the h1 and div
If I inspect the body or the h1, I'm not getting the starting style (which is properly applies, the transition is visible on screen)
The only way I can see the starting style rules is for element that are added while the inspector is already open.
So for my test page, it's visible on divs that are added with the "Add div" button.

Would you know what's happening?

Comment 8

[Boris Chiou [:boris]]

(In reply to Nicolas Chevobbe [:nchevobbe][PTO - back on Monday May 13th] from comment #7)

If possible, it would be nice to not return it. For now, I'm filtering the result to only include rules whose parent rule is a CSSStartingStyleRule

It seems we don't have extra information in the rule node to know if this style rule is in @starting-style. I may miss something or need to dig deeper. I could check this later.

Right. I wonder if that could be weird for developers because they won't see the starting-style rules until the transition is triggered, but we might follow up on this

Thanks. Resolving it earlier and keeping this value longer need to tweak the framework. We might have to change a lot, but yes, we could follow up on this.

I updated https://phabricator.services.mozilla.com/D209329 and I can see starting style rules in some cases.
It seems that there's something odd with InspectorUtils.getCSSStartingStyleRules for transitions triggered before the inspector is opened/registered?
I have this test page https://ffx-devtools-starting-style.glitch.me/ , where I have a few @starting-style applying to the body, the h1 and div
If I inspect the body or the h1, I'm not getting the starting style (which is properly applies, the transition is visible on screen)
The only way I can see the starting style rules is for element that are added while the inspector is already open.
So for my test page, it's visible on divs that are added with the "Add div" button.

Would you know what's happening?

I will check this soon. (Keep my ni until I find the root cause.) Thanks.

Comment 9

[Boris Chiou [:boris]]

It looks like we hit this assertion. In other words, we cannot map this rule to the style rule map.

However, if we put the style into the same file, e.g. put every thing in style.css into the html file, we can see the starting style again.

I suspect I forgot to add starting style rules into ServoStyleRuleMap, which seems only for Inspector.

I'm still trying to figure out a way to fix this API anyway.

Updated: It seems we recreate the style rules when opening Inspector, if we are using a separate style file (e.g. style.css in the test). And we fill the ServoStyleRuleMap with the new created rules. However, the starting style is still linked to the old style rules, so we cannot map it to the properly style rule in ServoStyleRuleMap.

Comment 10

[Boris Chiou [:boris]]

Attachment: Bug 1893409 - Move resolve_starting_style() and after_change_style() into StyleResolverForElement.

We would like to add an API to resolve the starting style from Gecko, so
make them public. (And I think it'd be more suitable to put them into
StyleResolverForElement).

Comment 11

[Boris Chiou [:boris]]

I updated https://phabricator.services.mozilla.com/D209329 and I can see starting style rules in some cases.
It seems that there's something odd with InspectorUtils.getCSSStartingStyleRules for transitions triggered before the inspector is opened/registered?
I have this test page https://ffx-devtools-starting-style.glitch.me/ , where I have a few @starting-style applying to the body, the h1 and div
If I inspect the body or the h1, I'm not getting the starting style (which is properly applies, the transition is visible on screen)
The only way I can see the starting style rules is for element that are added while the inspector is already open.
So for my test page, it's visible on divs that are added with the "Add div" button.
Would you know what's happening?

The newer version should fix them. We generate the starting style when you call this API, based on the current style rules we create.

Also, it's not necessary to specify transition property in this element now.

This API returns the starting style if the given element has any rules inside @starting-style, anytime when you call this API.

However, the returned rule list still have rules which are not in @starting-style. We could do that later.

Please let me know if there are any cases that the patches don't work. Thanks for your debugging and verifying.

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Boris Chiou [:boris] from comment #11)

I updated https://phabricator.services.mozilla.com/D209329 and I can see starting style rules in some cases.
It seems that there's something odd with InspectorUtils.getCSSStartingStyleRules for transitions triggered before the inspector is opened/registered?
I have this test page https://ffx-devtools-starting-style.glitch.me/ , where I have a few @starting-style applying to the body, the h1 and div
If I inspect the body or the h1, I'm not getting the starting style (which is properly applies, the transition is visible on screen)
The only way I can see the starting style rules is for element that are added while the inspector is already open.
So for my test page, it's visible on divs that are added with the "Add div" button.
Would you know what's happening?

The newer version should fix them. We generate the starting style when you call this API, based on the current style rules we create.

Also, it's not necessary to specify transition property in this element now.

This API returns the starting style if the given element has any rules inside @starting-style, anytime when you call this API.

However, the returned rule list still have rules which are not in @starting-style. We could do that later.

Please let me know if there are any cases that the patches don't work. Thanks for your debugging and verifying.

Thanks a lot Boris, everything seems to work well I think 👍
I'll go on with the DevTools patch, add tests, and let you know if I come across any issue

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

Boris, I'm realizing that properties inside @starting-style can be overridden by later property definition.
So given the following CSS

div {
  transition: all 1s linear;
}

@starting-style {
  div {
    border-color: cyan;
    background-color: blue; /* this won't be applied … */
  }
}

div {
  background-color: transparent; /* … because this overrides it */
}

And so, what we usually do in Devtools is that we strike through properties that are overridden.
We're doing that with some custom logic in DevTools, which relies on the order of the rules.
So, ideally, in the Inspector, we'd show the following

div {
  background-color: transparent;
}

@starting-style {
  div {
    border-color: cyan;
    ~~background-color: blue;~~
  }
}

div {
  transition: all 1s linear;
}

where we can detect that the blue background color within starting-style will be ignored.
But here, for now, since we're getting the starting-style rules separately from the other rules, we can't know how they are actually sorted.

Ideally, it would be nice to get those rules directly from Inspector.getCSSStyleRules. Is this something that could be done?

Comment 14

[Boris Chiou [:boris]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #13)

Boris, I'm realizing that properties inside @starting-style can be overridden by later property definition.
So given the following CSS

div {
  transition: all 1s linear;
}

@starting-style {
  div {
    border-color: cyan;
    background-color: blue; /* this won't be applied … */
  }
}

div {
  background-color: transparent; /* … because this overrides it */
}

And so, what we usually do in Devtools is that we strike through properties that are overridden.
We're doing that with some custom logic in DevTools, which relies on the order of the rules.

Per the above example, I replace div with unknowtagname, and put them into test_getCSSStartingStyleRules.html, and then dump the log by ${JSON.stringify(rules.map(rule => rule.cssText))}).
Now it shows:

// Note that this follows the cascade order in the rule tree.
{
  "0":"unknowntagname { transition: 1s linear; }",
  "1":"unknowntagname { border-color: cyan; background-color: blue; }", // the background-color is overridden because the latter one wins.
  "2":"unknowntagname { background-color: transparent; }"
}

It includes the overridden rules (i.e. background-color: blue).

Basically, the API I wrote returns the rules after we do the cascade, including the overridden rules (no matter whether it comes from @starting-style or not). Are they enough for you?

But here, for now, since we're getting the starting-style rules separately from the other rules, we can't know how they are actually sorted.

The API should provide all sorted rules (based on the cascade order). I guess in DevTools there is a special logic to filter out only rules inside @starting-style, so perhaps we can reuse the original returned value from CSSStartingStyleRule to know the overridden rule?

Otherwise, I'm OK to provide any other APIs if needed.

Comment 15

[Boris Chiou [:boris]]

Ideally, it would be nice to get those rules directly from Inspector.getCSSStyleRules. Is this something that could be done?

Because it has different meaning.

Inspector.getCSSStyleRules returns the rules we are using for primary style. The is the rules we are using for rendering and what the users see on the browser. All rules inside @starting-style are not used, per spec. This means any rules inside @starting-style do not override rules outside.

Inspector.getCSSStartingStyleRules returns the rules we are using only for computing transitions. This value is used only for creating transitions and we don't use them in layout. I intentionally use a separate API because the cascade result may be different: the rules inside @starting-style may overrides rules outside, per cascade order.

If it is better for devtools to merge them together, e.g. if there are any matched rules inside @starting-style, we return the rules just like Inspector.getCSSStartingStyleRules; otherwise, we use the original logic from Inspector.getCSSStyleRules, I can provide a separate patch for it.

Updated: Please check the new patch I uploaded.
Note:

div {
  transition: all 1s linear;
}

div {
  ~background-color: transparent;~ /* This is ignored in starting style, but is not ignored in the computed style. */
}

@starting-style {
  div {
    border-color: cyan;
    background-color: blue;
  }
}

In this example, it may confuse people because the ignored rule is ignored only in starting style.

Comment 16

[Boris Chiou [:boris]]

Attachment: Bug 1893409 - Merge getCSSStartingStyleRules into getCSSStyleRules.

This merges the logic of getCSSStartingStyleRules into getCSSStyleRules.

If there are any matched rules inside @starting-style, we return the
starting style; otherwise, we return the computed style, for a given
element.

Note that this may confuse people because the rules inside @starting-style
should override rules outside in the primay style of the given element.

Comment 17

[Nicolas Chevobbe [:nchevobbe]]

Also, I think that rules(In reply to Boris Chiou [:boris] from comment #15)

If it is better for devtools to merge them together, e.g. if there are any matched rules inside @starting-style, we return the rules just like Inspector.getCSSStartingStyleRules; otherwise, we use the original logic from Inspector.getCSSStyleRules, I can provide a separate patch for it.
Updated: Please check the new patch I uploaded.

The provided patch works nicely and will allow us to more accurately mark the overridden properties, thanks a lot!
would it be possible to add a new parameter to Inspector.getCSSStyleRules so we can chose to not retrieve starting-style rules? We're calling the function with the parent nodes of the select nodes to get inherited style, and in such case, we don't want the starting-style rules (unless we can inherit properties from starting-style rules? in my testing, that wasn't the case)

Note:

div {
  transition: all 1s linear;
}

div {
  ~background-color: transparent;~ /* This is ignored in starting style, but is not ignored in the computed style. */
}

@starting-style {
  div {
    border-color: cyan;
    background-color: blue;
  }
}

In this example, it may confuse people because the ignored rule is ignored only in starting style.

Yes, I'll did some testing and there is some work to be done in the inspector to clarify the situation (see https://bugzilla.mozilla.org/show_bug.cgi?id=1892192#c3)

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

By the way Boris, would you know if there's a valid case for making @starting-style properties being overrid-able by rules not inside starting style? I'm curious why it was spec'd that way, as I found it quite surprising (but I guess I'm missing some cases?)

Comment 19

[Boris Chiou [:boris]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #17)

would it be possible to add a new parameter to Inspector.getCSSStyleRules so we can chose to not retrieve starting-style rules? We're calling the function with the parent nodes of the select nodes to get inherited style, and in such case, we don't want the starting-style rules (unless we can inherit properties from starting-style rules? in my testing, that wasn't the case)

I added a parameter:

  sequence<CSSStyleRule> getCSSStyleRules(
    Element element,
    optional [LegacyNullToEmptyString] DOMString pseudo = "",
    optional boolean relevantLinkVisited = false,
    optional boolean forStartingStyle = false);

So now if forStartingStyle is false, we are always returning the rules of its computed style. If it is true, we return rules if it has rules inside @starting-style. However, it returns empty sequence if there is no rules inside @starting-style. I also tweak the test case to make sure it works.

Comment 20

[Boris Chiou [:boris]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #18)

By the way Boris, would you know if there's a valid case for making @starting-style properties being overrid-able by rules not inside starting style? I'm curious why it was spec'd that way, as I found it quite surprising (but I guess I'm missing some cases?)

I guess we make it overridable by rules outside so the author can disable it by CSSOM (e.g. add new style rules or inline style), if the author don't want to create transitions in some cases. (However, I didn't join the spec discussion before, so I don't have the context and examples for it.)

In any case, it makes us easier to reuse the current logic of CSS cascade if we let @starting-style follow the cascade order as well. Perhaps just a sugar for implementation and for backward compatibility.

Comment 21

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Boris Chiou [:boris] from comment #19)

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #17)

would it be possible to add a new parameter to Inspector.getCSSStyleRules so we can chose to not retrieve starting-style rules? We're calling the function with the parent nodes of the select nodes to get inherited style, and in such case, we don't want the starting-style rules (unless we can inherit properties from starting-style rules? in my testing, that wasn't the case)

I added a parameter:

  sequence<CSSStyleRule> getCSSStyleRules(
    Element element,
    optional [LegacyNullToEmptyString] DOMString pseudo = "",
    optional boolean relevantLinkVisited = false,
    optional boolean forStartingStyle = false);

So now if forStartingStyle is false, we are always returning the rules of its computed style. If it is true, we return rules if it has rules inside @starting-style. However, it returns empty sequence if there is no rules inside @starting-style. I also tweak the test case to make sure it works.

For some reason I missed this comment, thanks for working on this Boris.
I probably wasn't specific in my request before. What I'd like is withStartingStyle, rather than forStartingStyle .
So if false it will work as it works now, and if true, it will return all the rules it returns at the moment, and eventually the starting style that applies to the element. Would that be possible?

With https://phabricator.services.mozilla.com/D209329 applied on top of your patches, I'd expect the added/updated tests, devtools/client/inspector/computed/test/browser_computed_matched-selectors-order.js and devtools/client/inspector/rules/test/browser_rules_at_starting-style.js to pass, as well as existing tests (e.g. devtools/client/inspector/rules/test/browser_rules_layer.js)

Comment 22

[Boris Chiou [:boris]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #21)

For some reason I missed this comment, thanks for working on this Boris.
I probably wasn't specific in my request before. What I'd like is withStartingStyle, rather than forStartingStyle .

OK, I will rename the name of this argument. i.e. replace forStartingStyle with withStartingStyle.

So if false it will work as it works now, and if true, it will return all the rules it returns at the moment, and eventually the starting style that applies to the element. Would that be possible?

OK. If false, everything keeps the same.
If true we return the starting style. If there is no rule inside @starting-style, we return the styles of this element instead, just like false case. (i.e. No matter where there are any rules inside @starting-style, it always returns the rules for this element.)

Note: I intentionally mentioned "if there is no rule inside @starting-style" because we early return in the function of computing starting style if there is no matched rule inside @starting-style, to avoid redundant rule matching and cascading.

It seems I can pass these tree tests now with my patches, at least, after applying https://phabricator.services.mozilla.com/D209329:
devtools/client/inspector/computed/test/browser_computed_matched-selectors-order.js
devtools/client/inspector/rules/test/browser_rules_at_starting-style.js
devtools/client/inspector/rules/test/browser_rules_layer.js

The try looks OK for devtools tests (https://treeherder.mozilla.org/jobs?repo=try&revision=acbcada3608118b545cbf79722bf070a5ac23f3c&selectedTaskRun=c2RZ_y9OQw2TS5ZjhsXdfQ.1). (Note that there is one orange but I guess it is an intermittent.)

Comment 23

[Nicolas Chevobbe [:nchevobbe]]

Thanks a lot Boris, that's perfect

Comment 24

[Boris Chiou [:boris]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #23)

Thanks a lot Boris, that's perfect

Awesome. I am refining the patches and updating the title for review. Thanks for checking the spec of this API.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/654340d6d78c
Move resolve_starting_style() and after_change_style() into StyleResolverForElement. r=layout-reviewers,firefox-style-system-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/e0770559552a
Make getCSSStyleRules() work for starting style as well. r=layout-reviewers,firefox-style-system-reviewers,nchevobbe,emilio

Comment 26

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/654340d6d78c
https://hg.mozilla.org/mozilla-central/rev/e0770559552a