Last Comment Bug 669265 - Shift+Ctrl+Tab should open the classic all-tabs menu with browser.ctrlTab.previews=true and browser.allTabs.previews=false
: Shift+Ctrl+Tab should open the classic all-tabs menu with browser.ctrlTab.pre...
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 8
Assigned To: Dão Gottwald [:dao]
:
:
Mentors:
Depends on:
Blocks: 597308
  Show dependency treegraph
 
Reported: 2011-07-05 00:49 PDT by Dão Gottwald [:dao]
Modified: 2011-08-02 02:22 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (2.22 KB, patch)
2011-07-05 00:49 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Splinter Review
patch (2.77 KB, patch)
2011-07-05 00:54 PDT, Dão Gottwald [:dao]
gavin.sharp: review+
Details | Diff | Splinter Review

Description Dão Gottwald [:dao] 2011-07-05 00:49:48 PDT
Created attachment 543890 [details] [diff] [review]
patch
Comment 1 Dão Gottwald [:dao] 2011-07-05 00:54:51 PDT
Created attachment 543892 [details] [diff] [review]
patch

with test fix
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-05 08:34:46 PDT
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] 2011-07-05 08:41:13 PDT
(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: gavin@gavinsharp.com] 2011-07-06 13:58:02 PDT
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] 2011-07-06 14:23:04 PDT
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: gavin@gavinsharp.com] 2011-07-07 14:16:49 PDT
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: gavin@gavinsharp.com] 2011-07-07 14:19:40 PDT
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] 2011-07-08 01:19:51 PDT
http://hg.mozilla.org/mozilla-central/rev/fa2c7d2c52b0
Comment 9 Joe Wilson 2011-08-01 13:50:07 PDT
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] 2011-08-02 02:22:57 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.