Bug 101591 – Web Inspector: Ctrl+a in the network panel should select resource content, not the entire panel

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Andrey Lushnikov]

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.

[Andrey Lushnikov]

Created attachment 173019 [details]
Patch

[Andrey Adaikin]

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.

[Alexander Pavlov (apavlov)]

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"

[Vsevolod Vlasov]

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.

[Vsevolod Vlasov]

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.

[Alexander Pavlov (apavlov)]

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.

[Vsevolod Vlasov]

(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! :)

[Andrey Lushnikov]

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

[Alexander Pavlov (apavlov)]

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

[Andrey Lushnikov]

Created attachment 173223 [details]
Patch

[Vsevolod Vlasov]

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.

[Andrey Lushnikov]

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.

[Andrey Lushnikov]

Created attachment 173230 [details]
Patch

[WebKit Review Bot]

Comment on attachment 173230 [details]
Patch

Clearing flags on attachment: 173230

Committed r134030: <http://trac.webkit.org/changeset/134030>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.