Closed Bug 357951 Opened 13 years ago Closed 12 years ago

Double clicking on the tab overflow scroll buttons should scroll by the width of the tab bar

Categories

(Firefox :: Tabbed Browser, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 3 alpha8

People

(Reporter: ventnor.bugzilla, Assigned: dao)

References

Details

Attachments

(1 file, 8 obsolete files)

Much of the criticism for our implementation of tab overflow is how it is really slow and awkward for power users to manage. However, I have a proposal to make the overflow tolerable and quick for power users of Firefox.

Most power users are familiar with the usefulness of the middle-mouse button. So, I thought that clicking the overflow buttons with the middle mouse button should scroll by a large amount. Ideally, by the width of the tab strip.

I made an implementation of this and found that I could go from one end of the tab strip to the other, with about 25 tabs open, in 3 mouse clicks. This was with the default minTabWidth. In my opinion, this would help make tab overflow a lot more tolerable while adding no feature or UI bloat whatsoever.

Nonetheless, I need your thoughts.
Attached patch Patch (obsolete) — Splinter Review
Just to give you an idea of what I did.
Attachment #243459 - Flags: ui-review?(beltzner)
Summary: Middle clicking on the tab overflow scroll buttons should scroll by the width of the tab → Middle clicking on the tab overflow scroll buttons should scroll by the width of the tab bar
Attached patch Patch v2 (obsolete) — Splinter Review
Clean up the code a bit by removing unnecessary parameter-passing.
Assignee: nobody → ventnor.bugzilla
Attachment #243459 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #249550 - Flags: ui-review?(beltzner)
Attachment #249550 - Flags: review?(mano)
Attachment #243459 - Flags: ui-review?(beltzner)
Comment on attachment 249550 [details] [diff] [review]
Patch v2

onmousedown="_startScroll(event);" shodld work as well
(In reply to comment #3)
> (From update of attachment 249550 [details] [diff] [review] [edit])
> onmousedown="_startScroll(event);" shodld work as well
> 

It doesn't. Not wrapping _startScroll in a function(event){ } statement breaks it too.
Comment on attachment 249550 [details] [diff] [review]
Patch v2

Also, removeEvnetListener doesn't work this way AFAICT since you're creating a new function. As noted, you could just keep using onmousedown.
Attachment #249550 - Flags: review?(mano) → review-
Attached patch Patch v3 (obsolete) — Splinter Review
Ah, I found the reason why onmousedown didn't work. There is some code duplication in tabbrowser.xml. It was still passing a number to _startScroll instead of the mouse event like I want.
Attachment #249550 - Attachment is obsolete: true
Attachment #249606 - Flags: ui-review?(beltzner)
Attachment #249606 - Flags: review?(mano)
Attachment #249550 - Flags: ui-review?(beltzner)
Comment on attachment 249606 [details] [diff] [review]
Patch v3

We should only do this for tabbrowser IMO.
Attached patch Like this? (obsolete) — Splinter Review
Do you mean just changing the values for tabbrowser but not in the scrollbox implementation like in this patch? If not please make it more clear to me.

I tested this and it works, which questions why the scrollbox implementation is there in the first place.
Attachment #249606 - Attachment is obsolete: true
Attachment #253811 - Flags: review?(mano)
Attachment #249606 - Flags: ui-review?(beltzner)
Hey Michael,

Middle click carries a "open in new tab / close tab" semantic already on the tabstrip, so I wonder if we can't think of a better accelerator here. I was thinking double click ... is it possible to detect that?
(In reply to comment #10)
> I wonder if we can't think of a better accelerator here. I was
> thinking double click ... is it possible to detect that?

Yes, it is. I have working code for that and can submit the corresponding patch once bug 347363 is fixed.

Michael, feel free to reassign this bug to you if you were still actively working on it. FWIW, based on the current patch for bug 347363, you should utilize ensureElementIsVisible() instead of scrollByPixels().
Assignee: ventnor.bugzilla → dao
Status: ASSIGNED → NEW
Depends on: 347363
OS: Windows XP → All
Hardware: PC → All
Summary: Middle clicking on the tab overflow scroll buttons should scroll by the width of the tab bar → Double clicking on the tab overflow scroll buttons should scroll by the width of the tab bar
Status: NEW → ASSIGNED
By all means, take it.
Since we're expecting users to press this several times in a row, double-click seems slightly awkward. For example Michael's example of scrolling through 24 tabs in three mouseclicks translates into 6 mouseclicks. Although on the plus side it is quite easily discoverable (possibly too easily, since there are legitimate cases for scrolling by e.g. two tabs at a time).
With smooth scrolling (bug 347363), scrolling by two tabs with a double click isn't supported. It could be implemented though. Either way, the user will discover it. As somebody who usually has 40+ tabs in a single window, I find scrolling by the width of the tab bar more useful. I just hold the mouse button pressed in order to scroll by two tabs.

By the way, my current implementation makes triple clicking scroll to the beginning resp. end of the tab bar.
Attachment #253811 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
Attachment #276920 - Flags: review?(mano)
Attached patch patch (obsolete) — Splinter Review
strict warning fixed
Attachment #276920 - Attachment is obsolete: true
Attachment #276922 - Flags: review?(mano)
Attachment #276920 - Flags: review?(mano)
Keywords: uiwanted
Attachment #276922 - Flags: review?(mano) → review?(enndeakin)
Comment on attachment 276922 [details] [diff] [review]
patch

>+
>+          if (aEvent.detail == 2) {
>+            // scroll by the width of the tab bar; make sure that the next

scroll by the width of the scrollbox...
Attachment #276922 - Flags: review?(enndeakin) → review+
Attached patch for checkin (obsolete) — Splinter Review
Attachment #276922 - Attachment is obsolete: true
Keywords: checkin-needed
Attached patch synced with trunk (obsolete) — Splinter Review
somebody check this in, please.
Attachment #277026 - Attachment is obsolete: true
Attachment #277868 - Attachment is obsolete: true
Checking in browser/base/content/tabbrowser.xml;
/cvsroot/mozilla/browser/base/content/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.240; previous revision: 1.239
done
Checking in toolkit/content/widgets/scrollbox.xml;
/cvsroot/mozilla/toolkit/content/widgets/scrollbox.xml,v  <--  scrollbox.xml
new revision: 1.27; previous revision: 1.26
done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 M8
Depends on: 399657
You need to log in before you can comment on or make changes to this bug.