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 InspectorAssignee: 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 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

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 BJ 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 BJ 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 BJ 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 BJ 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.