WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
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
,
EWS Watchlist
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
,
EWS Watchlist
no flags
Details
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
Details
Patch
(2.88 KB, patch)
2019-06-10 15:19 PDT
,
Nikita Vasilyev
ews-watchlist
: 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
,
EWS Watchlist
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
,
EWS Watchlist
no flags
Details
Patch
(6.00 KB, patch)
2019-06-15 16:56 PDT
,
Nikita Vasilyev
ews-watchlist
: 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
,
EWS Watchlist
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
,
EWS Watchlist
no flags
Details
Show Obsolete
(13)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2019-06-03 17:03:47 PDT
<
rdar://problem/51374780
>
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)
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
Comment 6
2019-06-08 18:09:11 PDT
Comment hidden (obsolete)
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 7
2019-06-08 18:17:26 PDT
Comment hidden (obsolete)
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
Comment 8
2019-06-08 18:17:28 PDT
Comment hidden (obsolete)
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 9
2019-06-08 18:49:55 PDT
Comment hidden (obsolete)
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
Comment 10
2019-06-08 18:49:56 PDT
Comment hidden (obsolete)
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 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)
Created
attachment 371783
[details]
Patch
EWS Watchlist
Comment 13
2019-06-10 19:57:35 PDT
Comment hidden (obsolete)
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
Comment 14
2019-06-10 19:57:37 PDT
Comment hidden (obsolete)
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
Comment 15
2019-06-12 17:09:58 PDT
Comment hidden (obsolete)
Created
attachment 372003
[details]
Patch To run on bots. Not for reviews.
EWS Watchlist
Comment 16
2019-06-12 18:38:11 PDT
Comment hidden (obsolete)
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
Comment 17
2019-06-12 18:38:13 PDT
Comment hidden (obsolete)
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
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
Created
attachment 372355
[details]
Patch
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)
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 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.
Top of Page
Format For Printing
XML
Clone This Bug