Closed Bug 645371 Opened 13 years ago Closed 12 years ago

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

Categories

(Firefox Graveyard :: Panorama, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: ttaubert, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

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).
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #522137 - Flags: review?(ian)
Attached patch patch v2Splinter Review
Attachment #522137 - Attachment is obsolete: true
Attachment #522137 - Flags: review?(ian)
Attachment #522172 - Flags: review?(ian)
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.
Attachment #522172 - Flags: review?(ian)
Attachment #522172 - Flags: review?(dao)
Attachment #522172 - Flags: feedback+
Blocks: 603789
Priority: -- → P3
No longer blocks: 603789
Blocks: 653099
Dão, could you please review this? Thanks!
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
Attachment #522172 - Flags: review?(dao)
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?
(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 :)
bugspam
No longer blocks: 653099
bugspam
Blocks: 660175
bugspam

(Fx7 was branched, removing open bugs from Fx7 meta bug, we won't create new meta bugs for upcoming Fx versions)
No longer blocks: 660175
Not going to work on this anytime soon.
Assignee: ttaubert → nobody
Status: ASSIGNED → NEW
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: