Last Comment Bug 643294 - [regression] Mousewheel scroll on tab bar should change tab
: [regression] Mousewheel scroll on tab bar should change tab
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Philip Chee
:
:
Mentors:
: 409792 (view as bug list)
Depends on: 356675
Blocks: 652822 484968 673878
  Show dependency treegraph
 
Reported: 2011-03-20 09:47 PDT by Philip Chee
Modified: 2011-07-25 03:37 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1.0 Tab Scroll Wheel. (2.02 KB, patch)
2011-03-20 10:03 PDT, Philip Chee
jh: feedback+
Details | Diff | Splinter Review
Patch v1.1 phase="capturing" (1.82 KB, patch)
2011-03-20 20:33 PDT, Philip Chee
neil: review+
Details | Diff | Splinter Review

Description Philip Chee 2011-03-20 09:47:50 PDT
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/>
Comment 1 Philip Chee 2011-03-20 10:03:40 PDT
Created attachment 520532 [details] [diff] [review]
Patch v1.0 Tab Scroll Wheel.

Based on the  Tab Scroll Wheel extension.

> -          document.removeEventListener("resize", this, false)
> +          window.removeEventListener("resize", this, false);
Brain fade from Bug 484968.
Comment 2 Jens Hatlak (:InvisibleSmiley) 2011-03-20 10:34:28 PDT
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. ;-)
Comment 3 neil@parkwaycc.co.uk 2011-03-20 12:29:37 PDT
Comment on attachment 520532 [details] [diff] [review]
Patch v1.0 Tab Scroll Wheel.

r- for not using a <handler> (see tabbox.xml).
Comment 4 Philip Chee 2011-03-20 17:33:14 PDT
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]
Comment 5 neil@parkwaycc.co.uk 2011-03-20 17:46:28 PDT
Even with phase="capturing"?
Comment 6 Philip Chee 2011-03-20 20:33:13 PDT
Created attachment 520593 [details] [diff] [review]
Patch v1.1 phase="capturing"

> Even with phase="capturing"?
Yes, this works. I must have been mistaken about Jens saying that it didn't work.
Comment 7 Jens Hatlak (:InvisibleSmiley) 2011-03-21 02:16:15 PDT
(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 8 neil@parkwaycc.co.uk 2011-03-21 03:02:23 PDT
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...)
Comment 9 Philip Chee 2011-03-21 12:47:24 PDT
> 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
Comment 10 Jens Hatlak (:InvisibleSmiley) 2011-03-21 15:17:35 PDT
*** Bug 409792 has been marked as a duplicate of this bug. ***

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