1384802 - Stylo: Empty rules should still be returned by inIDOMUtils.getCSSStyleRules

Status: RESOLVED FIXED

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

Description

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

DevTools expects an empty rule with no properties to still be returned by `inIDOMUtils.getCSSStyleRules`, but Stylo omits them.

This breaks a variety of tests under:

devtools/client/inspector/rules/test

Example log:

https://treeherder.mozilla.org/logviewer.html#?job_id=117569486&repo=try&lineNumber=4200

For example, one test:

devtools/client/inspector/rules/test/browser_rules_edit-property-order.js

loads the following page:

<style>#testid {}</style><div id='testid'>Styled Node</div>

as a dataURI.  It expects to get the empty rule node.

Comment 1

[Chih-Hsuan Kuo [:tommykuo]]

Hi Ryan, I want to look at this.

Comment 2

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

This appears to be the cause of most remaining DevTools failures (~40).

Comment 3

[Chih-Hsuan Kuo [:tommykuo]]

> For example, one test:
> 
> devtools/client/inspector/rules/test/browser_rules_edit-property-order.js
> 
> loads the following page:
> 
> <style>#testid {}</style><div id='testid'>Styled Node</div>
> 
> as a dataURI.  It expects to get the empty rule node.

Hi Ryan, I tried to open "data:text/html,data:text/html,<style>#testid {}</style><div id='testid'>Styled Node</div>" in Firefox (both in stylo and non-stylo), and it's an empty page. Do you have the same result?

Comment 4

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

(In reply to Tommy Kuo [:kuoe0] at UTC+8 from comment #3)
> > For example, one test:
> > 
> > devtools/client/inspector/rules/test/browser_rules_edit-property-order.js
> > 
> > loads the following page:
> > 
> > <style>#testid {}</style><div id='testid'>Styled Node</div>
> > 
> > as a dataURI.  It expects to get the empty rule node.
> 
> Hi Ryan, I tried to open "data:text/html,data:text/html,<style>#testid
> {}</style><div id='testid'>Styled Node</div>" in Firefox (both in stylo and
> non-stylo), and it's an empty page. Do you have the same result?

Ah, you have to URL encode it first, I think because of the `#` character.

So, something like this in the console:

`data:text/html,${encodeURIComponent("<style>#testid {}</style><div id='testid'>Styled Node</div>")}`

which evaluates to:

data:text/html,%3Cstyle%3E%23testid%20%7B%7D%3C%2Fstyle%3E%3Cdiv%20id%3D'testid'%3EStyled%20Node%3C%2Fdiv%3E

STR:

1. Navigate to the data URI above.
2. Open inspector
3. Click on the #testid div

In Gecko mode, an empty #testid {} rule appears on the right in rules view.
In Stylo mode, the #testid {} rule is not shown.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

I know why this happens, which is because we skip property declaration blocks without any declaration in the rule tree... I think it'd be unfortunate to need avoid doing it though...

Comment 6

[Emilio Cobos Álvarez (:emilio)]

I was about to make this worse in https://github.com/servo/servo/pull/17949, fwiw... I ended up not doing that, but I still think we should be able to do this kind of stuff and that devtools tests shouldn't rely on this.

Comment 7

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

(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> I was about to make this worse in https://github.com/servo/servo/pull/17949,
> fwiw... I ended up not doing that, but I still think we should be able to do
> this kind of stuff and that devtools tests shouldn't rely on this.

For comparison, Chrome's DevTools also show empty rules.  Generally, DevTools finds people prefer the tools to report the state of the world as accurately as possible.  So, if there is a rule with no properties, it should still appear in the tools.

Comment 8

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

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> > I was about to make this worse in https://github.com/servo/servo/pull/17949,
> > fwiw... I ended up not doing that, but I still think we should be able to do
> > this kind of stuff and that devtools tests shouldn't rely on this.
> 
> For comparison, Chrome's DevTools also show empty rules.  Generally,
> DevTools finds people prefer the tools to report the state of the world as
> accurately as possible.  So, if there is a rule with no properties, it
> should still appear in the tools.

:emilio, given this info, do you think it's okay for us to keep empty decl blocks around, or are you strongly opposed to it?

Comment 9

[Emilio Cobos Álvarez (:emilio)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> > (In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> > > I was about to make this worse in https://github.com/servo/servo/pull/17949,
> > > fwiw... I ended up not doing that, but I still think we should be able to do
> > > this kind of stuff and that devtools tests shouldn't rely on this.
> > 
> > For comparison, Chrome's DevTools also show empty rules.  Generally,
> > DevTools finds people prefer the tools to report the state of the world as
> > accurately as possible.  So, if there is a rule with no properties, it
> > should still appear in the tools.
> 
> :emilio, given this info, do you think it's okay for us to keep empty decl
> blocks around, or are you strongly opposed to it?

I think they should be rare enough that it's not worth caring much about it.

I think it's still somewhat unfortunate, but I guess I'm not too strongly opposed to it.

Comment 10

[Chih-Hsuan Kuo [:tommykuo]]

So, do we still want to fix this?

Comment 11

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

(In reply to Tommy Kuo [:kuoe0] at UTC+8 from comment #10)
> So, do we still want to fix this?

Yes, I think it's important to fix this for DevTools so it represents the state of the page accurately.

Comment 12

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: Bug 1384802 - Update the expectation of test cases.

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

Comment 13

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8896856 [details]
Bug 1384802 - Update the expectation of test cases.

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

Comment 14

[Chih-Hsuan Kuo [:tommykuo]]

I think we can insert all rules into the rule tree without checking by `any_normal()` or `any_important()`, so I removed the if-statement.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8896856 [details]
Bug 1384802 - Update the expectation of test cases.

https://reviewboard.mozilla.org/r/168150/#review173326

::: servo/components/style/rule_tree/mod.rs
(Diff revision 2)
>                          important_style_attr = Some(source.clone());
>                      },
>                      _ => {},
>                  };
>              }
> -            if any_normal {

> I think we can insert all rules into the rule tree without checking by
> `any_normal()` or `any_important()`, so I removed the if-statement.

Sure, that's doable, but that inserts two rule nodes instead of one when a declaration block only has !important rules, so I'd rather keep the if, wdyt?

Comment 16

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8896856 [details]
Bug 1384802 - Update the expectation of test cases.

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

Comment 17

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8896856 [details]
Bug 1384802 - Update the expectation of test cases.

https://reviewboard.mozilla.org/r/168150/#review173326

> > I think we can insert all rules into the rule tree without checking by
> > `any_normal()` or `any_important()`, so I removed the if-statement.
> 
> Sure, that's doable, but that inserts two rule nodes instead of one when a declaration block only has !important rules, so I'd rather keep the if, wdyt?

You're right. I didn't notice the important rules already handled above. I added a comment for it to make it clear.

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8896856 [details]
Bug 1384802 - Update the expectation of test cases.

https://reviewboard.mozilla.org/r/168150/#review173338

r=me, thanks!

::: servo/components/style/rule_tree/mod.rs:213
(Diff revision 3)
>                          important_style_attr = Some(source.clone());
>                      },
>                      _ => {},
>                  };
>              }
> -            if any_normal {
> +            // The condition `!any_important` means this rule-set is empty.

What about a comment like:

```
// 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.
```

I could see myself removing this condition accidentally in the future without thinking about it.

Comment 19

[Chih-Hsuan Kuo [:tommykuo]]

Hi jryan, which test cases would be fixed by this? I ran a try but I didn't any unexpected PASS.

Comment 20

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

(In reply to Tommy Kuo [:kuoe0] at UTC+8 from comment #19)
> Hi jryan, which test cases would be fixed by this? I ran a try but I didn't
> any unexpected PASS.

Various DevTools tests are currently skipped due to this bug, so they currently don't run at all:

http://searchfox.org/mozilla-central/search?q=1384802&case=true&path=

So, I'd suggest removing all the skip-if lines from that search and seeing what you get.  Stylo try runs should include DevTools tests on Linux64, and you can target them specifically with something like:

./mach try -b do -p linux64 -u mochitest-dt[linux64-stylo]

To run the tests locally, you can use something like:

./mach mochitest devtools/client/inspector/rules
./mach mochitest devtools/client/styleeditor

You'll want to be sure you are actually running in Stylo mode locally, so you may need to use `STYLO_FORCE_ENABLED=1 ./mach ...` if your local build defaults to Stylo off.

If your change fixes some of them but not all, it could be that there are multiple problems for these tests.  If that happens, attach a pointer to your try run or log, and I'll try to work out what's going on.

Comment 21

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8896856 [details]
Bug 1384802 - Update the expectation of test cases.

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

Comment 22

[Chih-Hsuan Kuo [:tommykuo]]

There are still some failures on try[1]. I'll keep skipping them.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=40b72f7e5385f3f974ba7d98e4976947789a1684

Comment 23

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

Comment on attachment 8896856 [details]
Bug 1384802 - Update the expectation of test cases.

https://reviewboard.mozilla.org/r/168150/#review174052

Thanks for working on this! :)

::: devtools/client/inspector/rules/test/browser.ini:131
(Diff revision 4)
>  [browser_rules_edit-property_02.js]
>  [browser_rules_edit-property_03.js]
>  [browser_rules_edit-property_04.js]
>  [browser_rules_edit-property_05.js]
>  [browser_rules_edit-property_06.js]
>  skip-if = stylo # Bug 1384802 - Empty rules aren't returned

Please update the remaining skip lines to mention bug 1387445 instead (and maybe delete the "Empty rules..." part since we aren't sure about the cause).  I'll search for them after your work lands and take a closer look at why they still fail.

Comment 24

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8896856 [details]
Bug 1384802 - Update the expectation of test cases.

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

Comment 25

[Chih-Hsuan Kuo [:tommykuo]]

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

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8f93b3a446b6
Update the expectation of test cases. r=emilio,jryans

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8f93b3a446b6