892244 - Ctrl+[ / Ctrl+] to switch devtools tabs

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Heather Arthur [:harth]]

We need a way to cycle through the devtools tabs when the devtools window is focused using the keyboard. Chrome uses Ctrl+[/] (Cmd+[/]) to go the previous and next tab.

Caveat to Ctrl+[/] (Cmd+[/]) is that this is hard to execute on a German keyboard, according to Dao.

Comment 1

[Girish Sharma [:Optimizer]]

Attachment: patch v1.0

This patch only contains the tab switching mechanism.
try at : https://tbpl.mozilla.org/?tree=Try&rev=71b04c3b0e61

Comment 2

[Heather Arthur [:harth]]

Comment on attachment 776569 [details] [diff] [review]
patch v1.0

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

Thanks Girish, few comments:

::: browser/devtools/framework/test/browser_toolbox_tabsswitch_shortcuts.js
@@ +65,5 @@
> +    EventUtils.synthesizeKey(key, modifiers, toolbox.doc.defaultView);
> +  }
> +}
> +
> +function selectCB(event, id) {

Nit: Don't see a "cb" convention in our code anywhere, onSelect()?

::: browser/devtools/framework/toolbox.js
@@ +234,5 @@
>     * Adds the keys and commands to the Toolbox Window in window mode.
>     */
>    _addKeysToWindow: function TBOX__addKeysToWindow() {
> +    this.doc.defaultView.selectNextTool = this.selectNextTool.bind(this);
> +    this.doc.defaultView.selectPreviousTool = this.selectPreviousTool.bind(this);

instead of this, why not just set the command listener dynamically?

@@ +547,5 @@
> +   */
> +  selectNextTool: function TBOX_selectNextTool() {
> +    let selected = this.doc.querySelector(".devtools-tab[selected]");
> +    let next = selected.nextSibling || selected.parentNode.firstChild;
> +    let tool = next.id.replace("toolbox-tab-", "");

Let's do this more right. Add a "data-toolid" attribute to each tab, and query that.

Comment 3

[Girish Sharma [:Optimizer]]

(In reply to Heather Arthur [:harth] from comment #2)
> Comment on attachment 776569 [details] [diff] [review]
> patch v1.0
> 
> Review of attachment 776569 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks Girish, few comments:
> 
> ::: browser/devtools/framework/test/browser_toolbox_tabsswitch_shortcuts.js
> @@ +65,5 @@
> > +    EventUtils.synthesizeKey(key, modifiers, toolbox.doc.defaultView);
> > +  }
> > +}
> > +
> > +function selectCB(event, id) {
> 
> Nit: Don't see a "cb" convention in our code anywhere, onSelect()?

Agreed.

> ::: browser/devtools/framework/toolbox.js
> @@ +234,5 @@
> >     * Adds the keys and commands to the Toolbox Window in window mode.
> >     */
> >    _addKeysToWindow: function TBOX__addKeysToWindow() {
> > +    this.doc.defaultView.selectNextTool = this.selectNextTool.bind(this);
> > +    this.doc.defaultView.selectPreviousTool = this.selectPreviousTool.bind(this);
> 
> instead of this, why not just set the command listener dynamically?

Then we will have to add a 'oncommand="void(0);"' to the <key> elements so that its command can be listened via JS. Also since there was no use of the event argument or any other argument, I thought this might be easier.


> @@ +547,5 @@
> > +   */
> > +  selectNextTool: function TBOX_selectNextTool() {
> > +    let selected = this.doc.querySelector(".devtools-tab[selected]");
> > +    let next = selected.nextSibling || selected.parentNode.firstChild;
> > +    let tool = next.id.replace("toolbox-tab-", "");
> 
> Let's do this more right. Add a "data-toolid" attribute to each tab, and
> query that.

Gotcha.

Comment 4

[Girish Sharma [:Optimizer]]

Attachment: patch v1.1

Addressed review comments.

new try : https://tbpl.mozilla.org/?tree=Try&rev=71cf8af140a0

Comment 5

[Heather Arthur [:harth]]

(In reply to Girish Sharma [:Optimizer] from comment #3)
> (In reply to Heather Arthur [:harth] from comment #2)
> > Comment on attachment 776569 [details] [diff] [review]
> > patch v1.0
> > 
> > Review of attachment 776569 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Thanks Girish, few comments:
> > 
> > ::: browser/devtools/framework/test/browser_toolbox_tabsswitch_shortcuts.js
> > @@ +65,5 @@
> > > +    EventUtils.synthesizeKey(key, modifiers, toolbox.doc.defaultView);
> > > +  }
> > > +}
> > > +
> > > +function selectCB(event, id) {
> > 
> > Nit: Don't see a "cb" convention in our code anywhere, onSelect()?
> 
> Agreed.
> 
> > ::: browser/devtools/framework/toolbox.js
> > @@ +234,5 @@
> > >     * Adds the keys and commands to the Toolbox Window in window mode.
> > >     */
> > >    _addKeysToWindow: function TBOX__addKeysToWindow() {
> > > +    this.doc.defaultView.selectNextTool = this.selectNextTool.bind(this);
> > > +    this.doc.defaultView.selectPreviousTool = this.selectPreviousTool.bind(this);
> > 
> > instead of this, why not just set the command listener dynamically?
> 
> Then we will have to add a 'oncommand="void(0);"' to the <key> elements so
> that its command can be listened via JS. Also since there was no use of the
> event argument or any other argument, I thought this might be easier.

I'd recommend the oncommand="void(0)" That pattern is already used in the file anyways, and there's no wondering where the magical selectNextTool() came from. That could also error if you haven't injected it into the document yet.

Comment 6

[Girish Sharma [:Optimizer]]

Attachment: patch v1.2

dammit, did not notice that I am doing just the same, lines above for options key .. :P

Changed to event listeners now :)

new try : https://tbpl.mozilla.org/?tree=Try&rev=e69e821d68b3

Comment 7

[Heather Arthur [:harth]]

Comment on attachment 777339 [details] [diff] [review]
patch v1.2

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

One more thing, no need to do another try if this passes locally.

::: browser/devtools/framework/toolbox.js
@@ +235,5 @@
>     */
>    _addKeysToWindow: function TBOX__addKeysToWindow() {
> +    let nextKey = this.doc.getElementById("toolbox-next-tool-key");
> +    nextKey.addEventListener("command", () => {
> +      this.selectNextTool();

nextKey.addEventListener("command", this.selectNextTool.bind(this), true);

is surely shorter.

Comment 8

[Heather Arthur [:harth]]

Comment on attachment 777339 [details] [diff] [review]
patch v1.2

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

one more thing. Test is failing locally right now so will try to debug that.

::: browser/devtools/framework/toolbox.js
@@ +554,5 @@
> +  selectNextTool: function TBOX_selectNextTool() {
> +    let selected = this.doc.querySelector(".devtools-tab[selected]");
> +    let next = selected.nextSibling || selected.parentNode.firstChild;
> +    let tool = next.getAttribute("toolid");
> +    return this.selectTool(tool);

selectTool doesn't return anything, and you're not using the return value of selectNextTool anywhere, so no need to explicitly return here.

Comment 9

[Girish Sharma [:Optimizer]]

(In reply to Heather Arthur [:harth] from comment #8)
> Comment on attachment 777339 [details] [diff] [review]
> patch v1.2
> 
> Review of attachment 777339 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> one more thing. Test is failing locally right now so will try to debug that.

I can do that tomorrow :) No hurries :)

> ::: browser/devtools/framework/toolbox.js
> @@ +554,5 @@
> > +  selectNextTool: function TBOX_selectNextTool() {
> > +    let selected = this.doc.querySelector(".devtools-tab[selected]");
> > +    let next = selected.nextSibling || selected.parentNode.firstChild;
> > +    let tool = next.getAttribute("toolid");
> > +    return this.selectTool(tool);
> 
> selectTool doesn't return anything, and you're not using the return value of
> selectNextTool anywhere, so no need to explicitly return here.

selectTool returns the promise of selecting the tool. I am not using it anywhere, but its better to return the promise so that someone else can chain promises .

Comment 10

[Girish Sharma [:Optimizer]]

Attachment: patch v2.0

The problem with tests failing in the previous patch was that the _addKeysToWindow method was called on each tool creation , thus multiple event handlers.
I moved the event handler addition to a separate method and it is now called only once.

All tests pass locally.

Comment 11

[Heather Arthur [:harth]]

Comment on attachment 777854 [details] [diff] [review]
patch v2.0

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

perfect. Thanks again for the patch!

::: browser/devtools/framework/toolbox.js
@@ +227,5 @@
>    _buildOptions: function TBOX__buildOptions() {
>      let key = this.doc.getElementById("toolbox-options-key");
>      key.addEventListener("command", function(toolId) {
>        this.selectTool(toolId);
>      }.bind(this, "options"), true);

just want to point out that we can also just do `this.selectTool.bind(this, "options")` here.

Comment 12

[Girish Sharma [:Optimizer]]

landed in fx-team : https://hg.mozilla.org/integration/fx-team/rev/ed6e47fb4bfe

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

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