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.

Hi Ryan, I want to look at this.

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

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

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

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

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.

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

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

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

So, do we still want to fix this?

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

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

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

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

There are still some failures on try[1]. I'll keep skipping them. [1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=40b72f7e5385f3f974ba7d98e4976947789a1684

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.

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

Pushed by tokuo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8f93b3a446b6 Update the expectation of test cases. r=emilio,jryans