1946445 - Declaration after nested rule appears in its parent rule

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, defect)

Description

[Nicolas Chevobbe [:nchevobbe]]

Steps to reproduce

1. Navigate to data:text/html,<meta charset=utf8><style>body { color: red; span {} color: green; }</style>Hello
2. Open the inspector

In the rules view, we're getting:

body {
  {
    color: green;
  }
}

body {
  color: red;
  color: green;
}

but we should have

body {
  {
    color: green;
  }
}

body {
  color: red;
}

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

parseDeclarationsInternal handling of nested rules predate changes made to the spec and additions of CSSNesstedDeclarations:

https://searchfox.org/mozilla-release/rev/21df1ac521242be96e0c070aa713ccff03e1b2d3/devtools/shared/css/parsing-utils.js#364-392

if (
  // If we're not already in a nested rule
  !isInNested &&
  // and there's an opening curly bracket
  token.tokenType === "CurlyBracketBlock" &&
  // and we're not inside a function or an attribute
  !currentBlocks.length
) {
  // Assume we're encountering a nested rule.
  isInNested = true;
  nestingLevel = 1;

  continue;
} else if (isInNested) {
  if (token.tokenType == "CurlyBracketBlock") {
    nestingLevel++;
  } else if (token.tokenType == "CloseCurlyBracket") {
    nestingLevel--;
  }

  // If we were in a nested rule, and we saw the last closing curly bracket,
  // reset the state to parse possible declarations declared after the nested rule.
  if (nestingLevel === 0) {
    isInNested = false;
    // We need to remove the previous pending declaration and reset the state
    declarations.pop();
    resetStateForNextDeclaration();
  }
  continue;

which means we still retrieve the declarations after nested block, although we shouldn't now, since they're now in a different rule that we do retrieve as well

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1919853

:emilio, since you are the author of the regressor, bug 1919853, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

You're definitely more familiar than me with this code, do you plan to look at this? Otherwise I could but I'm honestly a bit swamped with other things so...

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

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

You're definitely more familiar than me with this code, do you plan to look at this? Otherwise I could but I'm honestly a bit swamped with other things so...

yeah yeah, don't worry, I'm having a look

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1946445 - [devtools] Turn RuleRewriter into a proper ES6 class. r=#devtools.

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1946445 - [devtools] Fix parseDeclarationsInternal for nested declarations. r=#devtools.

Stop collecting declarations as soon as we detect a nested rule.
Any declaration that come after the nested rules are now put in a
separate CSSNestedDeclaration rule.
We still need to handle the nesting logic for commented declarations,
as highlighted by a test case in test_parseDeclarations.js.
Other test cases there for nested rules are updated, and we add
a CSSNestedDeclaration in a Rules view mochitest to make sure we
don't get the declaration in the "top level" rule anymore.

Comment 7

[Phabricator Automation]

Comment on attachment 9465079 [details]
Bug 1946445 - [devtools] Turn RuleRewriter into a proper ES6 class. r=#devtools.

Revision D237519 was moved to bug 1946442. Setting attachment 9465079 [details] to obsolete.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a08727399a37
[devtools] Fix parseDeclarationsInternal for nested declarations. r=devtools-reviewers,jdescottes.

Comment 9

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/a08727399a37

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:nchevobbe, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox136 to wontfix.

For more information, please visit BugBot documentation.

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1946445 - [devtools] Fix parseDeclarationsInternal for nested declarations. r=#devtools.

Stop collecting declarations as soon as we detect a nested rule.
Any declaration that come after the nested rules are now put in a
separate CSSNestedDeclaration rule.
We still need to handle the nesting logic for commented declarations,
as highlighted by a test case in test_parseDeclarations.js.
Other test cases there for nested rules are updated, and we add
a CSSNestedDeclaration in a Rules view mochitest to make sure we
don't get the declaration in the "top level" rule anymore.

Original Revision: https://phabricator.services.mozilla.com/D237520

Comment 12

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: duplicated declaration in Inspector Rules view, might confuse users
• Code covered by automated testing: yes
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: -
• Risk associated with taking this patch: low
• Explanation of risk level: devtools only fix, covered by unit test and mochitest
• String changes made/needed: -
• Is Android affected?: yes

Comment 13

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/ff08fc6dae65

Comment 14

[Florin Bilt]

For more information on performance sheriffing please see our FAQ.
(In reply to Cristian Tuns from comment #9)

https://hg.mozilla.org/mozilla-central/rev/a08727399a37

Perfherder has detected a devtools performance change from push a08727399a3753b89c1270329ebd0f0499c3740d.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
8%	damp browser-toolbox.webconsole-ready.DAMP	linux1804-64-shippable-qr	e10s fission stylo webrender	698.31 -> 644.43
6%	damp browser-toolbox.webconsole-ready.DAMP	windows11-64-shippable-qr	e10s fission stylo webrender	427.31 -> 400.16

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 43855