384793 - Option-click on the tab scroll buttons should scroll a page-width of tabs

Status: RESOLVED FIXED

(Camino Graveyard :: Tabbed Browsing, defect, P1)

Description

[froodian (Ian Leue)]

Summary says it all.  When you option-click on one of the tab scroll buttons, we should scroll a whole page-width of tabs.

Additionally, we tentatively discussed the idea of command-click going to the beginning or end of the tab bar.  These two behaviors would mimic text fields (in that option-left/right goes a word's worth, and cmd-left/right goes to the beginning/end of the line)

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

If this is UI, it's P1.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

We should probably try to get this feature for a1.

Comment 3

[froodian (Ian Leue)]

Attachment: Patch, v1

Scrolls a window's width, or as far as it can.  Scrolls regularly for option-scrollwheel, matching the OS's behavior for scroll bars.

Comment 4

[Håkan Waara]

Comment on attachment 282500 [details] [diff] [review]
Patch, v1

>Index: src/browser/BrowserTabBarView.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/browser/BrowserTabBarView.mm,v
>retrieving revision 1.31
>diff -u -8 -r1.31 BrowserTabBarView.mm
>--- src/browser/BrowserTabBarView.mm	28 Aug 2007 21:33:05 -0000	1.31
>+++ src/browser/BrowserTabBarView.mm	27 Sep 2007 03:57:30 -0000
>@@ -636,36 +636,53 @@
> 
>   // We don't use the accellation; just scroll one tab per event.
>   if (scrollIncrement > 0.0)
>     [self scrollRight:nil];
>   else if (scrollIncrement < 0.0)
>     [self scrollLeft:nil];
> }
> 
>-// Scrolls the tab bar one place to the right, if possible.
> -(void)scrollLeft:(id)aSender
> {
>-  if (mLeftMostVisibleTabIndex > 0)
>-    [self setLeftMostVisibleTabIndex:(mLeftMostVisibleTabIndex - 1)];
>+  int numberOfTabs = 1;

Please use a better variable name. numberOfTabsToScroll?

>+  // We can safely scroll up to the number of tabs hidden to the left
>+  int maxScrollableTabs = mLeftMostVisibleTabIndex;

Also here. maxScrollableTabs is really something like numTabsToTheLeft

>+  // If option's down and we're being called from a button-click
>+  if (([[NSApp currentEvent] modifierFlags] & NSAlternateKeyMask) && [aSender isKindOfClass:[NSButton class]])
>+    // scroll as far as we can, up to a window's width
>+    numberOfTabs = (mNumberOfVisibleTabs > maxScrollableTabs) ? maxScrollableTabs : mNumberOfVisibleTabs;
>+
>+  if (maxScrollableTabs >= numberOfTabs)
>+    [self setLeftMostVisibleTabIndex:(mLeftMostVisibleTabIndex - numberOfTabs)];

This condition doesn't make much sense. When would number of tabs to the left be less than the number of tabs we want to scroll? Only when we're at the first tab (and only because you preset numberOfTabs to 1). I think the |if (mLeftMostVisibleTabIndex > 0)| condition you removed was a simpler way of expressing the same thing.

Basically the same comments apply to the right scroll. Except for this, looks like a good patch! Thanks for adding some additional comments too.

Comment 5

[froodian (Ian Leue)]

Attachment: Patch, v2

Essentially addresses all of your comments.  For the condition at the end, I used the local variables we're already storing of that information to make it a little more concise and cleaner (for scrollRight particularly, and mostly just to make it consistent for scrollLeft)

Comment 6

[Håkan Waara]

Comment on attachment 284578 [details] [diff] [review]
Patch, v2

Nice

Comment 7

[Stuart Morgan]

Comment on attachment 284578 [details] [diff] [review]
Patch, v2

>+    // scroll as far as we can, up to a window's width

This description seems backwards; I'd rather it say something more like "Scroll a window's width (if possible)".

>+    numberOfTabsToScroll = (mNumberOfVisibleTabs > tabsHiddenToTheLeft) ? tabsHiddenToTheLeft : mNumberOfVisibleTabs;

... = MIN(mNumberOfVisibleTabs, tabsHiddenToTheLeft);

>+  // We can safely scroll up to the total number of tabs minus the index of the rightmost visible tab (ie the number of hidden tabs on the right)

No need to describe the equation that is right there. Just say:
// We can safely scroll up to the number of tabs hidden to the right

>+    // scroll as far as we can, up to a window's width
>+    numberOfTabsToScroll = (mNumberOfVisibleTabs > tabsHiddenToTheRight) ? tabsHiddenToTheRight : mNumberOfVisibleTabs;

Same comments here as above.

sr=smorgan with those changes.

Comment 8

[froodian (Ian Leue)]

Attachment: Patch for chicken

Addresses sr comments

Comment 9

[froodian (Ian Leue)]

Checked in on trunk and MOZILLA_1_8_BRANCH.