[regression] Mousewheel scroll on tab bar should change tab

RESOLVED FIXED

Status

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: philip.chee, Assigned: philip.chee)

Tracking

(Blocks 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
From Bug 409792 - Mousewheel scroll on tab bar should change tab in Suiterunner

> Jens Hatlak (:InvisibleSmiley)      2011-02-24 12:12:00 PST
> 
> (In reply to comment #8)
> > Now that bug 484968 is fixed, the scrollable tabbar eats the wheel events.
> 
> Yeah, one of these cases where one groups wins and one loses, and I'm in the
> latter group though I was first (as far as SM is concerned). :-(
> 
> > But on Linux builds, you can still use the scroll wheel to change tabs, 
> > as long as you hover over the tab picker or the new or close tab buttons.
> 
> Ah. I wonder whether that can be extended to work on all platforms, maybe with
> a modifier like Ctrl (Accel).
> 
> If everything goes wrong we can resort to providing an extension for this but
> I'd surely appreciate it if it was fixed in the application (so that the
> extension doesn't break in the future).
> 
> Jens Hatlak (:InvisibleSmiley) 2011-03-20 07:25:59 PDT
> 
> Workaround: I just discovered that the following add-on works with both SM 2.0
> and latest trunk (so quite probably also with 2.1 final):
> <https://addons.mozilla.org/de/seamonkey/addon/tab-wheel-scroll/>
(Assignee)

Comment 1

8 years ago
Posted patch Patch v1.0 Tab Scroll Wheel. (obsolete) — Splinter Review
Based on the  Tab Scroll Wheel extension.

> -          document.removeEventListener("resize", this, false)
> +          window.removeEventListener("resize", this, false);
Brain fade from Bug 484968.
Attachment #520532 - Flags: review?(neil)
Attachment #520532 - Flags: feedback?(jh)
Comment on attachment 520532 [details] [diff] [review]
Patch v1.0 Tab Scroll Wheel.

Thanks Phil! I'm plus'ing this since it works perfectly and doesn't regress tab bar scrolling (in case of more tabs that can be displayed) which IMO makes a modifier key distinction unnecessary. Nice! :-)

I noticed that this does not wrap around like Ctrl[+Shift]+Tab or the Tab Scroll Wheel extension (which has this configurable). This would be a nice addition, maybe also with a pref, but surely nothing to keep me from giving a positive feedback. ;-)
Attachment #520532 - Flags: feedback?(jh) → feedback+
Comment on attachment 520532 [details] [diff] [review]
Patch v1.0 Tab Scroll Wheel.

r- for not using a <handler> (see tabbox.xml).
Attachment #520532 - Flags: review?(neil) → review-
(Assignee)

Comment 4

8 years ago
Comment on attachment 520532 [details] [diff] [review]
Patch v1.0 Tab Scroll Wheel.

> r- for not using a <handler> (see tabbox.xml).

Using a <handler> doesn't work. The <handler> in the base binding gets to receive the event first then it eats the mouse events. However xbl:handlers always get the event last after *.addEventListener() [According to DevMo anyway]
Attachment #520532 - Flags: review- → review?(neil)
Even with phase="capturing"?
(Assignee)

Comment 6

8 years ago
> Even with phase="capturing"?
Yes, this works. I must have been mistaken about Jens saying that it didn't work.
Attachment #520532 - Attachment is obsolete: true
Attachment #520532 - Flags: review?(neil)
Attachment #520593 - Flags: review?(neil)
(In reply to comment #6)
> I must have been mistaken about Jens saying that it didn't work.

Sorry about that. I tried to "block" the event using event.* and then "skip" the intermediate handler in order to let the lower one which provides tab switching handle the event (or somehow call it). That failed; I guess what I tried is impossible in XBL. I didn't think about this.advanceSelectedTab and that one could use it without regressing the tab overflow scrolling. Anyway, the result counts and that is that you made it. Thanks!
Comment on attachment 520593 [details] [diff] [review]
Patch v1.1 phase="capturing"

>       <handler event="transitionend">
>         <![CDATA[
>           if (event.propertyName == "max-width")
>             this._handleNewTab(event.target);
>         ]]>
>       </handler>
>+
>+      <handler event="DOMMouseScroll" phase="capturing">
>+      <![CDATA[
>+        this.advanceSelectedTab(event.detail < 0 ? -1 : 1);
>+        event.stopPropagation();
>+      ]]>
>+      </handler>
Nit: inconsistent CDATA indentation. (I know, I know, why do we have at least three different styles in various XBL files...)
Attachment #520593 - Flags: review?(neil) → review+
(Assignee)

Comment 9

8 years ago
> neil@parkwaycc.co.uk      2011-03-21 03:02:23 PDT
> 
> >+
> >+      <handler event="DOMMouseScroll" phase="capturing">
> >+      <![CDATA[
> >+        this.advanceSelectedTab(event.detail < 0 ? -1 : 1);
> >+        event.stopPropagation();
> >+      ]]>
> >+      </handler>
> Nit: inconsistent CDATA indentation. (I know, I know, why do we have at least
> three different styles in various XBL files...)
Fixed.
Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/c90a5e567e7c
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Duplicate of this bug: 409792
(Assignee)

Updated

8 years ago
Blocks: 652822

Updated

8 years ago
Blocks: 673878
You need to log in before you can comment on or make changes to this bug.