Bug 151157 – Web Inspector: yank/kill shortcuts (CTRL+Y, K) don't work in Console / QuickConsole

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

SUMMARY:

Yanking and killing doesn't work correctly in Web Inspector console. In particular, it seems Kill (CTRL+K) will clear the line but not put the text into the pasteboard. You can kill text from a search box and yank it into the console, and it works correctly. I don't know if this is inspector accidentally preventing default, or an editing bug.

STEPS TO REPRODUCE:

* Open the inspector
* Type some text in the console at the bottom
* Place caret at start of line (CTRL+A)
* Kill the text (CTRL+K)
* Yank the text (CTRL+Y)

EXPECTED:

The same killed text should be placed back into the console.

ACTUAL:

Any previously-killed text is yanked into place, or nothing if the kill pasteboard is empty.

NOTES:

I don't know if this ever worked.

[Radar WebKit Bug Importer]

<rdar://problem/23506965>

[Blaze Burg]

To clarify, yank/kill doesn't use a system pasteboard, it's implemented by editor commands executeYank etc in EditorComand.cpp.

[Radar WebKit Bug Importer]

<rdar://problem/23507024>

[Blaze Burg]

Nikita suspects that this is a CodeMirror issue. We may simply not have those shortcut bindings implemented. We need to expose kill/yank in InspectorFrontendHost to have a consistent yank/kill experience across Web Inspector's native controls (<input type=search>) and CodeMirror instances.

[Blaze Burg]

(In reply to comment #4)
> Nikita suspects that this is a CodeMirror issue. We may simply not have
> those shortcut bindings implemented. We need to expose kill/yank in
> InspectorFrontendHost to have a consistent yank/kill experience across Web
> Inspector's native controls (<input type=search>) and CodeMirror instances.

CodeMirror implements CTRL + K entirely within the editor by removing some text from the editor. It doesn't handle CTRL + Y on Mac (it's redo on Windows). Since WebCore editing didn't handle the kill, it yanks old text if it had any but never gets kills done in CodeMirror. I think we can add a host function to tell WebCore about the killed text.

[Blaze Burg]

I have a fix in the works, going to try to write a test for the non-CodeMirror parts tomorrow.

[Blaze Burg]

Created attachment 265546 [details]
Proposed Fix

[Blaze Burg]

Ryosuke, could you take a look at the changes to WebCore::Editor? I believe that the changes should reflect the refactor we discussed, but let me know if anything is wrong.

Joe, you are probably best to look at the Inspector side, since I already explained most of the bug to you.

[Darin Adler]

Comment on attachment 265546 [details]
Proposed Fix

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

> Source/WebCore/ChangeLog:21
> +        No new tests, because we need to use both InspectorFrontendHost
> +        and TestRunner.execCommand, but the latter is not available in
> +        the inspector context where we would need to simulate a kill.

Isn’t that fixable?

> Source/WebCore/ChangeLog:22
> +        Editor refactorings are covered by existing tests.

I do not think that we have kill/yank tests that sufficiently cover the refactoring. I can only find a single test calling the "yank" command; seems unlikely that this is sufficient coverage. For example, I believe there is code here that runs only when the delete key is pressed and other code that runs only when the forward delete key is pressed.

> Source/WebCore/editing/Editor.h:339
> +    void addRangeToKillRing(Range*);

Since we are touching this function, I suggest making it take a Range& rather than a Range* as we would have done in new code.

> Source/WebCore/editing/Editor.h:341
> +    void addTextToKillRing(const String&, bool shouldStartNewSequence = false);
> +    void setStartNewKillRingSequence(bool);

I don’t think we need both a “shouldStartNewSequence” boolean argument *and* a setStartNewKillRingSequence function. We should have the caller use setStartNewKillRingSequence instead of passing a boolean.

> Source/WebCore/inspector/InspectorFrontendHost.cpp:272
> +    m_frontendPage->focusController().focusedOrMainFrame().editor().addTextToKillRing(text, shouldStartNewSequence);

Given that this class has a disconnectClient function, I’m surprised that functions like this don’t check m_frontendPage for null.

[Blaze Burg]

Comment on attachment 265546 [details]
Proposed Fix

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

>> Source/WebCore/ChangeLog:21
>> +        the inspector context where we would need to simulate a kill.
> 
> Isn’t that fixable?

It would require very significant refactoring of the test infrastructure, and we haven't needed it for anything else so far. My understanding is that EventSender, TestRunner and friends are installed in a WebProcess through the InjectedBundle provided by WebKitTestRunner. But, the Inspector context is its own WKWebView, which doesn't really have an injected bundle.

When it becomes possible to write UI tests via via UIScriptController or WebDriver, then this point will be moot; in that case, we can simulate raw user keystrokes into the inspector (ctrl-k, ctrl-y, etc) without using execCommand.

>> Source/WebCore/ChangeLog:22
>> +        Editor refactorings are covered by existing tests.
> 
> I do not think that we have kill/yank tests that sufficiently cover the refactoring. I can only find a single test calling the "yank" command; seems unlikely that this is sufficient coverage. For example, I believe there is code here that runs only when the delete key is pressed and other code that runs only when the forward delete key is pressed.

I agree that WebKit has poor coverage of editor commands that can lead to a kill in the kill ring, such as {backward,forward} delete {word, paragraph, line}. For the commands that have key bindings in Safari, I manually verified that there was no behavior change.

Since kills are only ever appended, it was my judgement that a test for just one of these code paths is sufficient for coverage of the kill ring's append / yank behavior. We seem to have the following tests specifically to yank / verify what's in the kill ring:

* LayoutTests/editing/pasteboard/emacs-ctrl-a-k-y.html
* LayoutTests/editing/pasteboard/emacs-ctrl-k-with-move.html

Other deletion-related editing tests could be improved to assert the contents of the kill ring, but I propose we make that a separate task so that this patch doesn't rot.

>> Source/WebCore/editing/Editor.h:339
>> +    void addRangeToKillRing(Range*);
> 
> Since we are touching this function, I suggest making it take a Range& rather than a Range* as we would have done in new code.

I was inclined to agree, but it seems there's no guarantee that the range (coming from RefPtr) is actually non-null. plainText() takes a pointer and handles the null case, so we would null-check it then immediately pass a pointer again. Someone with more background in TextIterator should do that cleanup.

>> Source/WebCore/editing/Editor.h:341
>> +    void setStartNewKillRingSequence(bool);
> 
> I don’t think we need both a “shouldStartNewSequence” boolean argument *and* a setStartNewKillRingSequence function. We should have the caller use setStartNewKillRingSequence instead of passing a boolean.

That's a good point. Will fix.

>> Source/WebCore/inspector/InspectorFrontendHost.cpp:272
>> +    m_frontendPage->focusController().focusedOrMainFrame().editor().addTextToKillRing(text, shouldStartNewSequence);
> 
> Given that this class has a disconnectClient function, I’m surprised that functions like this don’t check m_frontendPage for null.

The Page represented by m_frontendPage always exists when these methods are called by frontend code, because that is the page which is making the call to killText. 

However, the pointer could have been nulled out for other reasons, so I agree that these should all be null-checking m_frontendPage or m_client.

[Blaze Burg]

Comment on attachment 265546 [details]
Proposed Fix

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

>>> Source/WebCore/ChangeLog:22
>>> +        Editor refactorings are covered by existing tests.
>> 
>> I do not think that we have kill/yank tests that sufficiently cover the refactoring. I can only find a single test calling the "yank" command; seems unlikely that this is sufficient coverage. For example, I believe there is code here that runs only when the delete key is pressed and other code that runs only when the forward delete key is pressed.
> 
> I agree that WebKit has poor coverage of editor commands that can lead to a kill in the kill ring, such as {backward,forward} delete {word, paragraph, line}. For the commands that have key bindings in Safari, I manually verified that there was no behavior change.
> 
> Since kills are only ever appended, it was my judgement that a test for just one of these code paths is sufficient for coverage of the kill ring's append / yank behavior. We seem to have the following tests specifically to yank / verify what's in the kill ring:
> 
> * LayoutTests/editing/pasteboard/emacs-ctrl-a-k-y.html
> * LayoutTests/editing/pasteboard/emacs-ctrl-k-with-move.html
> 
> Other deletion-related editing tests could be improved to assert the contents of the kill ring, but I propose we make that a separate task so that this patch doesn't rot.

After further thinking about your comment, I convinced myself i was overlooking something, then I discovered https://bugs.webkit.org/show_bug.cgi?id=151300. So you are totally correct, however I don't think it should be addressed in this patch. I will add a note to the changelog.

> Source/WebCore/ChangeLog:35
> +            Remove the bool 'prepend' argument, since it's always false.

Per discovery in https://bugs.webkit.org/show_bug.cgi?id=151300, I will revert this part of the patch and submit a test case for that code path as part of that bug.

[Blaze Burg]

Created attachment 265884 [details]
Rebased Patch

[Joseph Pecoraro]

Comment on attachment 265884 [details]
Rebased Patch

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

r=me, Sounds good.

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorEditor.js:33
> +        // Set up default controllers that should be present for
> +        // all CodeMirror editor instances.

This comment seems unnecessary.

[WebKit Commit Bot]

Comment on attachment 265884 [details]
Rebased Patch

Clearing flags on attachment: 265884

Committed r192662: <http://trac.webkit.org/changeset/192662>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.