Closed Bug 1391198 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: kuoe0.tw, Assigned: kuoe0.tw)

References

Details

Attachments

(3 files, 1 obsolete file)

stylo.png
617.03 KB, image/png
Details
gecko.png
613.58 KB, image/png
Details
Bug 1391198 - Update expectation for passed test cases
59 bytes, text/x-review-board-request
jryans
: review+
Details 
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
Attached image stylo.png — 

    
    

    
  

      
      
      
      

        Attached image
          
        gecko.png
        — 
      

    


  

    
  
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.

    
    

    
  
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"

    
    

    
  
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.

    
    

    
  
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

    
    

    
  
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

    
  
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.

    
    

    
  
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)

    
    

    
  
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.

    
    

    
      
  


  

    
    

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

    
    

    
  
(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

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8902193 -
        Flags: review?(jryans)

        Attachment #8902194 -
        Flags: review?(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 :(

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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 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 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 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+

    
    

    
      
  

    
  

        Attachment #8902194 -
        Attachment is obsolete: true

    
    

    
      
  

    
    

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

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/c2abee347187
Status: NEW → RESOLVED
Closed: 7 years 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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: