Convert web console to use new key shortcut API

VERIFIED FIXED in Firefox 49

Status

DevTools
Console
P1
normal
VERIFIED FIXED
2 years ago
10 days ago

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

unspecified
Firefox 49
Dependency tree / graph

Firefox Tracking Flags

(firefox49 verified)

Details

(Whiteboard: [devtools-html])

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

2 years ago
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
(Assignee)

Updated

2 years ago
Summary: Convert eye dropper to use new key shortcut API → Convert web console to use new key shortcut API
(Assignee)

Updated

2 years ago
Whiteboard: [devtools-html][triage]

Updated

2 years ago
Blocks: 1259121
Flags: qe-verify?
Priority: -- → P2
Whiteboard: [devtools-html][triage] → [devtools-html]
(Assignee)

Comment 1

2 years ago
Created attachment 8750792 [details] [diff] [review]
patch v1

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

Updated

2 years ago
Assignee: nobody → poirot.alex
Status: NEW → ASSIGNED
Iteration: --- → 49.2 - May 23
Priority: P2 → P1

Updated

2 years ago
Flags: qe-verify? → qe-verify+
QA Contact: alexandra.lucinet

Updated

2 years ago
Iteration: 49.2 - May 23 → 49.3 - Jun 6
(Assignee)

Comment 8

2 years ago
Created attachment 8756908 [details] [diff] [review]
patch v2

Here is most keys translated to use key shortcuts module.
Attachment #8756908 - Flags: review?(bgrinstead)
(Assignee)

Updated

2 years ago
Attachment #8750792 - Attachment is obsolete: true
(Assignee)

Comment 9

2 years ago
Created attachment 8756909 [details] [diff] [review]
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.
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+
(Assignee)

Comment 13

2 years ago
Created attachment 8757981 [details] [diff] [review]
fix tests

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)
(Assignee)

Comment 14

2 years ago
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+
(Assignee)

Comment 16

2 years ago
Created attachment 8758358 [details] [diff] [review]
patch v3

Rebased and merged patch.

Waiting for try results on Mac and Win.
Attachment #8758358 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8756908 - Attachment is obsolete: true
Attachment #8756909 - Attachment is obsolete: true
Attachment #8757981 - Attachment is obsolete: true
(Assignee)

Comment 17

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

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d39456c7ccf1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49

Comment 19

2 years ago
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)
(Assignee)

Comment 20

2 years ago
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)

Comment 21

2 years ago
(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
status-firefox49: fixed → verified
Flags: qe-verify+

Updated

10 days ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.