Bug 120657 – Web Inspector: Fix keyboard shortcuts for other platforms

• Component: Web Inspector
• Status: RESOLVED FIXED

[Marcelo Morais]

The New Web Inspector is arriving in other platforms (Windows, EFL,GTK,...) and the keyboard shortcuts need to be updated to work in these environments because today they are tuned to OS X with ⌘ (Command) key.

[Radar WebKit Bug Importer]

<rdar://problem/14903833>

[Timothy Hatcher]

It would be nice to make this automatic so we don't need to declare multiple shortcuts per platform. Something like Command key auto-maps to Control on non-Mac platforms?

[Joseph Pecoraro]

Yea, I was thinking of WebInspector.KeyboardShortcut.Modifier.CommandOrControl which does expected per platform.

[Marcelo Morais]

(In reply to comment #3)
> Yea, I was thinking of WebInspector.KeyboardShortcut.Modifier.CommandOrControl which does expected per platform.

That's what I want to do, I put EFL specifc in the bug because it need some work on MiniBrowser(WK2) and EWebLauncher(WK2) too :)

[Marcelo Morais]

Sorry, I mean that I am working on EFL browsers too, to test this first.

[Marcelo Morais]

Hello,

In Source/WebInspectorUI/UserInterface/ContentBrowser.js we have this:

...

if (!disableBackForward) {
        this._backKeyboardShortcut = new WebInspector.KeyboardShortcut(WebInspector.KeyboardShortcut.Modifier.Command | WebInspector.KeyboardShortcut.Modifier.Control, WebInspector.KeyboardShortcut.Key.Left, this._backButtonClicked.bind(this));
        this._forwardKeyboardShortcut = new WebInspector.KeyboardShortcut(WebInspector.KeyboardShortcut.Modifier.Command | WebInspector.KeyboardShortcut.Modifier.Control, WebInspector.KeyboardShortcut.Key.Right, this._forwardButtonClicked.bind(this));

...

How could we move this to non-Mac platforms?

I would like some suggestions. :)

[Joseph Pecoraro]

(In reply to comment #6)
> How could we move this to non-Mac platforms?
> 
> I would like some suggestions. :)

The earlier comment was a suggestion!

For systems that want to use Control instead of Command we can add "WebInspector.KeyboardShortcut.Modifier.CommandOrControl" and map that value to the appropriate value ("Command" or "Control") for the platform that is running the frontend (InspectorFrontendHost.platform()).

[Marcelo Morais]

(In reply to comment #7)
> (In reply to comment #6)
> > How could we move this to non-Mac platforms?
> > 
> > I would like some suggestions. :)
> 
> The earlier comment was a suggestion!
> 
> For systems that want to use Control instead of Command we can add "WebInspector.KeyboardShortcut.Modifier.CommandOrControl" and map that value to the appropriate value ("Command" or "Control") for the platform that is running the frontend (InspectorFrontendHost.platform()).

Hi Joseph, I am already following your suggestion :)

I mean suggestions for these specific shortcuts:

Command + Control + left 
and
Command + Control + right


How could we map this to non-Mac platforms?

[Timothy Hatcher]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > How could we move this to non-Mac platforms?
> > > 
> > > I would like some suggestions. :)
> > 
> > The earlier comment was a suggestion!
> > 
> > For systems that want to use Control instead of Command we can add "WebInspector.KeyboardShortcut.Modifier.CommandOrControl" and map that value to the appropriate value ("Command" or "Control") for the platform that is running the frontend (InspectorFrontendHost.platform()).
> 
> Hi Joseph, I am already following your suggestion :)
> 
> I mean suggestions for these specific shortcuts:
> 
> Command + Control + left 
> and
> Command + Control + right
> 
> 
> How could we map this to non-Mac platforms?

I would just do:

WebInspector.KeyboardShortcut.Modifier.CommandOrControl | WebInspector.KeyboardShortcut.Modifier.Control, WebInspector.KeyboardShortcut.Key.Left

Then ignore WebInspector.KeyboardShortcut.Modifier.Control if WebInspector.KeyboardShortcut.Modifier.CommandOrControl becomes Control.

[Marcelo Morais]

Created attachment 212204 [details]
Patch

I followed Timothy's suggestions for the case above.

It works! :)

Tested the patch on Mac and Linux.

[Timothy Hatcher]

Comment on attachment 212204 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/KeyboardShortcut.js:96
> +        return (InspectorFrontendHost.platform() === "mac") ? this.Command : this.Control;

No need for the ().

[Marcelo Morais]

(In reply to comment #11)
> (From update of attachment 212204 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212204&action=review
> 
> > Source/WebInspectorUI/UserInterface/KeyboardShortcut.js:96
> > +        return (InspectorFrontendHost.platform() === "mac") ? this.Command : this.Control;
> 
> No need for the ().

Thanks for the reviewing, should I remove this () then? I can do it quickly :)

[Timothy Hatcher]

Comment on attachment 212204 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/KeyboardShortcut.js:96
>>> +        return (InspectorFrontendHost.platform() === "mac") ? this.Command : this.Control;
>> 
>> No need for the ().
> 
> Thanks for the reviewing, should I remove this () then? I can do it quickly :)

Sure.

[Timothy Hatcher]

Comment on attachment 212204 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/KeyboardShortcut.js:96
>>> +        return (InspectorFrontendHost.platform() === "mac") ? this.Command : this.Control;
>> 
>> No need for the ().
> 
> Thanks for the reviewing, should I remove this () then? I can do it quickly :)

Sure.

[Joseph Pecoraro]

Comment on attachment 212204 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/KeyboardShortcut.js:96
>>> +        return (InspectorFrontendHost.platform() === "mac") ? this.Command : this.Control;
>> 
>> No need for the ().
> 
> Thanks for the reviewing, should I remove this () then? I can do it quickly :)

I don't think we want to run this getter every time someone uses CommandOrControl. I suggest we just set it once. The IFH.platform() is not going to change. How about:

    WebInspector.KeyboardShortcut.Modifier = {
        …
    };
    WebInspector.KeyboardShortcut.Modifier.CommandOrControl = InspectorFrontendHost.platform() === "mac" ? WebInspector.KeyboardShortcut.Modifier.Command : WebInspector.KeyboardShortcut.Modifier.Control;

This only does the work once, ever, and should have the same result.

[Marcelo Morais]

Created attachment 212213 [details]
patch

Removed () like Timothy said above.

[Timothy Hatcher]

Comment on attachment 212213 [details]
patch

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

I fixed this issues and landed as http://trac.webkit.org/changeset/156198.

> Source/WebInspectorUI/ChangeLog:3
> [phone]  Marcelo Morais  <[e-mail]>
> +
> +        2013-09-19  Marcelo Morais  <[e-mail]>

Should be only one header and not double indented.

> Source/WebInspectorUI/UserInterface/InspectorFrontendHostStub.js:147
> +            var os = (navigator.platform.match(/mac|win|linux/i) || ["other"])[0].toLowerCase();
> +            return os;

Can be one line.

> Source/WebInspectorUI/UserInterface/KeyboardShortcut.js:96
> +        return (InspectorFrontendHost.platform === "mac") ? this.Command : this.Control;

platform is a function not a getter. So this is comparing the function as a string to "mac" which will always be false. This should be platform(). Also the () is still there.

[Marcelo Morais]

(In reply to comment #17)
> (From update of attachment 212213 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212213&action=review
> 
> I fixed this issues and landed as http://trac.webkit.org/changeset/156198.
> 
> > Source/WebInspectorUI/ChangeLog:3
> > [phone]  Marcelo Morais  <[e-mail]>
> > +
> > +        2013-09-19  Marcelo Morais  <[e-mail]>
> 
> Should be only one header and not double indented.
> 
> > Source/WebInspectorUI/UserInterface/InspectorFrontendHostStub.js:147
> > +            var os = (navigator.platform.match(/mac|win|linux/i) || ["other"])[0].toLowerCase();
> > +            return os;
> 
> Can be one line.
> 
> > Source/WebInspectorUI/UserInterface/KeyboardShortcut.js:96
> > +        return (InspectorFrontendHost.platform === "mac") ? this.Command : this.Control;
> 
> platform is a function not a getter. So this is comparing the function as a string to "mac" which will always be false. This should be platform(). Also the () is still there.

Thanks Timothy! :)