1838011 - Nested rules are showing up in parent rule

Status: VERIFIED FIXED

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

Description

[Nicolas Chevobbe [:nchevobbe][PTO - back October 26]]

Attachment: nested rules displayed as (invalid) properties

Steps to reproduce

1. On Nightly (so nesting is enabled), navigate to https://ffx-devtools-css-nesting.glitch.me/
2. Open the inspector
3. Make sure the body node is selected

Expected results

The rule view does not show nested rules

Actual results

Nested rules are displayed in the rule, but treated as property, making it confusing

---

We get the nested rules as declarations in https://searchfox.org/mozilla-central/rev/b91e9bef5a6d6f7b549fbc9cba70cb4665ed3866/devtools/server/actors/style-rule.js#393-396 , and so we should fix parseDeclarationInternals in parsing-utils.js#266-294 so it does not include the nested rules (or at least handle them properly)

Comment 1

[Nicolas Chevobbe [:nchevobbe][PTO - back October 26]]

Attachment: Bug 1838011 - [devtools] Handle CSS Nesting in parseDeclarations. r=#devtools-reviewers.

This adds different nested state so we can simply ignore nested CSS rules.
I added a lot of different test cases in the existing parseDeclarations
unit test so we can have good coverage.
I also took this as an opportunity to properly check that the produced offsets
does map to the expected original text.

Note that there will probably be a follow-up patch/bug to add mochitests for the
rule view, once Bug 1838012 lands (as I'll extend the test I add in D181041).

Comment 2

[Nicolas Chevobbe [:nchevobbe][PTO - back October 26]]

Attachment: Bug 1838011 - [devtools] Assert parseDeclaration offsets easier. r=#devtools-reviewers.

Seeing those offsets integer in the test case is a bit hard to assert that they
do actually refer to the expected part of the input.
This adds a declarationText property on the expected object that can be used
to properly asserts produced offsets.

Comment 3

[Phabricator Automation]

Comment on attachment 9339302 [details]
Bug 1838011 - [devtools] Assert parseDeclaration offsets easier. r=#devtools-reviewers.

Revision D181094 was moved to bug 1839178. Setting attachment 9339302 [details] to obsolete.

Comment 4

[Nicolas Chevobbe [:nchevobbe][PTO - back October 26]]

There was discussion on Phabricator on how we could use what's already done at platform level to be able to parse nested rules.

From https://phabricator.services.mozilla.com/D180701#5982342:

we do want to get invalid and commented declarations to be able to show them in the inspector
That's a pretty important behavior for users to know they mistyped something (or are using some unsupported property)

The parseNamedDeclaration function we use only returns declarations based on the syntax, there's not actual check on the validity of the name or value
We then go through each declaration to tag them as using invalid name/value:
https://searchfox.org/mozilla-central/rev/986024d59bff59819a3ed2f7c1d0f5254cdc3f3d/devtools/server/actors/style-rule.js#437-443,446-450

// InspectorUtils.supports only supports the 1-arg version, but that's
// what we want to do anyways so that we also accept !important in the
// value.
decl.isValid = InspectorUtils.supports(
  `${decl.name}:${decl.value}`,
  supportsOptions
);
...
// Check property name. All valid CSS properties support "initial" as a value.
decl.isNameValid = InspectorUtils.supports(
  `${decl.name}:initial`,
  supportsOptions
);

I think https://searchfox.org/mozilla-central/source/devtools/client/shared/test/xpcshell/test_parseDeclarations.js does highlight what we need, which is:
an array of declarations object, with the following properties:

• {String} name: The name of the property
• {String} value: The value for the property
• {String} priority: "important" or empty string (this one could be turned into an boolean important property I guess? Not sure why we have it as a string)
• {String} terminator: the string to use to terminate the declaration
• {Array<int>} colonOffsets: An array of 2 integers, holding the start and end locations of the ":" that separates the property name from the value.
• {Array<int>} offset: An array of 2 integers, which are the index in the input string at which the declaration starts and ends (so calling substring on the input will give us the authored property)
• {Array<int>} commentOffsets: If the declaration appears in a comment, an array of 2 integers which will hold the offsets of the start and end of the enclosing comment input will give us the authored property)

we need the offsets so we can update the rule without minimal changes to the authored text of the stylesheets (e.g. not removing the comments, preserving indentation, …), since it can be visualize and saved from the style editor.

As I say earlier, since we do need them, we might add

• {bool} isNameValid: true if the property is valid
• {bool} isValid: Is the whole declaration valid (could also be isValueValid really)

One thing that we didn't decide on yet is if we want to also get the "nested rules declarations", so we could maybe display them as well in the rule view for easier authoring (see Bug 1838179)
Does this whole makes sense? Will it be too much work on your end? If so maybe there could try a more ad-hoc solution, with a function that would return the offsets or nested rules in a given input?

Comment 5

[Nicolas Chevobbe [:nchevobbe][PTO - back October 26]]

I missed Emilio's comment in https://phabricator.services.mozilla.com/D180701#5989361
I'll file a bug for what I was describing in Comment 4, but this won't be a good short term solution as it would require a bit of work and nesting is planned to ship in 116

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac83781a2320
[devtools] Handle CSS Nesting in parseDeclarations. r=devtools-reviewers,ochameau.

Comment 7

[Cosmin Sabou [:CosminS]]

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

Comment 8

[Dana Lucaci, Desktop QA]

Reproduced the issue with Firefox 116.0a1 (2023-06-12) on MacOS 12.
The issue is verified fixed with Firefox 117.0a1 (20230725211415) and Firefox 116.0 (20230724170120) on MacOS 12, Win 10 and Ubuntu 22.