Bug 198505 - REGRESSION(r240946): Web Inspector: Styles: Pasting multiple properties has issues
Summary: REGRESSION(r240946): Web Inspector: Styles: Pasting multiple properties has i...
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: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-03 17:03 PDT by Joseph Pecoraro
Modified: 2019-06-19 17:40 PDT (History)
8 users (show)

See Also:


Attachments
Patch (2.85 KB, patch)
2019-06-08 17:03 PDT, Nikita Vasilyev
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-highsierra (3.07 MB, application/zip)
2019-06-08 18:09 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (2.86 MB, application/zip)
2019-06-08 18:17 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-highsierra (2.88 MB, application/zip)
2019-06-08 18:49 PDT, Build Bot
no flags Details
Patch (2.88 KB, patch)
2019-06-10 15:19 PDT, Nikita Vasilyev
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-highsierra (3.02 MB, application/zip)
2019-06-10 19:57 PDT, Build Bot
no flags Details
Patch (4.11 KB, patch)
2019-06-12 17:09 PDT, Nikita Vasilyev
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-highsierra (3.15 MB, application/zip)
2019-06-12 18:38 PDT, Build Bot
no flags Details
Patch (6.00 KB, patch)
2019-06-15 16:56 PDT, Nikita Vasilyev
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-highsierra (2.99 MB, application/zip)
2019-06-15 18:44 PDT, Build Bot
no flags Details
Patch (7.00 KB, patch)
2019-06-17 18:03 PDT, Nikita Vasilyev
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
Patch for bots, not for review (6.35 KB, patch)
2019-06-17 19:38 PDT, Nikita Vasilyev
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
Patch (5.15 KB, patch)
2019-06-18 11:39 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
[Animated GIF] With patch applied (115.61 KB, image/gif)
2019-06-18 12:33 PDT, Nikita Vasilyev
no flags Details
Archive of layout-test-results from ews211 for win-future (13.94 MB, application/zip)
2019-06-18 15:38 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 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
Comment 1 Radar WebKit Bug Importer 2019-06-03 17:03:47 PDT
<rdar://problem/51374780>
Comment 2 Nikita Vasilyev 2019-06-03 17:16:40 PDT
I can reproduce this.
Comment 3 Nikita Vasilyev 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.
Comment 4 Nikita Vasilyev 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.
Comment 5 Build Bot 2019-06-08 18:09:09 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2019-06-08 18:09:11 PDT Comment hidden (obsolete)
Comment 7 Build Bot 2019-06-08 18:17:26 PDT Comment hidden (obsolete)
Comment 8 Build Bot 2019-06-08 18:17:28 PDT Comment hidden (obsolete)
Comment 9 Build Bot 2019-06-08 18:49:55 PDT Comment hidden (obsolete)
Comment 10 Build Bot 2019-06-08 18:49:56 PDT Comment hidden (obsolete)
Comment 11 Nikita Vasilyev 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.
Comment 12 Nikita Vasilyev 2019-06-10 15:19:18 PDT Comment hidden (obsolete)
Comment 13 Build Bot 2019-06-10 19:57:35 PDT Comment hidden (obsolete)
Comment 14 Build Bot 2019-06-10 19:57:37 PDT Comment hidden (obsolete)
Comment 15 Nikita Vasilyev 2019-06-12 17:09:58 PDT Comment hidden (obsolete)
Comment 16 Build Bot 2019-06-12 18:38:11 PDT Comment hidden (obsolete)
Comment 17 Build Bot 2019-06-12 18:38:13 PDT Comment hidden (obsolete)
Comment 18 Nikita Vasilyev 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.
Comment 19 Nikita Vasilyev 2019-06-15 16:56:59 PDT
Created attachment 372203 [details]
Patch

For bots, not for review.
Comment 20 Build Bot 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
Comment 21 Build Bot 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
Comment 22 Nikita Vasilyev 2019-06-17 18:03:07 PDT
Created attachment 372308 [details]
Patch

For bots, not for review.
Comment 23 Nikita Vasilyev 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.
Comment 24 Nikita Vasilyev 2019-06-18 11:39:06 PDT
Created attachment 372355 [details]
Patch
Comment 25 Nikita Vasilyev 2019-06-18 12:33:47 PDT
Created attachment 372362 [details]
[Animated GIF] With patch applied
Comment 26 Build Bot 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
Comment 27 Build Bot 2019-06-18 15:38:19 PDT Comment hidden (obsolete)
Comment 28 Nikita Vasilyev 2019-06-18 15:51:00 PDT
Comment on attachment 372355 [details]
Patch

Unrelated test failures.
Comment 29 Devin Rousso 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.
Comment 30 Matt Baker 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.
Comment 31 Nikita Vasilyev 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.
Comment 32 Nikita Vasilyev 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).
Comment 33 WebKit Commit Bot 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>
Comment 34 WebKit Commit Bot 2019-06-19 17:40:35 PDT
All reviewed patches have been landed.  Closing bug.