Bug 198505

<rdar://problem/51374780>

I can reproduce this.

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.

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.

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.

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.

Created attachment 372203 [details] Patch For bots, not for review.

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

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

Created attachment 372308 [details] Patch For bots, not for review.

Created attachment 372318 [details] Patch for bots, not for review One of the changes fixed the problem! Narrowing it doing to see which one.

Created attachment 372355 [details] Patch

Created attachment 372362 [details] [Animated GIF] With patch applied

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

Comment on attachment 372355 [details] Patch Unrelated test failures.

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.

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.

(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.

(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).

Comment on attachment 372355 [details] Patch Clearing flags on attachment: 372355 Committed r246621: <https://trac.webkit.org/changeset/246621>

All reviewed patches have been landed. Closing bug.