Bug 201571 – Web Inspector: Styles: for rules that don't match selected node, property values always show as invalid

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

Created attachment 378260 [details]
Reduction

Steps To Reproduce:
1. Inspect attached reduction.html
2. In the style sidebar, hit the + to add a new selector
3. Type “iframe” as the selector
4. Add “outline: 2px solid orange;”

Results:
Rule is marked as invalid, but applies.

<rdar://problem/54530335>

[Nikita Vasilyev]

Created attachment 378262 [details]
Patch

This fixes the bug but it also updates rules that do match selected node twice. I'm still thinking of a better way of fixing this.

[Nikita Vasilyev]

Created attachment 378805 [details]
Patch

[Joseph Pecoraro]

inspector/css/modify-inline-style.html seems to be failing after this change.

[Nikita Vasilyev]

Comment on attachment 378805 [details]
Patch

This broken tests on wk1 and there only. Looking into it.

[Nikita Vasilyev]

(In reply to Nikita Vasilyev from comment #4)
> Comment on attachment 378805 [details]
> Patch
> 
> This broken tests on wk1 and there only. Looking into it.

Updating twice (after CSSAgent.setStyleText and on CSSAgent.getMatchedStylesForNode) caused a race condition on WK1. In my next patch, updates only happen once.

If it matches the selected DOM node, update on CSSAgent.getMatchedStylesForNode.
If it doesn't, update on CSSAgent.setStyleText.

[Nikita Vasilyev]

Created attachment 379798 [details]
Patch

[Matt Baker]

I don't understand what is meant by "rule is marked invalid, but applies".

1. Inspect test case
2. Open Elements tab (<foo> is selected by default)
2. Press "+" to add new selector
3. Adds selector:

      foo {
      }

4. Type "frame", replacing the selection "foo"
5. Press tab. Type "outline". Press tab. Type "2px solid orange". Press tab.

Result:
Rule marked invalid, but style doesn't apply (there are no iframe elements in document)

Repeating the above without changing "foo" to "iframe" works as expected. Am I missing something?

[Nikita Vasilyev]

(In reply to Matt Baker from comment #7)
> I don't understand what is meant by "rule is marked invalid, but applies".

Property value (e.g. "2px solid orange") is marked invalid (has a red strikethrough and a warning), but applies.

See rdar://problem/54530335 if it's still unclear.

(In reply to Matt Baker from comment #7)
> Repeating the above without changing "foo" to "iframe" works as expected. Am
> I missing something?

Yes, repeating the above without changing "foo" to "iframe" works as expected, even before the patch.

The bug is specifically about the rules that don't match the selected node.

[Matt Baker]

(In reply to Nikita Vasilyev from comment #8)
> (In reply to Matt Baker from comment #7)
> > I don't understand what is meant by "rule is marked invalid, but applies".
> 
> Property value (e.g. "2px solid orange") is marked invalid (has a red
> strikethrough and a warning), but applies.
> 
> See rdar://problem/54530335 if it's still unclear.
> 
> (In reply to Matt Baker from comment #7)
> > Repeating the above without changing "foo" to "iframe" works as expected. Am
> > I missing something?
> 
> Yes, repeating the above without changing "foo" to "iframe" works as
> expected, even before the patch.
> 
> The bug is specifically about the rules that don't match the selected node.

Okay I get it. I think it would have been helpful to have an <iframe> in the test case, and mention this.

[Matt Baker]

I saw some strange results with the patch applied.

Test page:
<div>I'm a div</div>
<p>And I'm a paragraph</p>

Steps to Reproduce:
1. Inspect test page
2. Elements > select the <div>
3. Press "+" to add new selector
4. Type "p", replacing "div"
5. Hit tab. Type "outline". Hit tab. Type "20px solid orange". Hit tab.
=> Selector not applied. Property value shown as invalid.
6. Select the <p> node in the DOM tree
=> Style sidebar shows selector contents: 
p {
   outline: outline: 20;
   outline: 20p;
   utline: 20;
   outline: 20px;
   tline: 20;
   outline: 20p;
   utline: 20;
   outline: 20px saddlebrown;
   line: 20;
   outline: 20p;
   ...
}

Looks like a parsing step might be getting stuck in a loop.

[Nikita Vasilyev]

The console is full of "_styleSheetTextRange data is invalid" errors:

[Error] Assertion Failed: Cannot have negative numbers in TextRange 0:-2...0:9
	cloneAndModify (TextRange.js:134)
	_updateOwnerStyleText (CSSProperty.js:463)
	_updateStyleText (CSSProperty.js:438)
	rawValue (CSSProperty.js:257)
	_handleValueChange (SpreadsheetStyleProperty.js:736)
	spreadsheetTextFieldDidChange (SpreadsheetStyleProperty.js:348)
	_handleInput (SpreadsheetTextField.js:370)
	_handleInput
[Error] Assertion Failed: _styleSheetTextRange data is invalid.
	_updateOwnerStyleText (CSSProperty.js:468)
	_updateStyleText (CSSProperty.js:438)
	rawValue (CSSProperty.js:257)
	_handleValueChange (SpreadsheetStyleProperty.js:736)
	spreadsheetTextFieldDidChange (SpreadsheetStyleProperty.js:348)
	_handleInput (SpreadsheetTextField.js:370)
	_handleInput

[Nikita Vasilyev]

Created attachment 379983 [details]
Patch

The problem with the previous patch was that I updated CSSProperty#styleSheetTextRange but didn't update CSSStyleDeclaration#styleSheetTextRange, which caused data corruption.

[Nikita Vasilyev]

Comment on attachment 379983 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=379983&action=review

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:595
> -        let existingStyles = this._stylesMap.get(mapKey);
> -        if (existingStyles && !style) {
> -            for (let existingStyle of existingStyles) {
> -                if (existingStyle.node === node && existingStyle.inherited === inherited) {
> -                    style = existingStyle;
> -                    break;
> +        if (matchesNode) {
> +            let existingStyles = this._stylesMap.get(mapKey);
> +            if (existingStyles && !style) {
> +                for (let existingStyle of existingStyles) {
> +                    if (existingStyle.node === node && existingStyle.inherited === inherited) {
> +                        style = existingStyle;
> +                        break;
> +                    }
>                  }
>              }
> -        }
>  
> -        if (style)
> -            this._stylesMap.add(mapKey, style);
> +            if (style)
> +                this._stylesMap.add(mapKey, style);
>  
> -        var shorthands = {};
> -        for (var i = 0; payload.shorthandEntries && i < payload.shorthandEntries.length; ++i) {
> -            var shorthand = payload.shorthandEntries[i];
> -            shorthands[shorthand.name] = shorthand.value;
> +            var shorthands = {};
> +            for (var i = 0; payload.shorthandEntries && i < payload.shorthandEntries.length; ++i) {
> +                var shorthand = payload.shorthandEntries[i];
> +                shorthands[shorthand.name] = shorthand.value;
> +            }

This seem like a lot of changes but I simply wrapped a chunk of code in `if (matchesNode) { ... }`

[Devin Rousso]

Comment on attachment 379983 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=379983&action=review

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:459
> +            // Update validity status of each property for rules that don't match the selected DOM node.

I'd either say "`valid` status" or "validity".

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:460
> +            // These rules don't get updated by CSSAgent.getMatchedStylesForNode.

We haven't been adding the "Agent" to comments like these lately, as the `CSSAgent` is purely a frontend concept.  It should just be `CSS.getMatchedStylesForNode` (e.g. "Domain.command").

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:464
> +                const inherited = false;
> +                const pseudoId = null;
> +                const matchesNode = false;

We don't typically create `const` variables when calling functions that are in the same file (especially "private" functions) as it's easy to just scroll up/down, rather than having to switch files.

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:560
> +    _parseStyleDeclarationPayload(payload, node, inherited, pseudoId, type, rule, matchesNode = true)

We usually put optional values like this in a bag, and often have the "default" case be `false`.
```
    _parseStyleDeclarationPayload(payload, node, type, {inherited, pseudoId, rule, nonMatching} = {})
```
This way, we can make all the callsites a bit cleaner in that they only have to pass what they actually need.

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:575
>          let style = rule ? rule.style : null;

I'd imagine that if `nonMatching` is provided, you'd expect a `rule` to be provided.  We should assert as such:
```
    console.assert(!nonMatching || style);
```

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:578
> +            let existingStyles = this._stylesMap.get(mapKey);

We still use `mapKey` below in this function, which adds a newly created `WI.CSSStyleDeclaration` to `_stylesMap`, so we should probably add a `console.assert(!nonMatching)` there.

> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:594
> +            var shorthands = {};
> +            for (var i = 0; payload.shorthandEntries && i < payload.shorthandEntries.length; ++i) {
> +                var shorthand = payload.shorthandEntries[i];
> +                shorthands[shorthand.name] = shorthand.value;

Can we just remove this code?  It doesn't look like it's used for anything.

[Nikita Vasilyev]

Comment on attachment 379983 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=379983&action=review

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:459
>> +            // Update validity status of each property for rules that don't match the selected DOM node.
> 
> I'd either say "`valid` status" or "validity".

Thanks.

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:460
>> +            // These rules don't get updated by CSSAgent.getMatchedStylesForNode.
> 
> We haven't been adding the "Agent" to comments like these lately, as the `CSSAgent` is purely a frontend concept.  It should just be `CSS.getMatchedStylesForNode` (e.g. "Domain.command").

`CSSAgent.getMatchedStylesForNode` is something you can search for in this file and find a function call.

>> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:560
>> +    _parseStyleDeclarationPayload(payload, node, inherited, pseudoId, type, rule, matchesNode = true)
> 
> We usually put optional values like this in a bag, and often have the "default" case be `false`.
> ```
>     _parseStyleDeclarationPayload(payload, node, type, {inherited, pseudoId, rule, nonMatching} = {})
> ```
> This way, we can make all the callsites a bit cleaner in that they only have to pass what they actually need.

I'd prefer to update all _parseStyleDeclarationPayload and _parseRulePayload in a follow-up patch. I prefer to separate refactoring patches from bug fixes.

Also, I don't want to use `nonMatching` name because it's a negative. I'd prefer to use `matchesNode` or `matchingNode`.

[Nikita Vasilyev]

Created attachment 380046 [details]
Patch

[Matt Baker]

Comment on attachment 380046 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 380046 [details]
Patch

Clearing flags on attachment: 380046

Committed r250633: <https://trac.webkit.org/changeset/250633>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.