1391198 - stylo: The order of rules in DevTools is not specificity order when all declarations in a rule are !important.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Chih-Hsuan Kuo [:tommykuo]]

The order of rules in DevTools on stylo are different to gecko. It makes this test case[1] failed. Please see the attachment.

[1]: https://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/test/browser_rules_custom.js

Comment 1

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: stylo.png

Comment 2

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: gecko.png

Comment 3

[Chih-Hsuan Kuo [:tommykuo]]

The specificity order seems already fixed by someone. But the test case is still failed with `TEST-UNEXPECTED-FAIL` on the part of the important rule.

Comment 4

[Chih-Hsuan Kuo [:tommykuo]]

Hmmm... The issue still exists.

Comment 5

[Chih-Hsuan Kuo [:tommykuo]]

After removing these lines[1], the order of rules would be specificity order. It seems used to fix bug 1387906.

[1]: https://searchfox.org/mozilla-central/rev/18c16ebf818abb86805ce08a6e537e4cd826f044/servo/ports/geckolib/glue.rs#1929-1936

Comment 6

[Chih-Hsuan Kuo [:tommykuo]]

Please ignore comment 5, I'm wrong. It should be [1] fix the specificity order.


[1]: https://searchfox.org/mozilla-central/rev/18c16ebf818abb86805ce08a6e537e4cd826f044/servo/ports/geckolib/glue.rs#1929-1936

Comment 7

[Chih-Hsuan Kuo [:tommykuo]]

Okay, I think I figured out why sometimes the order is correct and sometime not.

- When all declarations are important, the order IS NOT specificity order.
- When any declaration is not important, the order IS specificity order

I think this line[1] make it correct when any declaration is not important.

And the code[2] I mentioned before, it doesn't affect this issue when all declarations are important.

[1]: https://searchfox.org/mozilla-central/rev/18c16ebf818abb86805ce08a6e537e4cd826f044/servo/components/style/rule_tree/mod.rs#224
[2]: https://searchfox.org/mozilla-central/rev/18c16ebf818abb86805ce08a6e537e4cd826f044/servo/ports/geckolib/glue.rs#1929-1936

Comment 8

[Chih-Hsuan Kuo [:tommykuo]]

Hi, emilio. Is it possible to know which selector is matched for a rule in [1]? I want to get the selectors and calculate their specificity, then sort them before return.

[1]: https://searchfox.org/mozilla-central/rev/18c16ebf818abb86805ce08a6e537e4cd826f044/servo/ports/geckolib/glue.rs#1924-1927

Comment 9

[Emilio Cobos Álvarez (:emilio)]

It is not, as far as I can tell. Rule nodes are per rule, not per selector.

Comment 10

[Chih-Hsuan Kuo [:tommykuo]]

I have an idea:

- Get the rule list in inspector
- Calculate the specificity for each rule (I think we can know which selector is matched in inspector)
- Sort by the specificity

Hi jryans, what do you think this idea? But I have a question, where do we get the rule list in inspector?

Comment 11

[Chih-Hsuan Kuo [:tommykuo]]

I think we get the rule list here[1]?

[1]: https://searchfox.org/mozilla-central/rev/18c16ebf818abb86805ce08a6e537e4cd826f044/devtools/server/css-logic.js#521

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Tommy Kuo [:kuoe0] at UTC+8 from comment #10)
> I have an idea:
> 
> - Get the rule list in inspector
> - Calculate the specificity for each rule (I think we can know which
> selector is matched in inspector)
> - Sort by the specificity
> 
> Hi jryans, what do you think this idea? But I have a question, where do we
> get the rule list in inspector?

Hmm, we could consider doing something like this, but could we instead just always add to the rule tree at the normal level to solve this?  For example, if we remove the condition[1] `if any_normal || !any_important` and just always run that block, does it fix the issue?

It feels a bit strange to have to fix this at the DevTools layer, but we can think about that if we have to do it.

[1]: https://searchfox.org/mozilla-central/rev/18c16ebf818abb86805ce08a6e537e4cd826f044/servo/components/style/rule_tree/mod.rs#216

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Tommy Kuo [:kuoe0] at UTC+8 from comment #11)
> I think we get the rule list here[1]?
> 
> [1]:
> https://searchfox.org/mozilla-central/rev/
> 18c16ebf818abb86805ce08a6e537e4cd826f044/devtools/server/css-logic.js#521

Yes, that looks like the place where DevTools retrieves rules for the rule view.

Comment 14

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: Bug 1391198 - Update expectation for passed test cases

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

Comment 15

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: Bug 1391198 - Make the order of rules in DevTools be the specificity order.

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

Comment 16

[Chih-Hsuan Kuo [:tommykuo]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #12)
> Hmm, we could consider doing something like this, but could we instead just
> always add to the rule tree at the normal level to solve this?  For example,
> if we remove the condition[1] `if any_normal || !any_important` and just
> always run that block, does it fix the issue?

In my patch, I removed this line you mentioned and also change the function `Servo_ComputedValues_GetStyleRuleList` to avoid to get the important one. It makes this test case passed. And I also ran a full try, I think we need to check other test cases.

And with this method, it would make the rule contained only important rules be inserted into rule tree twice. Maybe it's okay, we do the same thing for the rule contained both normal and important rules. If we are sure to use this way, I think this patch need to be reviewed by someone familiar with rule tree.

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8902194 [details]
Bug 1391198 - Make the order of rules in DevTools be the specificity order.

https://reviewboard.mozilla.org/r/173678/#review179118

This seems reasonable to me at a high level, but I am not an expert on the rule tree.  Previously, we were special casing a rule with ALL important values, which seems like a fairly rare thing to have anyway.  Now we do an extra insertion in that case, but it feels easier to follow the code now, since we take the same path no matter how many important values are present.

::: servo/components/style/rule_tree/mod.rs:213
(Diff revision 1)
>              }
> -            // We really want to ensure empty rule nodes appear in the rule tree for
> -            // devtools, this condition ensures that if we find an empty rule node, we
> -            // insert it at the normal level.
> -            if any_normal || !any_important {
> -                if matches!(level, Transitions) && found_important {
> +            if matches!(level, Transitions) && found_important {

We may want to keep a comment here about DevTools depending on rules to be inserted unconditionally here, since it could be surprising to future readers.

::: servo/ports/geckolib/glue.rs:1930
(Diff revision 1)
>          let style_rule = match *node.style_source() {
>              StyleSource::Style(ref rule) => rule,
>              _ => continue,
>          };
>  
> +            }

These braces seem out of place, should they be removed?

::: servo/ports/geckolib/glue.rs:1933
(Diff revision 1)
>          };
>  
> +            }
> +        }
> +
>          if node.importance().important() {

A comment about why this makes sense would be good to have.

Comment 18

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I believe this change will also fix:

devtools/client/inspector/rules/test/browser_rules_edit-property_06.js
devtools/client/inspector/rules/test/browser_rules_mark_overridden_03.js

These are currently skipped right now, so try unskipping them in your try run.

Comment 19

[Chih-Hsuan Kuo [:tommykuo]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #18)
> I believe this change will also fix:
> 
> devtools/client/inspector/rules/test/browser_rules_edit-property_06.js
> devtools/client/inspector/rules/test/browser_rules_mark_overridden_03.js
> 
> These are currently skipped right now, so try unskipping them in your try
> run.

I already tried that and I found `browser_rules_add-property-and-reselect.js` can also be passed now.

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

Comment 20

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8902193 [details]
Bug 1391198 - Update expectation for passed test cases

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

Comment 21

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8902194 [details]
Bug 1391198 - Make the order of rules in DevTools be the specificity order.

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

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8902194 [details]
Bug 1391198 - Make the order of rules in DevTools be the specificity order.

https://reviewboard.mozilla.org/r/173678/#review179900

This looks good, but again, I'm not sure I love regressing performance of hot code to satisfy devtools... Let me sleep a bit over it :(

Comment 23

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8902193 [details]
Bug 1391198 - Update expectation for passed test cases

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

Comment 24

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8902194 [details]
Bug 1391198 - Make the order of rules in DevTools be the specificity order.

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

Comment 25

[Chih-Hsuan Kuo [:tommykuo]]

I filed a bug (bug 1395479) for the remaining test cases of DevTools, so I also update the bug number when updating the expectation.

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8902194 [details]
Bug 1391198 - Make the order of rules in DevTools be the specificity order.

https://reviewboard.mozilla.org/r/173678/#review180006

::: servo/components/style/rule_tree/mod.rs:204
(Diff revision 3)
>              };
> +
> +            // For the rules with any important declaration, we insert them into
> +            // rule tree later again with important levels. It could make them
> +            // can be calculated with higher priority when traversing the rule
> +            // tree.

I don't think this comment is particularly useful.

::: servo/components/style/rule_tree/mod.rs:221
(Diff revision 3)
>                  };
>              }
> -            // We really want to ensure empty rule nodes appear in the rule tree for
> -            // devtools, this condition ensures that if we find an empty rule node, we
> -            // insert it at the normal level.
> -            if any_normal || !any_important {
> +
> +            // No matter any important declaration found, we still insert the
> +            // rules into rule tree as normal rules. It could make devtools get
> +            // the rules with correct specificity order.

Instead of this, let's make this comment read:

```
            // We don't optimize out empty rules, even though we could.
            //
            // Inspector relies on every rule being inserted in the normal level
            // at least once, in order to return the rules with the correct
            // specificity order.
            //
            // TODO(emilio): If we want to apply these optimizations without
            // breaking inspector's expectations, we'd need to run
            // selector-matching again at the inspector's request. That may or
            // may not be a better trade-off.
```

Comment 27

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8902193 [details]
Bug 1391198 - Update expectation for passed test cases

https://reviewboard.mozilla.org/r/173676/#review180022

I made very similar looking changes in bug 1387445.  Please rebase and repost, so I can see what's really changing in this bug.

Comment 28

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8902193 [details]
Bug 1391198 - Update expectation for passed test cases

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173676/diff/3-4/

Comment 29

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8902194 [details]
Bug 1391198 - Make the order of rules in DevTools be the specificity order.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173678/diff/3-4/

Comment 30

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8902193 [details]
Bug 1391198 - Update expectation for passed test cases

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173676/diff/4-5/

Comment 31

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8902194 [details]
Bug 1391198 - Make the order of rules in DevTools be the specificity order.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173678/diff/4-5/

Comment 32

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8902193 [details]
Bug 1391198 - Update expectation for passed test cases

https://reviewboard.mozilla.org/r/173676/#review180292

Thanks for working on this! :)

::: devtools/client/inspector/rules/test/browser.ini:183
(Diff revision 5)
>  [browser_rules_inline-source-map.js]
>  [browser_rules_invalid.js]
>  [browser_rules_invalid-source-map.js]
>  [browser_rules_keybindings.js]
>  [browser_rules_keyframes-rule_01.js]
> -skip-if = stylo # Bug 1394994
> +skip-if = stylo # Bug 1395479

Let's leave these 3 alone, since bug 1395479 was duped to bug 1394994.

::: devtools/client/inspector/rules/test/browser.ini:222
(Diff revision 5)
>  [browser_rules_search-filter-computed-list_04.js]
>  [browser_rules_search-filter-computed-list_expander.js]
>  [browser_rules_search-filter-overridden-property.js]
>  [browser_rules_search-filter_01.js]
>  [browser_rules_search-filter_02.js]
> -skip-if = stylo # Bug 1394994
> +skip-if = stylo # Bug 1395479

This one can also be left as is.

Comment 33

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8902193 [details]
Bug 1391198 - Update expectation for passed test cases

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173676/diff/5-6/

Comment 34

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8902193 [details]
Bug 1391198 - Update expectation for passed test cases

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173676/diff/6-7/

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c2abee347187
Update expectation for passed test cases r=jryans

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c2abee347187