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

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

Comment 1

[Dão Gottwald [:dao]]

Attachment: patch

with test fix

Comment 2

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 3

[Dão Gottwald [:dao]]

(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.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

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).

Comment 5

[Dão Gottwald [:dao]]

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).

Comment 6

[:Gavin Sharp [email: [e-mail]]]

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 7

[:Gavin Sharp [email: [e-mail]]]

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).

Comment 8

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/fa2c7d2c52b0

Comment 9

[Joe Wilson]

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.

Comment 10

[Dão Gottwald [:dao]]

(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.