Shift+Ctrl+Tab should open the classic all-tabs menu with browser.ctrlTab.previews=true and browser.allTabs.previews=false

RESOLVED FIXED in Firefox 8

Status

()

Firefox
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Created attachment 543890 [details] [diff] [review]
patch
Attachment #543890 - Flags: review?(gavin.sharp)
(Assignee)

Comment 1

6 years ago
Created attachment 543892 [details] [diff] [review]
patch

with test fix
Attachment #543890 - Attachment is obsolete: true
Attachment #543892 - Flags: review?(gavin.sharp)
Attachment #543890 - Flags: review?(gavin.sharp)
I don't really understand the motivation here. Why should Shift+Ctrl+Tab behave differently than Ctrl+Tab (apart from the obvious ordering difference)?

In general, I think we need to commit one way or the other on these ctrlTab.previews and allTabs.previews prefs, and stop having so much disabled-by-default code to maintain (particularly when both features can be toggled independently and can interact, as in this case). m-c isn't really the right place for this kind of experimental work to happen.
(Assignee)

Comment 3

6 years ago
(In reply to comment #2)
> I don't really understand the motivation here. Why should Shift+Ctrl+Tab
> behave differently than Ctrl+Tab (apart from the obvious ordering
> difference)?

Exactly because of this difference. Shift is supposed to reverse the order, theoretically, but this doesn't really make sense for a most-recently-used ordered list. This a) is the reason why Shift+Ctrl+Tab currently opens the all tabs panel when browser.ctrlTab.previews is set to true and b) was part of beltzner's design that's currently implemented.

Flipping browser.ctrlTab.previews would then be a way to properly fix bug 597308.
I'm kind of confused about how this code works in general. What part of this patch limits the behavior to Shift+Ctrl+Tab (as opposed to Ctrl+Tab)? The command handling for this stuff is very difficult to follow (another reason it'd be nice to either fully integrate and enable this feature or remove it).
(Assignee)

Comment 5

6 years ago
What limits the behavior to Shift+Ctrl+Tab isn't part of this patch, it's in browser-sets.inc:

<command id="Browser:ShowAllTabs" oncommand="allTabs.open();"/>

<key id="key_showAllTabs" command="Browser:ShowAllTabs" keycode="VK_TAB" modifiers="control,shift"/>

This only works when Ctrl+Tab isn't handled by the tab box (where Shift moving backwards actually makes sense).
I was confused in general, I thought you were patching ctrlTab_open, and didn't realize ctrlTab__init worked the way it did. It's confusing to have a Browser:ShowAllTabs command/key that seems generic but is actually specific to ctrlTab.previews, and disabled dynamically. Another reason why I would like these optional features to either not be optional, or not be there.
Comment on attachment 543892 [details] [diff] [review]
patch

Another point of confusion: allTabs.open() gets called despite allTabs.previews=false, and then uses allTabsButton.getAttribute("type") to determine whether it is itself enabled (rather than this.enabled or something).
Attachment #543892 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 8

6 years ago
http://hg.mozilla.org/mozilla-central/rev/fa2c7d2c52b0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 8

Comment 9

6 years ago
And how am I supposed to scroll 1 tab back now?
Please, could you use another hotkey for opening alltabs list?
Ctrl+Tab always was the hotkey that switched to the next tab, and Shift+Ctrl+Tab was to the previous one.
And now you broke it.
(Assignee)

Comment 10

6 years ago
(In reply to comment #9)
> Ctrl+Tab always was the hotkey that switched to the next tab, and
> Shift+Ctrl+Tab was to the previous one.

Not with browser.ctrlTab.previews=true, no.
You need to log in before you can comment on or make changes to this bug.