WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
159734
Web Inspector: Command-Z doesn't work when editing CSS selectors in Styles sidebar
https://bugs.webkit.org/show_bug.cgi?id=159734
Summary
Web Inspector: Command-Z doesn't work when editing CSS selectors in Styles si...
Nikita Vasilyev
Reported
2016-07-13 13:27:12 PDT
Steps: 1. Inspect <body> on this page. 2. In the Styles sidebar on the right focus on "body". 3. Type "bar" 4. Press Cmd-Z Actual: Nothing happened. Expected: Selector reverted to the original value, e.g. "body".
Attachments
[Patch] WIP
(30.13 KB, patch)
2016-08-24 22:46 PDT
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews107 for mac-yosemite-wk2
(943.83 KB, application/zip)
2016-08-31 00:12 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews102 for mac-yosemite
(850.29 KB, application/zip)
2016-08-31 00:15 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews113 for mac-yosemite
(1.47 MB, application/zip)
2016-08-31 00:25 PDT
,
Build Bot
no flags
Details
Patch
(31.19 KB, patch)
2016-08-31 13:40 PDT
,
Devin Rousso
bburg
: commit-queue-
Details
Formatted Diff
Diff
Patch
(31.46 KB, patch)
2016-09-07 00:11 PDT
,
Devin Rousso
bburg
: review+
Details
Formatted Diff
Diff
Patch
(31.46 KB, patch)
2016-09-08 13:32 PDT
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2016-07-13 13:27:32 PDT
<
rdar://problem/27330833
>
Devin Rousso
Comment 2
2016-08-23 16:59:33 PDT
I just tried this, and it seems like the issue is with when the selector is still focused. Once the contenteditable element is blurred, Cmd+Z works just fine.
Nikita Vasilyev
Comment 3
2016-08-24 12:08:18 PDT
(In reply to
comment #2
)
> I just tried this, and it seems like the issue is with when the selector is > still focused. Once the contenteditable element is blurred, Cmd+Z works > just fine.
Yes, correct. However, this shouldn't be the case. Cmd-Z should still work while I'm focused on a selector.
Devin Rousso
Comment 4
2016-08-24 22:46:31 PDT
Created
attachment 286945
[details]
[Patch] WIP I think that this works, but I have yet to test it extensively yet. Any feedback is welcome now, though.
Build Bot
Comment 5
2016-08-31 00:12:05 PDT
Comment on
attachment 286945
[details]
[Patch] WIP
Attachment 286945
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
http://webkit-queues.webkit.org/results/1978828
New failing tests: inspector/css/generate-css-rule-string.html inspector/css/stylesheet-with-mutations.html
Build Bot
Comment 6
2016-08-31 00:12:08 PDT
Created
attachment 287498
[details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Build Bot
Comment 7
2016-08-31 00:15:33 PDT
Comment on
attachment 286945
[details]
[Patch] WIP
Attachment 286945
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.webkit.org/results/1978862
New failing tests: inspector/css/generate-css-rule-string.html inspector/css/stylesheet-with-mutations.html
Build Bot
Comment 8
2016-08-31 00:15:36 PDT
Created
attachment 287499
[details]
Archive of layout-test-results from ews102 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Build Bot
Comment 9
2016-08-31 00:25:25 PDT
Comment on
attachment 286945
[details]
[Patch] WIP
Attachment 286945
[details]
did not pass mac-debug-ews (mac): Output:
http://webkit-queues.webkit.org/results/1978866
New failing tests: inspector/css/generate-css-rule-string.html inspector/css/stylesheet-with-mutations.html
Build Bot
Comment 10
2016-08-31 00:25:28 PDT
Created
attachment 287500
[details]
Archive of layout-test-results from ews113 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Devin Rousso
Comment 11
2016-08-31 13:40:10 PDT
Created
attachment 287536
[details]
Patch
Blaze Burg
Comment 12
2016-09-06 17:33:26 PDT
Comment on
attachment 287536
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=287536&action=review
Some questions.
> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:70 > refresh()
Personally, I find it nice to append 'Soon' to things that happen asynchronously and return a promise. So: refreshSoon().then(...)
> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:76 > + this._refreshPending = new WebInspector.WrappedPromise;
This variable name was better suited when it was a boolean flag. Now I think this._pendingRefresh[Task] is more appropriate. We are still experimenting with clear naming rules for retained promises.
> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:246 > + let refreshPendingPromise = this._refreshPending.promise;
I don't think there's any reason to create this._refreshPending prior to this point. Is there any way someone could access it re-entrantly or before this method returns? If it's assigned down here, then we can do: let refreshTask = this._pendingRefreshTask = Promise.all(...) .then(() => { this._pendingRefreshTask = null; }); return refreshTask; There's no need to create refreshPendingPromise as a separate promise, this happens implicitly when chaining with .then().
> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:-432 > - var selector = match[1].trim();
Please do: let [selector, value] = match; this._style.nodeStyles.changeRule(this._style.ownerRule, selector.trim(), value) Unless this is in a tight loop the perf impact should be nil, and it's much easier to read the changeRule arguments.
> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:-528 > - if (this._hasInvalidSelector) {
Ok, removing this makes sense in theory, but if the invalid selector causes a syntax error, then how much of the stylesheet will stop working? If it's more than just the single rule, then I'm not ok with this change.
> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:599 > + if (event.keyCode === WebInspector.KeyboardShortcut.Key.Enter.keyCode) {
Nice!
> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:626 > + // Ensures that input does not scroll with added characters.
o_O ? Please explain this a bit more in the change log or something.
Devin Rousso
Comment 13
2016-09-07 00:11:35 PDT
Created
attachment 288113
[details]
Patch (In reply to
comment #12
)
> > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:70 > > refresh() > > Personally, I find it nice to append 'Soon' to things that happen asynchronously and return a promise. So: refreshSoon().then(...)
Not sure how I feel about this. Even though the CSSAgent callbacks are asynchronous, the calling function is initiated immediately, which is sort of synchronous. My distinction is similar to how WI.View does needsLayout vs. updateLayout. Do we consider anything that deals with Promises or Inspector backend code asynchronous?
> > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:-528 > > - if (this._hasInvalidSelector) { > > Ok, removing this makes sense in theory, but if the invalid selector causes a syntax error, then how much of the stylesheet will stop working? If it's more than just the single rule, then I'm not ok with this change.
When I tested this (such as by adding "{"), it would simply revert the invalid selector to the previous value. I was never a huge fan of allowing temporary invalid input to begin with (and it would actually be reverted in the same fashion on the next refresh anyways), so I thought it was a benefit.
Blaze Burg
Comment 14
2016-09-08 12:09:27 PDT
Comment on
attachment 288113
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=288113&action=review
r=me
> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:246 > + let refreshTask = this._pendingRefreshTask = Promise.all([fetchedMatchedStylesPromise, fetchedInlineStylesPromise, fetchedComputedStylesPromise]).then(() => {
I don't think we need this local variable. (I errantly suggested it.)
> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:631 > + this._selectorElement.append(String.fromCharCode(event.keyCode));
It seems unlikely to me that this will work well with IME input. That said, I have never seen a non-ascii CSS selector, so this is probably moot.
Devin Rousso
Comment 15
2016-09-08 13:31:41 PDT
Comment on
attachment 288113
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=288113&action=review
>> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:631 >> + this._selectorElement.append(String.fromCharCode(event.keyCode)); > > It seems unlikely to me that this will work well with IME input. That said, I have never seen a non-ascii CSS selector, so this is probably moot.
I tried using the macOS Emoji & Symbols editor and it worked just fine. Is there any other input that I could try?
Devin Rousso
Comment 16
2016-09-08 13:32:01 PDT
Created
attachment 288315
[details]
Patch
Blaze Burg
Comment 17
2016-09-09 10:09:45 PDT
(In reply to
comment #15
)
> Comment on
attachment 288113
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=288113&action=review
> > >> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:631 > >> + this._selectorElement.append(String.fromCharCode(event.keyCode)); > > > > It seems unlikely to me that this will work well with IME input. That said, I have never seen a non-ascii CSS selector, so this is probably moot. > > I tried using the macOS Emoji & Symbols editor and it worked just fine. Is > there any other input that I could try?
No
Blaze Burg
Comment 18
2016-09-09 10:10:17 PDT
I think this is ready to land, no?
WebKit Commit Bot
Comment 19
2016-09-09 11:27:15 PDT
Comment on
attachment 288315
[details]
Patch Clearing flags on attachment: 288315 Committed
r205754
: <
http://trac.webkit.org/changeset/205754
>
WebKit Commit Bot
Comment 20
2016-09-09 11:27:22 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