Closed Bug 892244 Opened 6 years ago Closed 6 years ago

Ctrl+[ / Ctrl+] to switch devtools tabs

Categories

(DevTools :: Framework, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 25

People

(Reporter: harth, Assigned: Optimizer)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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.
Blocks: 879219
Assignee: nobody → scrapmachines
Status: NEW → ASSIGNED
Attached patch patch v1.0 (obsolete) — Splinter Review
This patch only contains the tab switching mechanism.
try at : https://tbpl.mozilla.org/?tree=Try&rev=71b04c3b0e61
Attachment #776569 - Flags: review?(fayearthur)
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.
(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.
Attached patch patch v1.1 (obsolete) — Splinter Review
Addressed review comments.

new try : https://tbpl.mozilla.org/?tree=Try&rev=71cf8af140a0
Attachment #776569 - Attachment is obsolete: true
Attachment #776569 - Flags: review?(fayearthur)
Attachment #777217 - Flags: review?(fayearthur)
(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.
Attached patch patch v1.2 (obsolete) — Splinter Review
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
Attachment #777217 - Attachment is obsolete: true
Attachment #777217 - Flags: review?(fayearthur)
Attachment #777339 - Flags: review?(fayearthur)
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.
Attachment #777339 - Flags: review?(fayearthur) → review+
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.
(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 .
Attached patch patch v2.0Splinter Review
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.
Attachment #777339 - Attachment is obsolete: true
Attachment #777854 - Flags: review?(fayearthur)
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.
Attachment #777854 - Flags: review?(fayearthur) → review+
landed in fx-team : https://hg.mozilla.org/integration/fx-team/rev/ed6e47fb4bfe
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/ed6e47fb4bfe
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 25
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.