Bug 161514 – Web Inspector: Change Cmd-D from kill line to selecting next occurrence

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

I often find that when I want to edit the same value in CSS for multiple rules, I try to use Cmd-D to select multiple occurrences (like Sublime Text), but it will delete the line instead.

[Radar WebKit Bug Importer]

<rdar://problem/28125141>

[Nikita Vasilyev]

(In reply to comment #0)
> I often find that when I want to edit the same value in CSS for multiple
> rules, I try to use Cmd-D to select multiple occurrences (like Sublime
> Text), but it will delete the line instead.

Chrome DevTools do the same as Sublime Text.

In Xcode, Command-D does nothing for text and duplicates an item in Interface Builder.
http://stackoverflow.com/questions/10266170/xcode-duplicate-line

[Blaze Burg]

Sounds fine to me. As Nikita said, Cmd-D does not have any system-wide significance. The more cross-platform (emacs binding) way to kill a line is Ctrl-K.

[Devin Rousso]

Created attachment 287711 [details]
Patch

[Blaze Burg]

Comment on attachment 287711 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=287711&action=review

The rebinding is fine but I cannot tell what the other effects of this patch are.

> Source/WebInspectorUI/ChangeLog:3
> +        Web Inspector: Change Cmd-D from kill line to selecting next occurance

Please fix bug title with correct spelling prior to landing.

> Source/WebInspectorUI/UserInterface/External/CodeMirror/sublime.js:581
> +  CodeMirror.normalizeKeyMap(map);

I really wish we had a test that dumped out the normalized key map so we could see concretely what keybindings change when we add a new extension like this. It introduces a ton of new bindings, some of which might conflict (or not).

Do you think you could write such a test?

[Blaze Burg]

Comment on attachment 287711 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=287711&action=review

>> Source/WebInspectorUI/UserInterface/External/CodeMirror/sublime.js:581
>> +  CodeMirror.normalizeKeyMap(map);
> 
> I really wish we had a test that dumped out the normalized key map so we could see concretely what keybindings change when we add a new extension like this. It introduces a ton of new bindings, some of which might conflict (or not).
> 
> Do you think you could write such a test?

EDIT: filed https://bugs.webkit.org/show_bug.cgi?id=161536. This is out of scope for this change since it depends on UI tests probably.

[Joseph Pecoraro]

Comment on attachment 287711 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=287711&action=review

> Source/WebInspectorUI/UserInterface/Main.html:223
> +    <script src="External/CodeMirror/sublime.js"></script>

Please add this to:
Source/WebInspectorUI/Scripts/update-codemirror-resources.rb

So when we update CodeMirror (I tend to use that script) we get any updates to this file as well.

[Devin Rousso]

Created attachment 287812 [details]
Patch

[Build Bot]

Comment on attachment 287812 [details]
Patch

Attachment 287812 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1996419

New failing tests:
fast/dom/HTMLFormElement/document-deactivation-callback-crash.html

[Build Bot]

Created attachment 287820 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Blaze Burg]

Comment on attachment 287812 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=287812&action=review

r=me but please address comments prior to landing.

> Source/WebInspectorUI/UserInterface/Controllers/CodeMirrorTextKillController.js:-49
> -            "Cmd-D": this._handleTextKillCommand.bind(this, "deleteLine", false),

Later in this file, there is a comment that talks about Cmd-D.

            // An entire line was deleted, including newline (Cmd-D).
            if (change.removed[0].length && !change.removed[1].length) 
                killedText = change.removed[0] + "\n";

I think we should just leave this code in and replace 'Cmd-D' in the comment with 'deleteLine'. If we re-add the command as a different keybinding, it's unlikely someone will know that this code needs to be changed, and the actual logic does not depend on the particular keybinding, obviously.

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:587
> +        "Cmd-D": "selectNextOccurrence",

I am not familiar with CodeMirror key maps. Do we inherit any other bindings from sublime.js, or just this one that we requested?

[Devin Rousso]

Created attachment 287876 [details]
Patch

(In reply to comment #11)
> I am not familiar with CodeMirror key maps. Do we inherit any other bindings
> from sublime.js, or just this one that we requested?

AFAICT, it seems like we only inherit that specific binding.

[WebKit Commit Bot]

Comment on attachment 287876 [details]
Patch

Clearing flags on attachment: 287876

Committed r205414: <http://trac.webkit.org/changeset/205414>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.