Bug 101591 - Web Inspector: Ctrl+a in the network panel should select resource content, not the entire panel
Summary: Web Inspector: Ctrl+a in the network panel should select resource content, no...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Andrey Lushnikov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-08 04:58 PST by Andrey Lushnikov
Modified: 2012-11-09 01:20 PST (History)
10 users (show)

See Also:


Attachments
Patch (3.69 KB, patch)
2012-11-08 05:11 PST, Andrey Lushnikov
no flags Details | Formatted Diff | Diff
Patch (3.88 KB, patch)
2012-11-09 00:17 PST, Andrey Lushnikov
no flags Details | Formatted Diff | Diff
Patch (4.45 KB, patch)
2012-11-09 00:50 PST, Andrey Lushnikov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrey Lushnikov 2012-11-08 04:58:34 PST
downstream: http://code.google.com/p/chromium/issues/detail?id=73644

ability to copy clean response content to clipboard as it was in chrome 8. Actually, in "Resources" tab I can hit ctrl+a to copy content of css/js/html. But same operation in network tab(if we need xhr content) works not the same - it's selects content + line numbers + rows, so we get lots of unnecessary stuff in clipboard and it's problem to just paste output to text editor to inspect that was wrong.
Comment 1 Andrey Lushnikov 2012-11-08 05:11:48 PST
Created attachment 173019 [details]
Patch
Comment 2 Andrey Adaikin 2012-11-08 05:42:21 PST
I believe this can be done with pure CSS using -webkit-user-select: none; on the sidebar elements. I just played quickly with DevTools opened on DevTools, and it seemed working for me without any JS code.
Comment 3 Alexander Pavlov (apavlov) 2012-11-08 05:47:26 PST
Comment on attachment 173019 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Web Inspector: Ctrl+a in the network panel should select resource content, not the entire panel

Capital 'A'

> Source/WebCore/ChangeLog:8
> +        Intercepts Ctrl-a event in DefaultTextEditor to select resource content

Intercepts -> Intercept (i.e. "what we do by this patch").

Also, 'a' should be consistently capitalized across the patch, as is usual with the shortcuts notation (see the Shortcuts help screen).

> Source/WebCore/ChangeLog:12
> +        (WebInspector.DefaultTextEditor.prototype._handleKeyDown): intercept Ctrl-A even for readonly fields

Comments should be sentence-capitalized ("Intercept...")

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1492
> +        window.getSelection().addRange(range);

You should probably invoke
window.getSelection().removeAllRanges();
before this line, so that an existing selection will not interfere with your override.

> Source/WebCore/inspector/front-end/KeyboardShortcut.js:179
> +WebInspector.KeyboardShortcut.SELECT_ALL = WebInspector.KeyboardShortcut.makeKey("a", WebInspector.KeyboardShortcut.Modifiers.CtrlOrMeta);

Constants are camel-cased in WebKit: "...SelectAll"
Comment 4 Vsevolod Vlasov 2012-11-08 05:51:46 PST
Comment on attachment 173019 [details]
Patch

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

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:412
> +        var handleSelectAll = this._mainPanel.handleSelectAll.bind(this._mainPanel);

inline?

> Source/WebCore/inspector/front-end/KeyboardShortcut.js:179
> +WebInspector.KeyboardShortcut.SELECT_ALL = WebInspector.KeyboardShortcut.makeKey("a", WebInspector.KeyboardShortcut.Modifiers.CtrlOrMeta);

SelectAll per webkit constants naming convention.
Comment 5 Vsevolod Vlasov 2012-11-08 05:55:24 PST
Comment on attachment 173019 [details]
Patch

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

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1492
>> +        window.getSelection().addRange(range);
> 
> You should probably invoke
> window.getSelection().removeAllRanges();
> before this line, so that an existing selection will not interfere with your override.

You'd better do this.setSelection() instead.
Comment 6 Alexander Pavlov (apavlov) 2012-11-08 06:12:18 PST
Comment on attachment 173019 [details]
Patch

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

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:412
>> +        var handleSelectAll = this._mainPanel.handleSelectAll.bind(this._mainPanel);
> 
> inline?

I though about this as well but handleUndo is not reused either, so I figured this might be good for consistency.

>>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1492
>>> +        window.getSelection().addRange(range);
>> 
>> You should probably invoke
>> window.getSelection().removeAllRanges();
>> before this line, so that an existing selection will not interfere with your override.
> 
> You'd better do this.setSelection() instead.

That's right, I missed the fact that this is an editor.
Comment 7 Vsevolod Vlasov 2012-11-08 07:45:28 PST
(In reply to comment #6)
> (From update of attachment 173019 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173019&action=review
> 
> >> Source/WebCore/inspector/front-end/DefaultTextEditor.js:412
> >> +        var handleSelectAll = this._mainPanel.handleSelectAll.bind(this._mainPanel);
> > 
> > inline?
> 
> I though about this as well but handleUndo is not reused either, so I figured this might be good for consistency.
Let's inline all of them for consistency then! :)
Comment 8 Andrey Lushnikov 2012-11-08 23:50:50 PST
Comment on attachment 173019 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        Web Inspector: Ctrl+a in the network panel should select resource content, not the entire panel
> 
> Capital 'A'

fixed

>> Source/WebCore/ChangeLog:8
>> +        Intercepts Ctrl-a event in DefaultTextEditor to select resource content
> 
> Intercepts -> Intercept (i.e. "what we do by this patch").
> 
> Also, 'a' should be consistently capitalized across the patch, as is usual with the shortcuts notation (see the Shortcuts help screen).

fixed

>> Source/WebCore/ChangeLog:12
>> +        (WebInspector.DefaultTextEditor.prototype._handleKeyDown): intercept Ctrl-A even for readonly fields
> 
> Comments should be sentence-capitalized ("Intercept...")

fixed

>>>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:412
>>>> +        var handleSelectAll = this._mainPanel.handleSelectAll.bind(this._mainPanel);
>>> 
>>> inline?
>> 
>> I though about this as well but handleUndo is not reused either, so I figured this might be good for consistency.
> 
> Let's inline all of them for consistency then! :)

But, handleRedo is reused; are you guys ok if I inline two out of three?

>>> Source/WebCore/inspector/front-end/KeyboardShortcut.js:179
>>> +WebInspector.KeyboardShortcut.SELECT_ALL = WebInspector.KeyboardShortcut.makeKey("a", WebInspector.KeyboardShortcut.Modifiers.CtrlOrMeta);
>> 
>> Constants are camel-cased in WebKit: "...SelectAll"
> 
> SelectAll per webkit constants naming convention.

fixed
Comment 9 Alexander Pavlov (apavlov) 2012-11-08 23:59:15 PST
Comment on attachment 173019 [details]
Patch

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

>>>>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:412
>>>>> +        var handleSelectAll = this._mainPanel.handleSelectAll.bind(this._mainPanel);
>>>> 
>>>> inline?
>>> 
>>> I though about this as well but handleUndo is not reused either, so I figured this might be good for consistency.
>> 
>> Let's inline all of them for consistency then! :)
> 
> But, handleRedo is reused; are you guys ok if I inline two out of three?

Sure. We discussed this point with Seva yesterday :)
Comment 10 Andrey Lushnikov 2012-11-09 00:17:26 PST
Created attachment 173223 [details]
Patch
Comment 11 Vsevolod Vlasov 2012-11-09 00:30:09 PST
Comment on attachment 173223 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        * inspector/front-end/KeyboardShortcut.js: Added SELECT_ALL constant for Ctrl+A combination

SelectAll

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:412
>          this._shortcuts[WebInspector.KeyboardShortcut.makeKey("z", modifiers.Shift | modifiers.CtrlOrMeta)] = handleRedo;

I would group together all the code dealing with redo here.

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:433
> +        if (this.readOnly() && shortcutKey !== WebInspector.KeyboardShortcut.SelectAll)

This is odd. We would need to add such a check for each shortcut supported in read only mode that we add.
I'd rather move readOnly check to handlers.
Comment 12 Andrey Lushnikov 2012-11-09 00:49:55 PST
Comment on attachment 173223 [details]
Patch

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

>> Source/WebCore/ChangeLog:14
>> +        * inspector/front-end/KeyboardShortcut.js: Added SELECT_ALL constant for Ctrl+A combination
> 
> SelectAll

fixed

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:412
>>          this._shortcuts[WebInspector.KeyboardShortcut.makeKey("z", modifiers.Shift | modifiers.CtrlOrMeta)] = handleRedo;
> 
> I would group together all the code dealing with redo here.

fixed!

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:433
>> +        if (this.readOnly() && shortcutKey !== WebInspector.KeyboardShortcut.SelectAll)
> 
> This is odd. We would need to add such a check for each shortcut supported in read only mode that we add.
> I'd rather move readOnly check to handlers.

ok, fixed that.
Comment 13 Andrey Lushnikov 2012-11-09 00:50:14 PST
Created attachment 173230 [details]
Patch
Comment 14 WebKit Review Bot 2012-11-09 01:20:31 PST
Comment on attachment 173230 [details]
Patch

Clearing flags on attachment: 173230

Committed r134030: <http://trac.webkit.org/changeset/134030>
Comment 15 WebKit Review Bot 2012-11-09 01:20:35 PST
All reviewed patches have been landed.  Closing bug.