Created attachment 543890 [details] [diff] [review]
Created attachment 543892 [details] [diff] [review]
with test fix
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.
(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
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).
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]
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).
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.
(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.