RESOLVED FIXED 198505
REGRESSION(r240946): Web Inspector: Styles: Pasting multiple properties has issues
https://bugs.webkit.org/show_bug.cgi?id=198505
Summary REGRESSION(r240946): Web Inspector: Styles: Pasting multiple properties has i...
Joseph Pecoraro
Reported 2019-06-03 17:03:33 PDT
Web Inspector: Styles Redesign: Pasting multiple properties should create properties instead of a bad property (179622) 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, the new properties are added but don't show up in the sidebar
Attachments
Patch (2.85 KB, patch)
2019-06-08 17:03 PDT, Nikita Vasilyev
nvasilyev: commit-queue-
Archive of layout-test-results from ews103 for mac-highsierra (3.07 MB, application/zip)
2019-06-08 18:09 PDT, EWS Watchlist
no flags
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (2.86 MB, application/zip)
2019-06-08 18:17 PDT, EWS Watchlist
no flags
Archive of layout-test-results from ews114 for mac-highsierra (2.88 MB, application/zip)
2019-06-08 18:49 PDT, EWS Watchlist
no flags
Patch (2.88 KB, patch)
2019-06-10 15:19 PDT, Nikita Vasilyev
ews-watchlist: commit-queue-
Archive of layout-test-results from ews116 for mac-highsierra (3.02 MB, application/zip)
2019-06-10 19:57 PDT, EWS Watchlist
no flags
Patch (4.11 KB, patch)
2019-06-12 17:09 PDT, Nikita Vasilyev
nvasilyev: commit-queue-
Archive of layout-test-results from ews114 for mac-highsierra (3.15 MB, application/zip)
2019-06-12 18:38 PDT, EWS Watchlist
no flags
Patch (6.00 KB, patch)
2019-06-15 16:56 PDT, Nikita Vasilyev
ews-watchlist: commit-queue-
Archive of layout-test-results from ews112 for mac-highsierra (2.99 MB, application/zip)
2019-06-15 18:44 PDT, EWS Watchlist
no flags
Patch (7.00 KB, patch)
2019-06-17 18:03 PDT, Nikita Vasilyev
nvasilyev: commit-queue-
Patch for bots, not for review (6.35 KB, patch)
2019-06-17 19:38 PDT, Nikita Vasilyev
nvasilyev: commit-queue-
Patch (5.15 KB, patch)
2019-06-18 11:39 PDT, Nikita Vasilyev
no flags
[Animated GIF] With patch applied (115.61 KB, image/gif)
2019-06-18 12:33 PDT, Nikita Vasilyev
no flags
Archive of layout-test-results from ews211 for win-future (13.94 MB, application/zip)
2019-06-18 15:38 PDT, EWS Watchlist
no flags
Radar WebKit Bug Importer
Comment 1 2019-06-03 17:03:47 PDT
Nikita Vasilyev
Comment 2 2019-06-03 17:16:40 PDT
I can reproduce this.
Nikita Vasilyev
Comment 3 2019-06-08 16:29:14 PDT
The view doesn't get updated after pasting CSS because WI.CSSStyleDeclaration.Event.PropertiesChanged doesn't fire. CSSStyleDeclaration: update(text, ...) { let oldText = this._text; this._text = text; ... if (oldText === this._text && this._type !== WI.CSSStyleDeclaration.Type.Computed) return; ... this.dispatchEventToListeners(WI.CSSStyleDeclaration.Event.PropertiesChanged); oldText matches this._text, so it returns early. Since r240946, CSSStyleDeclaration#text setter updates text immediately (e.g. it doesn't wait for a backend roundtrip). `oldText === this._text` check no longer makes sense for this case.
Nikita Vasilyev
Comment 4 2019-06-08 17:03:01 PDT
Created attachment 371683 [details] Patch cq- because I'm still wondering if there's a more elegant solution. One that doesn't require adding another property to CSSStyleDeclaration.
EWS Watchlist
Comment 5 2019-06-08 18:09:09 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 6 2019-06-08 18:09:11 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 7 2019-06-08 18:17:26 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 8 2019-06-08 18:17:28 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 9 2019-06-08 18:49:55 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 10 2019-06-08 18:49:56 PDT Comment hidden (obsolete)
Nikita Vasilyev
Comment 11 2019-06-10 15:17:58 PDT
Comment on attachment 371683 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371683&action=review > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:187 > - if (oldText === this._text && this._type !== WI.CSSStyleDeclaration.Type.Computed) > + if (!this._pendingPropertiesChanged && this._type !== WI.CSSStyleDeclaration.Type.Computed) The failed test modified the style attribute. We should keep `oldText === this._text` for that case.
Nikita Vasilyev
Comment 12 2019-06-10 15:19:18 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 13 2019-06-10 19:57:35 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 14 2019-06-10 19:57:37 PDT Comment hidden (obsolete)
Nikita Vasilyev
Comment 15 2019-06-12 17:09:58 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 16 2019-06-12 18:38:11 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 17 2019-06-12 18:38:13 PDT Comment hidden (obsolete)
Nikita Vasilyev
Comment 18 2019-06-15 16:42:43 PDT
modify-css-property.html fails only on Debug and I trying to figure out why. I built Debug ToT WebKit locally and every test I run times out. This is even without my patch; master branch. $ run-webkit-tests --debug --time-out-ms=2000 --no-retry-failures --no-build LayoutTests/inspector/unit-tests/array-utilities.html WebKitBuild/Debug/layout-test-results/inspector/unit-tests/array-utilities-actual.txt: #PID UNRESPONSIVE - WebKitTestRunner (pid 19059) FAIL: Timed out waiting for notifyDone to be called #EOF #EOF I'm trying to figure out what's wrong with my WebKit build.
Nikita Vasilyev
Comment 19 2019-06-15 16:56:59 PDT
Created attachment 372203 [details] Patch For bots, not for review.
EWS Watchlist
Comment 20 2019-06-15 18:44:01 PDT
Comment on attachment 372203 [details] Patch Attachment 372203 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12486684 New failing tests: inspector/css/modify-css-property.html
EWS Watchlist
Comment 21 2019-06-15 18:44:03 PDT
Created attachment 372208 [details] Archive of layout-test-results from ews112 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-highsierra Platform: Mac OS X 10.13.6
Nikita Vasilyev
Comment 22 2019-06-17 18:03:07 PDT
Created attachment 372308 [details] Patch For bots, not for review.
Nikita Vasilyev
Comment 23 2019-06-17 19:38:29 PDT
Created attachment 372318 [details] Patch for bots, not for review One of the changes fixed the problem! Narrowing it doing to see which one.
Nikita Vasilyev
Comment 24 2019-06-18 11:39:06 PDT
Nikita Vasilyev
Comment 25 2019-06-18 12:33:47 PDT
Created attachment 372362 [details] [Animated GIF] With patch applied
EWS Watchlist
Comment 26 2019-06-18 15:38:15 PDT
Comment on attachment 372355 [details] Patch Attachment 372355 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12512143 New failing tests: fast/frames/take-focus-from-iframe.html svg/text/textpath-reference-update.html css3/filters/blur-various-radii.html
EWS Watchlist
Comment 27 2019-06-18 15:38:19 PDT Comment hidden (obsolete)
Nikita Vasilyev
Comment 28 2019-06-18 15:51:00 PDT
Comment on attachment 372355 [details] Patch Unrelated test failures.
Devin Rousso
Comment 29 2019-06-19 17:07:50 PDT
Comment on attachment 372355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372355&action=review So why exactly aren't we rolling out r240946 then? It seems like that broke this functionality. > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:187 > + if (oldText === this._text && !this._pendingPropertiesChanged && this._type !== WI.CSSStyleDeclaration.Type.Computed) Based on the explanation in the ChangeLog, doesn't this mean that `oldText === this._text` will always be true? > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:194 > this.dispatchEventToListeners(WI.CSSStyleDeclaration.Event.PropertiesChanged); Is there a reason to not just always fire `CSSStyleDeclaration.Event.PropertiesChanged`? All of the listeners for it just refresh their UI.
Matt Baker
Comment 30 2019-06-19 17:08:30 PDT
Comment on attachment 372355 [details] Patch r=me. Reviewing the surrounding code makes me think that CSSStyleDeclaration needs to be rewritten. It is very hard to follow.
Nikita Vasilyev
Comment 31 2019-06-19 17:13:26 PDT
(In reply to Devin Rousso from comment #29) > Comment on attachment 372355 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=372355&action=review > > So why exactly aren't we rolling out r240946 then? It seems like that broke > this functionality. r240946 "Web Inspector: Styles: fix race conditions when editing" did fix race conditions. I still think it was the right direction. > > > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:187 > > + if (oldText === this._text && !this._pendingPropertiesChanged && this._type !== WI.CSSStyleDeclaration.Type.Computed) > > Based on the explanation in the ChangeLog, doesn't this mean that `oldText > === this._text` will always be true? > > > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:194 > > this.dispatchEventToListeners(WI.CSSStyleDeclaration.Event.PropertiesChanged); > > Is there a reason to not just always fire > `CSSStyleDeclaration.Event.PropertiesChanged`? All of the listeners for it > just refresh their UI. I did that initially. See https://bugs.webkit.org/show_bug.cgi?id=198505#c4. It broke the test when inline CSS was modified via web page's JS.
Nikita Vasilyev
Comment 32 2019-06-19 17:22:35 PDT
(In reply to Matt Baker from comment #30) > Comment on attachment 372355 [details] > Patch > > r=me. Reviewing the surrounding code makes me think that CSSStyleDeclaration > needs to be rewritten. It is very hard to follow. I agree that it's hard to follow. I don't think rewriting CSSStyleDeclaration alone would be sufficient. I find most of DOMNodeStyles class is even harder to understand than CSSStyleDeclaration. On the bright side, we have more CSS editor tests than a year ago. So making changes is slightly less risky now. Let me know what was particularly hard to follow (either here or in-person later).
WebKit Commit Bot
Comment 33 2019-06-19 17:40:33 PDT
Comment on attachment 372355 [details] Patch Clearing flags on attachment: 372355 Committed r246621: <https://trac.webkit.org/changeset/246621>
WebKit Commit Bot
Comment 34 2019-06-19 17:40:35 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.