1268447 - Convert web console to use new key shortcut API

Status: VERIFIED FIXED

(DevTools :: Console, defect, P1)

Description

[Alexandre Poirot [:ochameau]]

We need to stop using xul for key shortcuts and instead use the upcoming module from bug 1260493.
http://mxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/webconsole.xul#56

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

WIP, needs to convert zoom keys, but waiting for toolbox review first.

Comment 2

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5c6422cad45

Comment 3

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3fdc1b9db69

Comment 4

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=de96a15403ef

Comment 5

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=79084c0b413b

Comment 6

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fa4779911b5

Comment 7

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=79c006ac4414

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

Here is most keys translated to use key shortcuts module.

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Merge zoom key logic between toolbox and browser console one - v1

A specific patch just for zoom key as it's not as simple.
For now, webconsole has a very specific thing to increase its font size,
which is only enabled in the Browser console.
It seems to be a relic from the past. Something implemented before we had Zoom support over the whole toolbox.
So I thought like we could just unify the browser console and the toolbox behavior.

I have to pull out the Zoom code out of toolbox.js as the browser console
doesn't have a reference to a toolbox object.

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8756908 [details] [diff] [review]
patch v2

Review of attachment 8756908 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!  This is a definite improvement

::: devtools/client/webconsole/webconsole.js
@@ +663,5 @@
>      }
>    },
>  
> +  _initShortcuts: function() {
> +    var shortcuts = new KeyShortcuts({

var -> let

@@ +673,5 @@
> +                   this.filterBox.focus();
> +                   event.preventDefault();
> +                 });
> +
> +    let clearShortcut;

Nit: I'd prefer something like:

    let clearShortcut = system.constants.platform === "macosx" ?
                          l10n.getStr("webconsole.clear.keyOSX") :
                          l10n.getStr("webconsole.clear.key");

::: devtools/client/webconsole/webconsole.xul
@@ -62,5 @@
>      <key key="&fullZoomEnlargeCmd.commandkey2;" command="cmd_fullZoomEnlarge" modifiers="accel"/>
>      <key key="&fullZoomEnlargeCmd.commandkey3;" command="cmd_fullZoomEnlarge" modifiers="accel"/>
>      <key id="key_fullZoomReset" key="&fullZoomResetCmd.commandkey;" command="cmd_fullZoomReset" modifiers="accel"/>
>      <key key="&fullZoomResetCmd.commandkey2;" command="cmd_fullZoomReset" modifiers="accel"/>
> -    <key key="&findCmd.key;" command="cmd_find" modifiers="accel"/>

These entities should be able to be removed from webconsole.dtd

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8756909 [details] [diff] [review]
Merge zoom key logic between toolbox and browser console one - v1

Review of attachment 8756909 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/shared/zoom-keys.js
@@ +31,5 @@
> +  let docShell = window.QueryInterface(Ci.nsIInterfaceRequestor)
> +    .getInterface(Ci.nsIWebNavigation)
> +    .QueryInterface(Ci.nsIDocShell);
> +  let contViewer = docShell.contentViewer;
> +  let zoomValue = parseFloat(Services.prefs.getCharPref(ZOOM_PREF));

I could see potentially wanting different zoom prefs for the browser console and the toolbox, so making this pref an option, but I guess let's not do it unless if we get that request

@@ +70,5 @@
> +  if (zoomIn3) {
> +    shortcuts.on(zoomIn3, zoomIn);
> +  }
> +
> +  shortcuts.on(l10n("toolbox.zoomOut.key"),

Nit: this shouldn't wrap onto two lines

@@ +77,5 @@
> +  if (zoomOut2) {
> +    shortcuts.on(zoomOut2, zoomOut);
> +  }
> +
> +  shortcuts.on(l10n("toolbox.zoomReset.key"),

Nit: this shouldn't wrap onto two lines

::: devtools/client/webconsole/webconsole.xul
@@ -53,2 @@
>    </commandset>
>    <keyset id="consoleKeys">

This keyset could be removed

@@ -53,3 @@
>    </commandset>
>    <keyset id="consoleKeys">
> -    <key id="key_fullZoomReduce" key="&fullZoomReduceCmd.commandkey;" command="cmd_fullZoomReduce" modifiers="accel"/>

These entities can be removed from the dtd

Comment 12

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e082cfa51705

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: fix tests

Here is some tweaks required for tests and a small eslint fix.
(looks like Services is magically inserted into global scope...)

Comment 14

[Alexandre Poirot [:ochameau]]

Comment on attachment 8757981 [details] [diff] [review]
fix tests

Review of attachment 8757981 [details] [diff] [review]:
-----------------------------------------------------------------

Oh, I haven't seen Brian was on PTO,
Ryan, would you be able to review in the meantime?

Comment 15

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c33160ced81

Comment 16

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

Rebased and merged patch.

Waiting for try results on Mac and Win.

Comment 17

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/d39456c7ccf19d98bb20c3de1dd88cb3c9513c55
Bug 1268447 - Convert webconsole to use new key shortcut module. r=bgrins

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d39456c7ccf1

Comment 19

[Ada [:adalucinet]]

Verified with latest Nightly 49.0a1 (from 2016-06-06), across platforms [1].
At a first glance, everything is working as expected based on MDN Keyboard shortcuts document [2]; although found a potential issue → with the following str:
1. Open web console
2. Ctrl + F
3. Click to dock to side/bottom/separate window
4. Ctrl + F
Result: the focus from the web console is either lost (if 'Show in separate window' option is selected) or Find in page is opened (for both dock to side/bottom). Note that this is applicable for the rest of the toolbox-hosted tools.

Alexandre, what's your take on the above? Thanks in advance!


[1] Windows 10 64-bit, Ubuntu 16.04 64-bit and Mac OS X 10.10.5
[2] https://developer.mozilla.org/en-US/docs/Tools/Keyboard_shortcuts#toolbox

Comment 20

[Alexandre Poirot [:ochameau]]

Is it a regression from this patch?
I can reproduce this on nightly and yes, it doesn't seem right.
It is even worse than just Ctrl+F. The focus is lost on the tool, so that you have similar STR if you were just focusing the console input. The focus is going to be lost if you change the host.

Comment 21

[Ada [:adalucinet]]

(In reply to Alexandre Poirot [:ochameau] from comment #20)
> Is it a regression from this patch?
Nope, it's not - also reproducible with Nightly from 2016-05-31, but not with Firefox 47.0RC.
Logged bug 1278889 to have it under the radar. Since this is the only issue found and does not depend on this fix, marking here accordingly.