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

RESOLVED FIXED in Firefox 57

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: Tommy Kuo (away forever...), Assigned: Tommy Kuo (away forever...))

Tracking

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

10 months ago
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
(Assignee)

Comment 1

10 months ago
Created attachment 8898207 [details]
stylo.png
(Assignee)

Comment 2

10 months ago
Created attachment 8898208 [details]
gecko.png
(Assignee)

Updated

10 months ago
Summary: stylo: The order of rules in DevTools on stylo are different to gecko. → stylo: The order of rules in DevTools is not specificity order when any !important exists.
Blocks: 1357715
(Assignee)

Comment 3

10 months ago
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.
Summary: stylo: The order of rules in DevTools is not specificity order when any !important exists. → stylo: UNEXPECTED-TEST-FAIL for "devtools/client/inspector/rules/test/browser_rules_custom.js"
(Assignee)

Comment 4

10 months ago
Hmmm... The issue still exists.
Summary: stylo: UNEXPECTED-TEST-FAIL for "devtools/client/inspector/rules/test/browser_rules_custom.js" → stylo: The order of rules in DevTools is not specificity order when any !important exists.
(Assignee)

Comment 5

10 months ago
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
(Assignee)

Comment 7

10 months ago
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
(Assignee)

Updated

10 months ago
Summary: stylo: The order of rules in DevTools is not specificity order when any !important exists. → stylo: The order of rules in DevTools is not specificity order when all declarations in a rule are !important.
(Assignee)

Comment 8

10 months ago
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
Flags: needinfo?(emilio)
It is not, as far as I can tell. Rule nodes are per rule, not per selector.
Flags: needinfo?(emilio)
(Assignee)

Comment 10

10 months ago
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?
Flags: needinfo?(jryans)
(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
Flags: needinfo?(jryans)
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 16

10 months ago
(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

10 months ago
mozreview-review
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.

Updated

10 months ago
Priority: -- → P2
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.
(Assignee)

Comment 19

10 months ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8902193 - Flags: review?(jryans)
Attachment #8902194 - Flags: review?(emilio)

Comment 22

10 months ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 25

10 months ago
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

10 months ago
mozreview-review
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.
```
Attachment #8902194 - Flags: review?(emilio) → review+

Comment 27

10 months ago
mozreview-review
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.
Attachment #8902193 - Flags: review?(jryans) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 32

10 months ago
mozreview-review
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.
Attachment #8902193 - Flags: review?(jryans) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8902194 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Comment 35

10 months ago
Pushed by tokuo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c2abee347187
Update expectation for passed test cases r=jryans

Comment 36

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c2abee347187
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.