REGRESSION(r240946): Web Inspector: Styles: Pasting multiple properties has issues

[Joseph Pecoraro]

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

[Radar WebKit Bug Importer]

<rdar://problem/51374780>

[Nikita Vasilyev]

I can reproduce this.

[Nikita Vasilyev]

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]

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 on attachment 371683 [details]
Patch

Attachment 371683 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12422090

New failing tests:
inspector/css/modify-css-property-race.html

[EWS Watchlist]

Created attachment 371688 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 371683 [details]
Patch

Attachment 371683 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12422098

New failing tests:
inspector/css/modify-css-property-race.html

[EWS Watchlist]

Created attachment 371690 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 371683 [details]
Patch

Attachment 371683 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12422125

New failing tests:
inspector/css/modify-css-property-race.html
inspector/css/modify-css-property.html

[EWS Watchlist]

Created attachment 371694 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Nikita Vasilyev]

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]

Created attachment 371783 [details]
Patch

[EWS Watchlist]

Comment on attachment 371783 [details]
Patch

Attachment 371783 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12439613

New failing tests:
inspector/css/modify-css-property.html

[EWS Watchlist]

Created attachment 371814 [details]
Archive of layout-test-results from ews116 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Nikita Vasilyev]

Created attachment 372003 [details]
Patch

To run on bots. Not for reviews.

[EWS Watchlist]

Comment on attachment 372003 [details]
Patch

Attachment 372003 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12459678

New failing tests:
inspector/css/modify-css-property.html

[EWS Watchlist]

Created attachment 372011 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Nikita Vasilyev]

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]

Created attachment 372203 [details]
Patch

For bots, not for review.

[EWS Watchlist]

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]

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]

Created attachment 372308 [details]
Patch

For bots, not for review.

[Nikita Vasilyev]

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]

Created attachment 372355 [details]
Patch

[Nikita Vasilyev]

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

[EWS Watchlist]

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]

Created attachment 372394 [details]
Archive of layout-test-results from ews211 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews211  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Nikita Vasilyev]

Comment on attachment 372355 [details]
Patch

Unrelated test failures.

[Devin Rousso]

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 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]

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

(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 on attachment 372355 [details]
Patch

Clearing flags on attachment: 372355

Committed r246621: <https://trac.webkit.org/changeset/246621>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.