Bug 159734

Summary:

Web Inspector: Command-Z doesn't work when editing CSS selectors in Styles sidebar

Product:

WebKit

Reporter:

Nikita Vasilyev <nvasilyev>

Component:

Web Inspector

Assignee:

Devin Rousso <hi>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bburg, buildbot, commit-queue, hi, joepeck, mattbaker, nvasilyev, rniwa, timothy, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
[Patch] WIP	none
Archive of layout-test-results from ews107 for mac-yosemite-wk2	none
Archive of layout-test-results from ews102 for mac-yosemite	none
Archive of layout-test-results from ews113 for mac-yosemite	none
Patch	bburg: commit-queue-
Patch	bburg: review+
Patch	none

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.