Bug 159734 - Web Inspector: Command-Z doesn't work when editing CSS selectors in Styles sidebar
Summary: Web Inspector: Command-Z doesn't work when editing CSS selectors in Styles si...
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: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-07-13 13:27 PDT by Nikita Vasilyev
Modified: 2016-09-09 11:27 PDT (History)
10 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 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".
Comment 1 Radar WebKit Bug Importer 2016-07-13 13:27:32 PDT
<rdar://problem/27330833>
Comment 2 Devin Rousso 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.
Comment 3 Nikita Vasilyev 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.
Comment 4 Devin Rousso 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.
Comment 5 Build Bot 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
Comment 6 Build Bot 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
Comment 7 Build Bot 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
Comment 8 Build Bot 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
Comment 9 Build Bot 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
Comment 10 Build Bot 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
Comment 11 Devin Rousso 2016-08-31 13:40:10 PDT
Created attachment 287536 [details]
Patch
Comment 12 Brian Burg 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.
Comment 13 Devin Rousso 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.
Comment 14 Brian Burg 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.
Comment 15 Devin Rousso 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?
Comment 16 Devin Rousso 2016-09-08 13:32:01 PDT
Created attachment 288315 [details]
Patch
Comment 17 Brian Burg 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
Comment 18 Brian Burg 2016-09-09 10:10:17 PDT
I think this is ready to land, no?
Comment 19 WebKit Commit Bot 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>
Comment 20 WebKit Commit Bot 2016-09-09 11:27:22 PDT
All reviewed patches have been landed.  Closing bug.