Bug 179622 - Web Inspector: Styles Redesign: Pasting multiple properties should create properties instead of a bad property
Summary: Web Inspector: Styles Redesign: Pasting multiple properties should create pro...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-13 12:08 PST by Joseph Pecoraro
Modified: 2018-02-02 14:53 PST (History)
5 users (show)

See Also:


Attachments
Patch (9.75 KB, patch)
2017-12-18 16:40 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (8.72 KB, patch)
2018-01-22 20:05 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (9.17 KB, patch)
2018-01-27 00:44 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (9.21 KB, patch)
2018-02-02 00:14 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2017-11-13 12:08:40 PST
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.
Comment 1 Radar WebKit Bug Importer 2017-11-13 12:09:08 PST
<rdar://problem/35511170>
Comment 2 Nikita Vasilyev 2017-12-15 18:37:17 PST
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.
Comment 3 Devin Rousso 2017-12-18 16:40:34 PST
Created attachment 329708 [details]
Patch
Comment 4 Nikita Vasilyev 2017-12-19 16:49:49 PST
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.
Comment 5 Nikita Vasilyev 2018-01-05 16:43:28 PST
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.
Comment 6 Devin Rousso 2018-01-22 20:05:41 PST
Created attachment 332000 [details]
Patch
Comment 7 Nikita Vasilyev 2018-01-24 17:00:07 PST
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.
Comment 8 Nikita Vasilyev 2018-01-24 17:40:47 PST
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.
Comment 9 Nikita Vasilyev 2018-01-24 19:02:54 PST
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();
    }
Comment 10 Devin Rousso 2018-01-27 00:44:36 PST
Created attachment 332463 [details]
Patch
Comment 11 Nikita Vasilyev 2018-01-27 19:31:01 PST
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;
Comment 12 Matt Baker 2018-02-01 15:06:22 PST
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.
Comment 13 Nikita Vasilyev 2018-02-01 16:40:36 PST
(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.
Comment 14 Matt Baker 2018-02-01 16:43:39 PST
(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.
Comment 15 Nikita Vasilyev 2018-02-01 16:53:26 PST
Oh, it is broken when pasting `/*color: red; margin: 0;*/` to a rule (such as a style attribute) without any existing properties.
Comment 16 Devin Rousso 2018-02-02 00:14:18 PST
Created attachment 332945 [details]
Patch
Comment 17 Matt Baker 2018-02-02 14:28:17 PST
Comment on attachment 332945 [details]
Patch

r=me, nice work.
Comment 18 WebKit Commit Bot 2018-02-02 14:53:01 PST
Comment on attachment 332945 [details]
Patch

Clearing flags on attachment: 332945

Committed r228030: <https://trac.webkit.org/changeset/228030>
Comment 19 WebKit Commit Bot 2018-02-02 14:53:03 PST
All reviewed patches have been landed.  Closing bug.