Bug 179622 – Web Inspector: Styles Redesign: Pasting multiple properties should create properties instead of a bad property

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Pasting multiple properties should create properties instead of a bad property

Copy this text:
---------
margin: 0;
padding: 0;
---------

Steps to reproduce:
1. Copy the above text
2. Inspect this page
3. Show the Styles sidebar
4. Click to start adding a new property
5. Paste
  => Bad behavior, expected 2 new properties to be created.

[Radar WebKit Bug Importer]

<rdar://problem/35511170>

[Nikita Vasilyev]

Devin, out of all styles sidebar bugs I haven't started working on, this is the most important one! If you don't feel like tackling it, please assign back to me.

[Devin Rousso]

Created attachment 329708 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 329708 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:567
> +                    properties.lastValue.value = text.substring(savedIndex, i).trim();

1. This doesn't look for CSS comments.

2. Pasting the following throws an exception:

    1px;
    padding: 0;

[Error] TypeError: undefined is not an object (evaluating 'properties.lastValue.value = text.substring(savedIndex, i).trim()')
	_handleNamePaste (SpreadsheetStyleProperty.js:569)
	_handleNamePaste

It shouldn't throw even when the CSS is invalid.

---

I see you're trying to guess how many properties are being added so you can add a blank property after. I don't know how to make it simpler just yet, but I strongly suggest to avoid parsing CSS on the front-end.

[Nikita Vasilyev]

Comment on attachment 329708 [details]
Patch

> I see you're trying to guess how many properties are being added so you can add a blank property after. I don't know how to make it simpler just yet, but I strongly suggest to avoid parsing CSS on the front-end.

I have an idea how to avoid guessing how many properties are inserted on the front-end.

Say, we are adding a blank property after the first one:

    color: red;
    []
    float: left;
    outline: none;
    

and pasting:

    font-size: 12px;
    font-family: sans-serif;

After pasting, we want to focus on the next property, "float: left", regardless of how many properties are being added.
During pasting, we know there are 4 properties in the style declaration (the focused property is blank).
"float: left" is the second property from the end. Regardless of how many properties are being added, we always want to focus on the second property from the end.

[Devin Rousso]

Created attachment 332000 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 332000 [details]
Patch

The pasted properties don't appear in the styles sidebar. Looks like layout doesn't get called on WI.SpreadsheetCSSStyleDeclarationEditor.

This could be related to https://trac.webkit.org/changeset/227370/webkit. I'm looking into this.

[Nikita Vasilyev]

I just checkout out one revision before https://trac.webkit.org/changeset/227370/webkit, and pasting worked fine.
I'm looking into making pasting work with the new locking.

[Nikita Vasilyev]

This seems to fix it:

    _propertiesChanged(event)
    {
+       let hasPastedProperties = !isNaN(this._pendingAddBlankPropertyIndexOffset);
-       if (this.editing) {
+       if (this.editing && !hasPastedProperties) {
            for (let propertyView of this._propertyViews)
                propertyView.updateStatus();
        } else
            this.needsLayout();
    }

[Devin Rousso]

Created attachment 332463 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 332463 [details]
Patch

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

It works as expected now. Good work!

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:126
> +    remove(replacement = "")

Nit: I'd introduce a new method and call it `replace` or `replaceWithText`.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:566
> +    _handleNamePaste(event)

This patch only addresses pasting CSS properties into name fields. I imagine this is the most common case so it's fine. Eventually, it would be nice to handle pasting into value fields as well.

    color: []

Pasting:

    red;
    font-size: 12px;

Should result in:

    color: red;
    font-size: 12px;

[Matt Baker]

Comment on attachment 332463 [details]
Patch

r- for the following:

Pasting multiple commented properties doesn't work, and leaves the Style {} UI in an unusable state.

Paste `/*color: red; margin: 0;*/` in the blank property created after clicking in the Style {} UI.

Expected:
Style {
    [ ] /* color: red; */
    [ ] /* margin: 0; */
    [ ] _
}

Actual:
Style {
}

The Style {} UI is blank, and clicking inside it no longer creates a new blank property.

[Nikita Vasilyev]

(In reply to Matt Baker from comment #12)
> Comment on attachment 332463 [details]
> Patch
> 
> r- for the following:
> 
> Pasting multiple commented properties doesn't work, and leaves the Style {}
> UI in an unusable state.
> 
> Paste `/*color: red; margin: 0;*/` in the blank property created after
> clicking in the Style {} UI.
> 
> Expected:
> Style {
>     [ ] /* color: red; */
>     [ ] /* margin: 0; */
>     [ ] _
> }
> 
> Actual:
> Style {
> }
> 
> The Style {} UI is blank, and clicking inside it no longer creates a new
> blank property.

This isn't specific to CSS pasting. A resource with `/*color: red; margin: 0;*/` don't display two individually commented out properties.

CSS parser on the backend only identifies a single commented out property as disabled.

Disabled property:

    /* color: red */

Not a property:

    /*color: red; margin: 0;*/

Pasting a single commented out CSS property, such as "/* color: red */", works as expected.
We use the same CSS parser as Chrome, so Chrome DevTools have the exact same issues.

[Matt Baker]

(In reply to Nikita Vasilyev from comment #13)
> (In reply to Matt Baker from comment #12)
> > Comment on attachment 332463 [details]
> > Patch
> > 
> > r- for the following:
> > 
> > Pasting multiple commented properties doesn't work, and leaves the Style {}
> > UI in an unusable state.
> > 
> > Paste `/*color: red; margin: 0;*/` in the blank property created after
> > clicking in the Style {} UI.
> > 
> > Expected:
> > Style {
> >     [ ] /* color: red; */
> >     [ ] /* margin: 0; */
> >     [ ] _
> > }
> > 
> > Actual:
> > Style {
> > }
> > 
> > The Style {} UI is blank, and clicking inside it no longer creates a new
> > blank property.
> 
> This isn't specific to CSS pasting. A resource with `/*color: red; margin:
> 0;*/` don't display two individually commented out properties.
> 
> CSS parser on the backend only identifies a single commented out property as
> disabled.
> 
> Disabled property:
> 
>     /* color: red */
> 
> Not a property:
> 
>     /*color: red; margin: 0;*/
> 
> Pasting a single commented out CSS property, such as "/* color: red */",
> works as expected.
> We use the same CSS parser as Chrome, so Chrome DevTools have the exact same
> issues.

Okay that's fine, but still r- until the issue with the UI being broken after pasting is resolved.

[Nikita Vasilyev]

Oh, it is broken when pasting `/*color: red; margin: 0;*/` to a rule (such as a style attribute) without any existing properties.

[Devin Rousso]

Created attachment 332945 [details]
Patch

[Matt Baker]

Comment on attachment 332945 [details]
Patch

r=me, nice work.

[WebKit Commit Bot]

Comment on attachment 332945 [details]
Patch

Clearing flags on attachment: 332945

Committed r228030: <https://trac.webkit.org/changeset/228030>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.