645371 - "Switch to next/previous tab group" shortcut should be a <key> and <command>

Status: RESOLVED WONTFIX

(Firefox Graveyard :: Panorama, defect, P3)

Description

[Tim Taubert [:ttaubert] (inactive)]

This way we don't need to register a custom keypress handler to switch groups. When offering a <key> and a <command> it's way easier for add-ons to re-use this functionality (e.g. change the shortcut).

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

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

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=ffb28699d069

Comment 4

[Ian Gilman [:iangilman]]

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

Good idea! It looks good to me, but I'd like Dao to weigh in as well.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Dão, could you please review this? Thanks!

Comment 6

[Dão Gottwald [:dao]]

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

>+    <key id="key_nextTabGroup" key="&switchTabGroup.commandkey;" command="Browser:NextTabGroup" modifiers="accel"/>

This doesn't work at all with my keyboard layout...

>+    <key id="key_previousTabGroup" key="&switchTabGroup.commandkey;" command="Browser:PreviousTabGroup" modifiers="accel,shift"/>

And if Ctrl+` worked, wouldn't Shift+Ctrl+` change the key code, preventing this from functioning?

>+  _switchGroupItem: function (reverse) {
>+    let numHiddenTabs = gBrowser.tabs.length - gBrowser.visibleTabs.length;
>+    if (this.isVisible() || !numHiddenTabs)
>+      return;

visibleTabs doesn't need to be queried in case _firstUseExperienced is false or this.isVisible() is true:

if (!this._firstUseExperienced ||
    this.isVisible() ||
    gBrowser.tabs.length - gBrowser.visibleTabs.length == 0)
  return;

>+      window.gBrowser.selectedTab = tabItem.tab;

"window." is redundant

Comment 7

[Scoobidiver (away)]

Currently, these shortcuts work only for some keyboard layouts where there is the `~ key at the top left (see http://en.wikipedia.org/wiki/Keyboard_layout), which is restricted to some en-US and European keyboard layouts.
Can these shortcuts be associated to the top left key whatever its content?

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #7)
> Can these shortcuts be associated to the top left key whatever its content?

I don't think it's that easy because we would have to figure this out for every locale. Btw, the discussion about the shortcut itself is in bug 591935 :)

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

bugspam

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

bugspam

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

bugspam

(Fx7 was branched, removing open bugs from Fx7 meta bug, we won't create new meta bugs for upcoming Fx versions)

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Not going to work on this anytime soon.