1946439 - Disabling declaration in nested rule throws

Status: ASSIGNED

(DevTools :: Inspector: Rules, defect, P2)

Description

[Nicolas Chevobbe [:nchevobbe]]

Steps to reproduce

1. Navigate to data:text/html,<meta charset=utf8><style>body { color: red; &:where(*) { color: blue; } color: green; }</style>Hello
2. Open the inspector
3. In the Rules view, disable the color: green declaration

Expected results

The text turns blue

Actual results

The text turns black

---

In the style editor, we can see that the stylesheet was cleared out:

body {
  /*! color: green; */
}

we're not seeing the nested declarations anymore

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Exceptions in the Browser Toolbox:

TypeError: InspectorUtils.getRelativeRuleLine: Argument 1 is not an object.
    onStyleApplied resource://devtools/server/actors/style-rule.js:758
    _onStylesheetUpdated resource://devtools/server/actors/page-style.js:1116
    _emit resource://devtools/shared/event-emitter.js:242
    emit resource://devtools/shared/event-emitter.js:186
    emit resource://devtools/shared/event-emitter.js:330
    #onStyleSheetUpdated resource://devtools/server/actors/utils/stylesheets-manager.js:982
    setStyleSheetText resource://devtools/server/actors/utils/stylesheets-manager.js:461
    setRuleText resource://devtools/server/actors/style-rule.js:944

Error while calling actor 'domstylerule's method 'setRuleText' can't access property "parentRule", rule is null Actor.js:103:13

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

The root issue here is that InspectorUtils.getRelativeRuleLine and InspectorUtils.getRelativeRuleColumn are returning the line and column of the parent rule.
This can be highlighted in the test we have for those functions:

diff --git a/layout/inspector/tests/test_getRelativeRuleLine.html b/layout/inspector/tests/test_getRelativeRuleLine.html
--- a/layout/inspector/tests/test_getRelativeRuleLine.html
+++ b/layout/inspector/tests/test_getRelativeRuleLine.html
@@ -26,6 +26,17 @@
      }
   </style>
   <style>#test { color: red; }</style><!-- This tests stylesheet caching -->
+  <style>
+    #test {
+      color: tomato;
+
+      &:where(*) {
+        color: gold;
+      }
+
+      color: blanchedalmond;
+    }
+  </style><!-- This tests nesting -->
   <script>
   const InspectorUtils = SpecialPowers.InspectorUtils;
 
@@ -36,12 +47,22 @@
     { sheetNo: 2, ruleNo: 0, lineNo: 1, columnNo: 1 },
     { sheetNo: 3, ruleNo: 0, lineNo: 5, columnNo: 6 },
     { sheetNo: 4, ruleNo: 0, lineNo: 1, columnNo: 1 },
+    { sheetNo: 5, ruleNo: 0, lineNo: 2, columnNo: 5 },
+    { sheetNo: 5, ruleNo: [0, 0], lineNo: 5, columnNo: 7 },
+    { sheetNo: 5, ruleNo: [0, 1], lineNo: 9, columnNo: 5 },
   ];
 
   function doTest() {
     for (let test of tests) {
       let sheet = document.styleSheets[test.sheetNo];
-      let rule = sheet.cssRules[test.ruleNo];
+      let rule;
+      if (Array.isArray(test.ruleNo)) {
+        for(const index of test.ruleNo) {
+          rule = (rule ? rule : sheet).cssRules[index];
+        }
+      } else {
+        rule = sheet.cssRules[test.ruleNo];
+      }
       let line = InspectorUtils.getRelativeRuleLine(rule);
       let column = InspectorUtils.getRuleColumn(rule);
       info("testing sheet " + test.sheetNo + ", rule " + test.ruleNo);

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Discussed this with Emilio on Element.

When parsing the nested rule, we're using the source location of the parent rule:

https://searchfox.org/mozilla-release/rev/8647b34112bf8f1ea2d630feffa0e863e82f79ca/servo/components/style/stylesheets/rule_parser.rs#654-657

let nested_rule = CssRule::NestedDeclarations(Arc::new(parser.shared_lock.wrap(NestedDeclarationsRule {
    block: Arc::new(parser.shared_lock.wrap(declarations)),
    source_location,
})));

we'd probably want to parse the start location around here so it points at the first declaration, maybe?

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

I tried to look into this but I'm a bit lost

https://searchfox.org/mozilla-release/rev/8647b34112bf8f1ea2d630feffa0e863e82f79ca/servo/components/style/stylesheets/rule_parser.rs#1022-1023

top.declaration_parser_state
    .parse_value(&top.context, name, input)

is being called for all the declarations of the rule, not only the nested ones. I'm not seeing a direct relationship with https://searchfox.org/mozilla-release/rev/63677cc33948d70cb9e676f88cdb775495cbdbdb/servo/components/style/stylesheets/rule_parser.rs#654-657

let nested_rule = CssRule::NestedDeclarations(Arc::new(parser.shared_lock.wrap(NestedDeclarationsRule {
    block: Arc::new(parser.shared_lock.wrap(declarations)),
    source_location,
})));

Emilio, could you elaborate a bit more what you had in mind?

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Sure. So, I think that parse_value needs to get a declaration_start argument, which can just be start here.

Then in parse_value you need to do something like adding a first_declaration_start: Option<SourceLocation> here, and around here do something like if self.first_declaration_start.is_none() { self.first_declaration_start = start.source_location(); }.

Then in flush_declarations, where creating a nested rule, you need to use source_location: parser.first_declaration_start.expect("We've parsed at least one declaration") rather than the given source_location, which you can remove.

Does that help?

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)

Sure. So, I think that parse_value needs to get a declaration_start argument, which can just be start here.

Then in parse_value you need to do something like adding a first_declaration_start: Option<SourceLocation> here, and around here do something like if self.first_declaration_start.is_none() { self.first_declaration_start = start.source_location(); }.

Then in flush_declarations, where creating a nested rule, you need to use source_location: parser.first_declaration_start.expect("We've parsed at least one declaration") rather than the given source_location, which you can remove.

Does that help?

Yes, thanks, that's super helpful.
I have a patch which compiles without Rust yelling at me and I can run so it's a good start.
I think I'm only missing resetting first_declaration_start when a nested declaration is seen, otherwise it's always the value of the first declaration of the rule

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Note that once we get the correct location, we're hitting another exception: Error while calling actor 'domstylerule's method 'setRuleText' Error in InspectorUtils.replaceBlockRuleBodyTextInStylesheet https://searchfox.org/mozilla-release/rev/201ece97e589a880d0b3f000affc9bf459e90cee/servo/ports/geckolib/glue.rs#9448-9463

// Search forward for the opening brace.
while let Ok(token) = input.next() {
    if matches!(*token, Token::CurlyBracketBlock) {
        found_start = true;
        break;
    }

    if token.is_parse_error() {
        break;
    }
}

if !found_start {
    ret_val.set_is_void(true);
    return;
}

So here we're looking for the opening curly bracket (i.e. after the selector)
But in such case, we don't have a selector, we're directly at the expected location

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1946439 - [devtools] Fix source_location of CssRule::NestedDeclarations. r=emilio.

Nested declarations were getting the location of their parent rule, which would
cause issue in the DevTools Inspector (e.g. for InspectorUtils.getRelativeRuleLine).
We now track the first declaration of the nested declaration to set it as the location
of the rule.

Comment 9

[Phabricator Automation]

Comment on attachment 9467339 [details]
Bug 1946439 - [devtools] Fix source_location of CssRule::NestedDeclarations. r=emilio.

Revision D238911 was moved to bug 1950551. Setting attachment 9467339 [details] to obsolete.