Bug 107944

Summary: Web Inspector: implmenet Ctrl-Arrow/Ctrl-Backspace in DefaultTextEditor
Product: WebKit Reporter: Andrey Lushnikov <lushnikov>
Component: Web Inspector (Deprecated)Assignee: Andrey Lushnikov <lushnikov>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, fmalita, gyuyoung.kim, keishi, loislo, pfeldman, pmuellr, rakuco, vsevik, web-inspector-bugs, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Andrey Lushnikov 2013-01-25 06:27:02 PST
Support Ctrl-Arrow/Ctrl-Shift-Arrow/Ctrl-Backspace shortcuts in DefaultTextEditor. Current implementation makes them unpredictable and thus unusable.
Comment 1 Andrey Lushnikov 2013-01-25 06:38:13 PST
Created attachment 184740 [details]
Patch
Comment 2 Pavel Feldman 2013-01-25 06:51:51 PST
Comment on attachment 184740 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        and delete words. Their behaviour should be equal to Sublime 2's.

This should be fixed in content editable.
Comment 3 Pavel Feldman 2013-01-25 07:08:24 PST
Comment on attachment 184740 [details]
Patch

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

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:416
> +        this._shortcuts[WebInspector.KeyboardShortcut.makeKey(keys.Left.code, modifiers.CtrlOrMeta)] = this._handleCtrlArrow.bind(this, "left");

Do not affect meta.

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:434
> +            return (char >= 'a' && char <= 'z') ||

what about unicode letters?

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:444
> +            return char === " ";

What about \t ?
Comment 4 Andrey Lushnikov 2013-01-25 07:50:19 PST
Created attachment 184752 [details]
Patch
Comment 5 Andrey Lushnikov 2013-01-25 07:52:19 PST
Created attachment 184754 [details]
Patch
Comment 6 Pavel Feldman 2013-01-28 01:05:35 PST
Comment on attachment 184754 [details]
Patch

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

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:417
> +        this._shortcuts[WebInspector.KeyboardShortcut.makeKey(keys.Right.code, modifiers.Ctrl)] = this._handleCtrlArrow.bind(this, "right");

It would be great if we could extract these into a common 'controller' subclass (located in the same file).

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:434
> +            var charCode = char.charCodeAt(0);

lets use string comparizon

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:454
> +        if (column === -1 && direction === "left") {

growRangeLeft/Right
Comment 7 Andrey Lushnikov 2013-01-29 06:00:48 PST
Created attachment 185229 [details]
Patch
Comment 8 Andrey Lushnikov 2013-01-29 06:01:54 PST
Comment on attachment 184754 [details]
Patch

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

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:417
>> +        this._shortcuts[WebInspector.KeyboardShortcut.makeKey(keys.Right.code, modifiers.Ctrl)] = this._handleCtrlArrow.bind(this, "right");
> 
> It would be great if we could extract these into a common 'controller' subclass (located in the same file).

fixed.

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:434
>> +            var charCode = char.charCodeAt(0);
> 
> lets use string comparizon

fixed.

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:454
>> +        if (column === -1 && direction === "left") {
> 
> growRangeLeft/Right

these methods are not applicable here, because they do different thing.
Comment 9 WebKit Review Bot 2013-01-30 03:54:37 PST
Comment on attachment 185229 [details]
Patch

Clearing flags on attachment: 185229

Committed r141245: <http://trac.webkit.org/changeset/141245>
Comment 10 WebKit Review Bot 2013-01-30 03:54:43 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Florin Malita 2013-01-30 09:26:20 PST
(In reply to comment #9)
> (From update of attachment 185229 [details])
> Clearing flags on attachment: 185229
> 
> Committed r141245: <http://trac.webkit.org/changeset/141245>

This seems to be causing consistent timeouts for text-editor-ctrl-movements.html:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=inspector%2Feditor%2Ftext-editor-ctrl-movements.html

Can you please take a look?
Comment 12 Florin Malita 2013-01-30 11:19:12 PST
http://trac.webkit.org/changeset/141286
Comment 13 Andrey Lushnikov 2013-01-31 01:19:16 PST
Thanks for letting me know!

Created a bug to fix this: https://bugs.webkit.org/show_bug.cgi?id=108440