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: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Attachments: |
|
Description
Nikita Vasilyev
2016-07-13 13:27:12 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. (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. 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.
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 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
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 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
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 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
Created attachment 287536 [details]
Patch
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. 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. 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. 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? Created attachment 288315 [details]
Patch
(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 I think this is ready to land, no? Comment on attachment 288315 [details] Patch Clearing flags on attachment: 288315 Committed r205754: <http://trac.webkit.org/changeset/205754> All reviewed patches have been landed. Closing bug. |