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.
<rdar://problem/35511170>
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.
Created attachment 329708 [details] Patch
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 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.
Created attachment 332000 [details] Patch
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.
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.
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(); }
Created attachment 332463 [details] Patch
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 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.
(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.
(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.
Oh, it is broken when pasting `/*color: red; margin: 0;*/` to a rule (such as a style attribute) without any existing properties.
Created attachment 332945 [details] Patch
Comment on attachment 332945 [details] Patch r=me, nice work.
Comment on attachment 332945 [details] Patch Clearing flags on attachment: 332945 Committed r228030: <https://trac.webkit.org/changeset/228030>
All reviewed patches have been landed. Closing bug.