Bug 178835 – Web Inspector: Styles: format style declarations after editing

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Currently, a newly added property get appended without any indentation or line-breaks.

Adding "color: red" to:

    body {
        font-family: sans-serif;
    }

Currently results in:

    body {
        font-family: sans-serif;color: red;
    }

Expected (when the indentation in setting is set to 4 spaces):

    body {
        font-family: sans-serif;
        color: red;
    }

[Radar WebKit Bug Importer]

<rdar://problem/35185060>

[Nikita Vasilyev]

Created attachment 429077 [details]
Patch

[Nikita Vasilyev]

Created attachment 429078 [details]
Patch

[Devin Rousso]

Comment on attachment 429078 [details]
Patch

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

So does this mean that if I'm editing a minified CSS style sheet resource in the Styles panel then my changes will result in a bunch of pretty-printed text appearing in the middle of the minified resource?  I'm not sure if that's what we want to do.  Maybe we should only do this if we believe that the style sheet resource already is pretty-printed (e.g. using `isTextLikelyMinified`)?

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:514
> +    generateFormattedText()

Why not use/modify `generateCSSRuleString`?  Like have an `options = {}` that controls what things are added so that we don't have repeated work (e.g. `includeGroupings`, `allowInline`, etc.).

[Nikita Vasilyev]

Comment on attachment 429078 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:514
>> +    generateFormattedText()
> 
> Why not use/modify `generateCSSRuleString`?  Like have an `options = {}` that controls what things are added so that we don't have repeated work (e.g. `includeGroupings`, `allowInline`, etc.).

Yes, I can merge the two.

Caveats:
- generateCSSRuleString generates a CSS rule, so it includes `cssSelector { cssDeclaration }`. It should probably make more sense to have this method on CSSRule, not CSSStyleDeclaration, but I digress.
- indentation is slightly different (e.g. for the closing "}")

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #4)
> Comment on attachment 429078 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429078&action=review
> 
> So does this mean that if I'm editing a minified CSS style sheet resource in
> the Styles panel then my changes will result in a bunch of pretty-printed
> text appearing in the middle of the minified resource?  I'm not sure if
> that's what we want to do. ...

Yes.

Why is it bad? Is it for saving the resource?

It is certainly easier to read and edit non-minified resource.

[Nikita Vasilyev]

Created attachment 430394 [details]
Patch

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #4)
> > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:514
> > +    generateFormattedText()
> 
> Why not use/modify `generateCSSRuleString`?  Like have an `options = {}`
> that controls what things are added so that we don't have repeated work
> (e.g. `includeGroupings`, `allowInline`, etc.).

I did that. The options are different though.


(In reply to Nikita Vasilyev from comment #5)
> - generateCSSRuleString generates a CSS rule, so it includes `cssSelector {
> cssDeclaration }`. It should probably make more sense to have this method on
> CSSRule, not CSSStyleDeclaration, but I digress.

I tried to refactor this and move generateCSSRuleString method to CSSRule. Then realized that it also need to work for inline styles. It makes sense to me to keep it as-is now.

[Nikita Vasilyev]

(In reply to Nikita Vasilyev from comment #8)
> (In reply to Devin Rousso from comment #4)
> > > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:514
> > > +    generateFormattedText()
> > 
> > Why not use/modify `generateCSSRuleString`?  Like have an `options = {}`
> > that controls what things are added so that we don't have repeated work
> > (e.g. `includeGroupings`, `allowInline`, etc.).
> 
> I did that. The options are different though.

To amend my message: I kept the two methods separate, still. I think generateCSSRuleString should generate, well, a CSS rule. It shouldn't generate a style declaration (e.g. a list of CSS properties).

[Nikita Vasilyev]

Created attachment 430422 [details]
Patch

[Devin Rousso]

Comment on attachment 430422 [details]
Patch

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

> LayoutTests/inspector/css/generateFormattedText.htmlLayoutTests/inspector/css/generateCSSRuleString.html:22
> +            InspectorTest.log(nodeStyles.inlineStyle.generateFormattedText({includeGroupings: true, forceMultiline: true}));

Can we add some tests for the other permutations (e.g. `{includeGroupings: true}`)?

> Source/WebInspectorUI/ChangeLog:18
> +        Introduce `_pendingText` property. It's only set when pasting CSS property text into the CSS property name field.

Can you explain why this is needed?  Why can't we just `this._text = text;`?

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:190
>          this._text = newText;

Do we need to do this before calling `_updateOwnerStyleText`?

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:-515
> -        this._styleSheetTextRange = this._styleSheetTextRange.cloneAndModify(0, 0, lineDelta, columnDelta);

Can we verify that the `_styleSheetTextRange` gets updated (and to the right value) after `this._ownerStyle.text = ...`?

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:503
> +            styleText += indentString.repeat(groupingsCount) + this.selectorText + " {";

We should probably have a separate option for `includeSelector` since it's not a CSS grouping (or rename `includeGrouping` to also mention "selector" or something).  I would also add a `console.assert(!options.includeGroupings || options.includeSelector, options)` too as it doesn't make sense to show CSS groupings without the selector.

[Nikita Vasilyev]

Created attachment 433595 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 430422 [details]
Patch

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

>> Source/WebInspectorUI/ChangeLog:18
>> +        Introduce `_pendingText` property. It's only set when pasting CSS property text into the CSS property name field.
> 
> Can you explain why this is needed?  Why can't we just `this._text = text;`?

When pasting CSS, we need to save that exact text. We don't want to save formatted text based on the name/value — they are outdated now.

In my latest patch, I replaced `_pendingText` with `_isTextPendingSave`, which is a boolean flag.

[Devin Rousso]

Comment on attachment 433595 [details]
Patch

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

> LayoutTests/ChangeLog:11
> +        - Add a basic test unsuring styleSheetTextRange updates correctly from the backend.

s/unsuring/ensuring

> LayoutTests/inspector/css/generateFormattedText-expected.txt:12
> +

I don't think there should be a newline at the start when we're not including groupings/selectors

> LayoutTests/inspector/css/generateFormattedText-expected.txt:13
> +    a: 1;

It's odd that this is indented when there's no groupings/selectors 🤔

> LayoutTests/inspector/css/generateFormattedText-expected.txt:32
> +

ditto (:12)

> LayoutTests/inspector/css/generateFormattedText-expected.txt:33
> +    a: 1;

ditto (:13)

> LayoutTests/inspector/css/generateFormattedText-expected.txt:53
> +@media only screen and (min-width: 0px) {@media only screen and (min-height: 0px) {body > div#test-node {a: 1; b: 2; c: 3;}}}

It's a bit odd that we add spaces between `#test-node` and `{` or between CSS properties, but not between `{` and `@media`/`body` or before/after the first/last CSS property.

Maybe we should be a bit more liberal with whitespace?

> LayoutTests/inspector/css/generateFormattedText-expected.txt:60
> +

ditto (:12)

> LayoutTests/inspector/css/generateFormattedText-expected.txt:61
> +            a: 1;

ditto (:13)

> LayoutTests/inspector/css/generateFormattedText-expected.txt:127
> +@media only screen and (min-width: 0px) {@media only screen and (min-height: 0px) {body > div#test-node {a: 1; /* b: 2; */ c: 3;}}}

ditto (:53)

> LayoutTests/inspector/css/generateFormattedText-expected.txt:134
> +

ditto (:12)

> LayoutTests/inspector/css/generateFormattedText-expected.txt:135
> +            a: 1;

ditto (:13)

> LayoutTests/inspector/css/generateFormattedText.html:16
> +    let suite = InspectorTest.createSyncSuite("CSS.generateCSSRuleString");

NIT: this should really be `"WI.CSSStyelDeclaration.prototype.generateFormattedText"` (and ditto below too)

> LayoutTests/inspector/css/modify-css-property.html:44
> +            styleDeclaration.awaitEvent(WI.CSSStyleDeclaration.Event.PropertiesChanged).then((event) => {

Can you use `singleFireEventListener` to make sure that the extra microtask of the `Promise` isn't influencing the test result?

> LayoutTests/inspector/css/modify-css-property.html:48
> +                let relativeRange = fontProperty.styleSheetTextRange.relativeTo(fontProperty.ownerStyle.styleSheetTextRange.startLine, fontProperty.ownerStyle.styleSheetTextRange.startOffset);
> +                relativeRange.resolveOffsets(fontProperty.ownerStyle.text);
> +                let propertyText = fontProperty.ownerStyle.text.slice(relativeRange.startOffset, relativeRange.endOffset);

Is this logic actually found in Web Inspector?  If yes, why can't we test that code instead of this?  If not, why is this needed?

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:214
> +        this._isTextPendingSave = true;

Please add this variable to the constructor to avoid adding a new JSC structure.

NIT: I think this needs a clearer name.  Maybe something like `_isUpdatingText` or `_isWaitingForResponseAfterChangingText`?

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:222
> +            return this._text.trim();

Instead of `trim()` each time this is called, can we `trim()` once inside `set text` instead?

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:507
> +        this._updateOwnerStyleText();

Should this also be wrapped with `_isTextPendingSave`?

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:516
> +        let multiline = this._ownerStyle.type === WI.CSSStyleDeclaration.Type.Rule

NIT: I'd inline this

[Nikita Vasilyev]

Comment on attachment 433595 [details]
Patch

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

>> LayoutTests/inspector/css/generateFormattedText-expected.txt:13
>> +    a: 1;
> 
> It's odd that this is indented when there's no groupings/selectors 🤔

It does indeed look odd in the test, but that's what we want to do to save formatted text — we need to send style declarations, which is everything inside of `selectior {...}`, but no selectors or groupings.

>> LayoutTests/inspector/css/generateFormattedText-expected.txt:53
>> +@media only screen and (min-width: 0px) {@media only screen and (min-height: 0px) {body > div#test-node {a: 1; b: 2; c: 3;}}}
> 
> It's a bit odd that we add spaces between `#test-node` and `{` or between CSS properties, but not between `{` and `@media`/`body` or before/after the first/last CSS property.
> 
> Maybe we should be a bit more liberal with whitespace?

This is not a real-world scenario. I'm testing dead code, essentially.
includeGroupingsAndSelectors is always used with `multiline` (for Copy Rule context menu).

>> LayoutTests/inspector/css/generateFormattedText.html:16
>> +    let suite = InspectorTest.createSyncSuite("CSS.generateCSSRuleString");
> 
> NIT: this should really be `"WI.CSSStyelDeclaration.prototype.generateFormattedText"` (and ditto below too)

Oops, thanks.

>> LayoutTests/inspector/css/modify-css-property.html:48
>> +                let propertyText = fontProperty.ownerStyle.text.slice(relativeRange.startOffset, relativeRange.endOffset);
> 
> Is this logic actually found in Web Inspector?  If yes, why can't we test that code instead of this?  If not, why is this needed?

No, I don't think it is.
I'm testing that styleSheetTextRange is correctly updated from the backend. Initially, I used expectEqual on styleSheetTextRange objects themselves, but it was too cryptic and hard to understand when the test was failing.

>> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:214
>> +        this._isTextPendingSave = true;
> 
> Please add this variable to the constructor to avoid adding a new JSC structure.
> 
> NIT: I think this needs a clearer name.  Maybe something like `_isUpdatingText` or `_isWaitingForResponseAfterChangingText`?

`_isUpdatingText` sounds good to me.

>> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:222
>> +            return this._text.trim();
> 
> Instead of `trim()` each time this is called, can we `trim()` once inside `set text` instead?

Either way it should only get called once. But sounds good.

>> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:507
>> +        this._updateOwnerStyleText();
> 
> Should this also be wrapped with `_isTextPendingSave`?

Like `if (this._isTextPendingSave) this._updateOwnerStyleText()` ? No, it shouldn't because `_isTextPendingSave` is false when we edit name/value. It's only true pasting CSS and commenting/uncommenting (the latter could be rewritten to not use this._text directly, actually, but I digress).

[Nikita Vasilyev]

Comment on attachment 433595 [details]
Patch

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

>>> LayoutTests/inspector/css/generateFormattedText-expected.txt:13
>>> +    a: 1;
>> 
>> It's odd that this is indented when there's no groupings/selectors 🤔
> 
> It does indeed look odd in the test, but that's what we want to do to save formatted text — we need to send style declarations, which is everything inside of `selectior {...}`, but no selectors or groupings.

To paraphrase, I need to add this newline at the start for the saved formatted text to make sense. For example, when I edit a CSS property in:

#foo {
    font-size: 14px;
}

I don't want to save it as:

#foo {    font-size: 14px;
}

[Nikita Vasilyev]

Created attachment 434469 [details]
Patch

[Devin Rousso]

Comment on attachment 434469 [details]
Patch

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

r=me

I still think it's a bit odd to have formatted text suddenly appear in the middle of a minified resource, but perhaps that could be a good thing as it would stand out more (and make subsequent edits easier).  It's also fair to say that when viewing a minified resource Web Inspector should auto-format it anyways, so unless the developer manually changes back to the minified view or saves the file to disk they're unlikely to be bothered by this.  Most developers spend their time in the Styles details sidebar panel in the Elements Tab anyways.

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:499
> +        if (options.includeGroupingsAndSelectors) {

NIT: I'm just realizing now that this probably should be `includeSelectorsAndGroupings` since you can't have a grouping without selectors but you can have the opposite, but it's not a big deal :P

[Razvan Caliman]

Created attachment 440369 [details]
Patch

Carry over R+; Address nit

[Razvan Caliman]

Created attachment 440505 [details]
Patch

Carry over R+; reattach Nikita\'s original patch

[EWS]

Committed r283723 (242647@main): <https://commits.webkit.org/242647@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 440505 [details].