Closed Bug 1268447 Opened 3 years ago Closed 3 years ago

Convert web console to use new key shortcut API

Categories

(DevTools :: Console, defect, P1)

defect

Tracking

(firefox49 verified)

VERIFIED FIXED
Firefox 49
Iteration:
49.3 - Jun 6
Tracking Status
firefox49 --- verified

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

(Whiteboard: [devtools-html])

Attachments

(1 file, 4 obsolete files)

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
Summary: Convert eye dropper to use new key shortcut API → Convert web console to use new key shortcut API
Whiteboard: [devtools-html][triage]
Flags: qe-verify?
Priority: -- → P2
Whiteboard: [devtools-html][triage] → [devtools-html]
Attached patch patch v1 (obsolete) — Splinter Review
WIP, needs to convert zoom keys, but waiting for toolbox review first.
Assignee: nobody → poirot.alex
Status: NEW → ASSIGNED
Iteration: --- → 49.2 - May 23
Priority: P2 → P1
Flags: qe-verify? → qe-verify+
QA Contact: alexandra.lucinet
Iteration: 49.2 - May 23 → 49.3 - Jun 6
Attached patch patch v2 (obsolete) — Splinter Review
Here is most keys translated to use key shortcuts module.
Attachment #8756908 - Flags: review?(bgrinstead)
Attachment #8750792 - Attachment is obsolete: true
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.
Attachment #8756909 - Flags: review?(bgrinstead)
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
Attachment #8756908 - Flags: review?(bgrinstead) → review+
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
Attachment #8756909 - Flags: review?(bgrinstead) → review+
Attached patch fix tests (obsolete) — Splinter Review
Here is some tweaks required for tests and a small eslint fix.
(looks like Services is magically inserted into global scope...)
Attachment #8757981 - Flags: review?(bgrinstead)
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?
Attachment #8757981 - Flags: review?(bgrinstead) → review?(jryans)
Attachment #8757981 - Flags: review?(jryans) → review+
Attached patch patch v3Splinter Review
Rebased and merged patch.

Waiting for try results on Mac and Win.
Attachment #8758358 - Flags: review+
Attachment #8756908 - Attachment is obsolete: true
Attachment #8756909 - Attachment is obsolete: true
Attachment #8757981 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/d39456c7ccf1
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
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
QA Whiteboard: [qe-dthtml]
Flags: needinfo?(poirot.alex)
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.
Flags: needinfo?(poirot.alex)
(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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1279459
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.