Closed Bug 378028 Opened 17 years ago Closed 16 years ago

two-fingered trackpad scrolling: horizontal movement causes vertical scrolling

Categories

(Core :: XUL, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: keithc--mozilla, Assigned: roc)

References

Details

(Keywords: verified1.9.1)

Attachments

(3 files, 5 obsolete files)

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-us) AppleWebKit/419 (KHTML, like Gecko) Safari/419.3
Build Identifier: version 3.0a1 (20070419)

Right-to-left movement on the trackpad in the message list pane causes vertical scrolling, as does top-to-bottom movement. Thus diagonal movement from top-right to bottom-left causes jitter as the scolling position is simultaneously moved up and down. This makes it necessary for the user to be very precise in their movements. The obvious fix is to ignore horizontal movement when there is no horizontal scrollbar (this would be consistent with other applications). This bug is also present in the release build of 2.0.0.0.

Reproducible: Always

Steps to Reproduce:
1.position cursor in message list pane for any long list of messages
2.enable two-fingered scrolling!
3.move fingers diagonally from top right towards bottom left
Actual Results:  
nasty jitter in the message list, unpredictable final scroll position

Expected Results:  
smooth scrolling; enough sweeps in this direction should leave the scroll position at the bottom of the list

tested with fresh profiles on two machines.
I see this in trees in Firefox trunk too (history sidebar, bookmarks window).  It makes it difficult to scroll using two fingers: if you don't go straight up or down, the scrolling jumps around.
Assignee: mscott → nobody
Status: UNCONFIRMED → NEW
Component: Mail Window Front End → XP Toolkit/Widgets: Trees
Ever confirmed: true
Product: Thunderbird → Core
QA Contact: front-end → xptoolkit.trees
Version: unspecified → Trunk
Flags: blocking1.9?
-'ing.  Not a regression.
Flags: blocking1.9? → blocking1.9-
Damon is right, this happens in Firefox 2.0.0.11 as well.  Pretty annoying, though.
Assignee: nobody → roc
Attached patch fix (obsolete) — Splinter Review
This patch borrows from Markus' patch in bug 350471 to create a nsDOMMouseScrollEvent class. Then I expose an "axis" attribute so that JS can know which direction the mouse scroll is coming from.

This is a very low risk approach compatibility-wise and Olli recommended it :-).

However, this patch has one problem. It spews a lot of assertions

###!!! ASSERTION: Class name and proto chain interface name mismatch!: 'nsCRT::strcmp(CutPrefix(name), mData->mName) == 0', file /Users/roc/mozilla-trunk/dom/src/base/nsDOMClassInfo.cpp, line 3680

'name' is nsIDOMNSMouseScrollEvent, 'mData->mName' is MouseScrollEvent. I guess this wouldn't happen if the interface name was nsIDOMMouseScrollEvent, but I thought we reserved such names for standard-ish things. Olli, do you think I should use NS here or not, and if we don't, how do we silence that assertion?
For event types we haven't used nsIDOMNS, see for example nsIDOMPopupBlockedEvent.
Attached patch fix v2 (obsolete) — Splinter Review
Okay, fix with name change.
Attachment #316560 - Attachment is obsolete: true
Attachment #316590 - Flags: superreview?
Attachment #316590 - Flags: review?(Olli.Pettay)
Attachment #316590 - Flags: superreview? → superreview?(jonas)
Comment on attachment 316590 [details] [diff] [review]
fix v2

>+nsDOMMouseScrollEvent::nsDOMMouseScrollEvent(nsPresContext* aPresContext,
>+                                             nsInputEvent* aEvent)
>+  : nsDOMMouseEvent(aPresContext, aEvent ? aEvent :
>+                 new nsMouseScrollEvent(PR_FALSE, 0, nsnull))
Missing few spaces?

>+nsDOMMouseScrollEvent::InitMouseScrollEvent(const nsAString & aType, PRBool aCanBubble, ...>+  if (mEvent->eventStructType == NS_MOUSE_SCROLL_EVENT) {
>+    static_cast<nsMouseScrollEvent*>(mEvent)->scrollFlags =
>+        (aAxis == HORIZONTAL_AXIS) ? nsMouseScrollEvent::kIsHorizontal : 0;

Here you set mEvent->scrollFlags, but GetAxis uses mScrollFlags.
Actually, there shouldn't be any need for mScrollFlags, since there is always
mEvent->scrollFlags

> XPIDLSRCS =					\
> 	nsIDOMNSEvent.idl			\
> 	nsIDOMDataContainerEvent.idl	\
> 	nsIDOMKeyEvent.idl			\
>+  nsIDOMMouseScrollEvent.idl \
Align this properly

>     <handlers>
>-      <handler event="DOMMouseScroll" action="this.scrollByIndex(event.detail); event.stopPropagation();"/>
>+      <handler event="DOMMouseScroll"><![CDATA[
>+        if (event.axis != event.HORIZONTAL_AXIS)
>+          return;
Should that be ==, not != ?
Attachment #316590 - Flags: review?(Olli.Pettay) → review-
Is it possible for mEvent to not be an NS_MOUSE_SCROLL_EVENT?

I hope you've got some big plan to simplify all this :-)
mEvent should be always NS_MOUSE_SCROLL_EVENT.
mEvent->message could be either NS_MOUSE_SCROLL or NS_USER_DEFINED_EVENT.

And yes, the current plan is to remove nsEvent usage from DOM :)
Attached patch fix v3 (obsolete) — Splinter Review
With those changes
Attachment #316590 - Attachment is obsolete: true
Attachment #316722 - Flags: superreview?(jonas)
Attachment #316722 - Flags: review?(Olli.Pettay)
Attachment #316590 - Flags: superreview?(jonas)
Comment on attachment 316722 [details] [diff] [review]
fix v3

Looks ok to me. And you have tested those XBL bindings, right ;)
Attachment #316722 - Flags: review?(Olli.Pettay) → review+
Attached patch fix v4 (obsolete) — Splinter Review
I really ought to test this before submitting
Attachment #316722 - Attachment is obsolete: true
Attachment #316725 - Flags: superreview?(jonas)
Attachment #316725 - Flags: review?(Olli.Pettay)
Attachment #316722 - Flags: superreview?(jonas)
I tested the XBL bindings to but failed to compile the C++, sorry.
Whiteboard: [needs review]
Comment on attachment 316725 [details] [diff] [review]
fix v4

>+NS_IMETHODIMP
>+nsDOMMouseScrollEvent::InitMouseScrollEvent(const nsAString & aType, PRBool aCanBubble, PRBool aCancelable,
>+                                nsIDOMAbstractView *aView, PRInt32 aDetail, PRInt32 aScreenX, 
>+                                PRInt32 aScreenY, PRInt32 aClientX, PRInt32 aClientY, 
>+                                PRBool aCtrlKey, PRBool aAltKey, PRBool aShiftKey, 
>+                                PRBool aMetaKey, PRUint16 aButton, nsIDOMEventTarget *aRelatedTarget,
>+                                PRInt32 aAxis)
>+{
>+  nsresult rv = nsDOMMouseEvent::InitMouseEvent(aType, aCanBubble, aCancelable, aView, aDetail,
>+                                                aScreenX, aScreenY, aClientX, aClientY, aCtrlKey,
>+                                                aAltKey, aShiftKey, aMetaKey, aButton, aRelatedTarget);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  
>+  if (mEvent->eventStructType == NS_MOUSE_SCROLL_EVENT) {
>+    static_cast<nsMouseScrollEvent*>(mEvent)->scrollFlags =
>+        (aAxis == HORIZONTAL_AXIS) ? nsMouseScrollEvent::kIsHorizontal : 0;
>+  }
Sorry, idiot me. Since scrollFlags is used also for other things than horizontal/vertical, 
it might after all be better to have separate mAxis, which is initiated in the constructor and in the
InitMouseScrollEvent. That way .axis returns always the right value, the value
which is either passed via nsMouseScrollEvent or InitMouseScrollEvent.
A bit ugly part is that also scrollFlags should be set in InitMouseScrollEvent if
aAxis is either HORIZONTAL_AXIS or VERTICAL_AXIS.

The current ? : expression in the patch is wrong anyway, it sets only horizontal flag, never the vertical one.

>+NS_IMETHODIMP
>+nsDOMMouseScrollEvent::GetAxis(PRInt32* aResult)
>+{
>+  NS_ENSURE_ARG_POINTER(aResult);
>+
>+  if (mEvent->eventStructType == NS_MOUSE_SCROLL_EVENT) {
>+    PRUint32 flags = static_cast<nsMouseScrollEvent*>(mEvent)->scrollFlags;
>+    *aResult = (flags & nsMouseScrollEvent::kIsHorizontal)
>+         ? PRInt32(HORIZONTAL_AXIS) : PRInt32(VERTICAL_AXIS);
>+  } else {
>+    *aResult = 0;
>+  }
>+  return NS_OK;
>+}

So this would become 
NS_ENSURE_ARG_POINTER(aResult);
*aResult = mAxis
return NS_OK;
Attached patch fix v5 (obsolete) — Splinter Review
I don't want to add mAxis ... I'd rather the information just be stored once so we don't have to worry about consistency. So here I'm just fixing the incorrect ?:.

Well, the other big thing I'm doing is adding mochitests for mousescroll events for trees, listboxes, and arrowscrollboxes. This turned out to be extremely painful but it's done. The tests also test XUL scrollboxes (via the richlistbox tests), which use C++ scrollframe/ESM logic but it's still good to have that tested.

Testing the arrowscrollboxes showed one issue with horizontal arrowscrollboxes (e.g. an overflowing tab strip). We should allow horizontal mouse scrolling to scroll such a tab strip. But we should *also* allow vertical mouse scrolling to scroll it horizontally, since a lot of people have vertical mouse scrolling but not horizontal mouse scrolling. On the other hand vertical arrowscrollboxes should not be horizontally scrollable. So there's a funny asymmetry there in the code and in the tests.
Attachment #316725 - Attachment is obsolete: true
Attachment #316807 - Flags: superreview?(jonas)
Attachment #316807 - Flags: review?(Olli.Pettay)
Attachment #316725 - Flags: superreview?(jonas)
Attachment #316725 - Flags: review?(Olli.Pettay)
Comment on attachment 316807 [details] [diff] [review]
fix v5

>+++ mozilla-trunk/toolkit/content/widgets/tree.xml

>       <handler event="DOMMouseScroll" phase="capturing">
>         <![CDATA[
>           if (this._editingColumn)
>             return;
>+          if (event.axis == event.HORIZONTAL_AXIS)
>+            return;

Wouldn't it be better to make use of that event using scrollToHorizontalPosition (of nsITreeBoxObject) instead of just throwing it away?

Attachment 128196 [details] (of bug 212789) can be used to play with horizontal tree scrolling.
Comment on attachment 316807 [details] [diff] [review]
fix v5

>+  /** Synthesize a mouse scroll event for a window. The event types supported
>+   *  are: 
>+   *    mousescroll
Shouldn't this be DOMMouseScroll.
Or perhaps you could remove the first parameter, aType, since that can be only
"mousescroll". Either way.
Attachment #316807 - Flags: review?(Olli.Pettay) → review+
We should keep the type since Markus wants to add DOMMousePixelScroll soon.

These names aren't really in the same namespace as the DOM event names as far as I can tell. But I'll make it DOMMouseScroll, sure.

(In reply to comment #16)
> Wouldn't it be better to make use of that event using
> scrollToHorizontalPosition (of nsITreeBoxObject) instead of just throwing it
> away?

Yeah. But I'm trying to not add any new features here right now :-). Adding that would be an easy extension later.
Whiteboard: [needs review] → [needs review sicking]
Is this something we're hoping to put in 1.9? What is the relation between this event and the one w3c is in the process of speccing?
I was hoping to get it into 1.9. Olli thought it was a low-risk way to fix this bug. Maybe it's too late. If so, we should still take it on trunk when the tree reopens.

This event is a legacy event that we already support. It's used by extensions and by Web content so we're not going to drop support anytime soon. When the W3C mouse-scroll event is done, we should support it as well, deprecate this one and maybe eventually remove it. But that's a few releases away I guess.
I can't justify doing this for 1.9 at this point. Punting.
Flags: wanted1.9-
Flags: wanted1.9+
Flags: wanted-next+
Would it be possible to change the Hardware: Macintosh and OS: Mac OS X to something more neutral. This is also an issue for GNU/Linux users that have configured two-finger trackpads with synaptics (as I did).
Adding a question. As the author of the SmoothWheel extension and submitter of bug 438426 (recently announced as a duplicate of this bug), Is there a way for an extension to access this info? Can smoothwheel distinguish horizontal from vertical events?
P.S. for comment #24.

It's obvious firefox can tell the difference because without smoothwheel installed, on OSX the scroll gestures work well, both vertical and horizontal. Smoothwheel only works on vertical scroll for now, and should ignore H scroll events, fut for now, I don't know how to do that.
(In reply to comment #24)
> Is there a way for
> an extension to access this info? Can smoothwheel distinguish horizontal from
> vertical events?

It can't. DOMMouseScroll events don't carry axis information yet; that's what the patch fixes. The patch gives DOMMouseScroll events an "axis" attribute so that you can check event.axis == event.HORIZONTAL_AXIS.

(In reply to comment #25)
> It's obvious firefox can tell the difference

Yes, because Firefox processes the original nsEvent [1] that contains a scrollFlags attribute; however, when the nsEvent is transformed into a DOMEvent, the scrollFlags information is lost and thus can't be accessed by JavaScript code.
There is one line in Firefox JavaScript code that pretends it can access the scrollFlags of a DOMMouseScroll event [2], but that's rubbish.

[1] http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/events/src/nsEventStateManager.cpp&rev=1.742&mark=2497-2499#2480
[2] http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/browser/base/content/browser-textZoom.js&rev=1.9&mark=151#138
Blocks: 350471
Thank you very much for the info.

Also, I'm trying to understand the practical implications of not including a solution for "1.9". Does that mean that it won't get into Firefox 3.0? Does it mean it's postponed to 4.0? or maybe that it can be fixed in a minor 3.0.x Firefox release?

We can land this as soon as Jonas reviews the patch. It would then be in the minor release of Firefox (3.1?) that we plan to have.
Is this following the new scroll and/or mouse events as defined by DOM L3 Events?
It's not - right now we're just extending the Mozilla-specific DOMMouseScroll event (see also comment 20). The Webapps wheel event [1] isn't quite ready for implementation - especially the unit stuff is still quite unclear, and Olli's mail about it [2] didn't receive any useful answers yet.

In bug 350471 comment 42, Olli suggested going the DOMMouseScroll route if the spec isn't ready.

[1] http://www.w3.org/2008/webapps/wiki/Mousewheel
[2] http://lists.w3.org/Archives/Public/public-webapps/2008AprJun/0231.html
Bug 350471 is separate for this, sort of. DOM 3 has already defined how to
handle X/Y/Z. What it is still missing is proper support for pixel scrolling (bug 350471).

But anyway, we could still use DOMMouseScroll (we can't remove support for that because of backward compatibility).

And note, DOM 3 Events isn't even in Last Call.

(In reply to comment #30)
> In bug 350471 comment 42, Olli suggested going the DOMMouseScroll route if the
> spec isn't ready.
I suggested going DOMMousePixelScroll route for pixel scrolling ;)
(In reply to comment #31)
> DOM 3 has already defined how to
> handle X/Y/Z.

Yeah, ok. It currently says that X/Y/Z should be handled using the attributes
  readonly attribute long            wheelDeltaX;
  readonly attribute long            wheelDeltaY;
  readonly attribute long            wheelDeltaZ;
... but if it's later decided to go with your suggestion 6 [1] and make these attributes *arrays* of longs, even this doesn't apply anymore.

[1] http://lists.w3.org/Archives/Public/public-webapps/2008AprJun/0233.html
(In reply to comment #32) 
> ... but if it's later decided to go with your suggestion 6 [1] and make these
> attributes *arrays* of longs, even this doesn't apply anymore.
true.

...hoping to find some good solution for pixel/line/page scroll events...
I don't think we should wait for the Webapps group to finalize their spec before we fix this for XUL apps and extensions. It's already a problem in Firefox.
Comment on attachment 316807 [details] [diff] [review]
fix v5

Ugh, sorry for the long review time :(
Attachment #316807 - Flags: superreview?(jonas) → superreview+
Whiteboard: [needs review sicking]
Attached patch fix v6Splinter Review
Updated to trunk, comments addressed, ready to check in.
Attachment #316807 - Attachment is obsolete: true
Attachment #331070 - Flags: superreview+
Attachment #331070 - Flags: review+
2 questions if you don't mind.

1. Just to see if I understand, is this the way to test orientation now?: if (event.axis == event.HORIZONTAL_AXIS)

2. Can this be accessed from normal html pages? or only from extensions?
It can be accessed from Web pages and extensions.

This isn't the greatest API because it can't handle diagonal motion in a single event, but it's more compatible with what we've already got. Hopefully the W3C event will obsolete this one before too long.
Pushed c0717c5ec0d2.
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
I backed this out because of test failures:

*** 61506 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_tree.xul | Error thrown during test: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindowUtils.sendMouseScrollEvent]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: http://localhost:8888/tests/SimpleTest/EventUtils.js :: synthesizeMouseScroll :: line 273"  data: no] - got 0, expected 1
*** 62352 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_tree_hier.xul | Error thrown during test: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindowUtils.sendMouseScrollEvent]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: http://localhost:8888/tests/SimpleTest/EventUtils.js :: synthesizeMouseScroll :: line 273"  data: no] - got 0, expected 1
*** 63084 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_tree_hier_cell.xul | Error thrown during test: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindowUtils.sendMouseScrollEvent]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: http://localhost:8888/tests/SimpleTest/EventUtils.js :: synthesizeMouseScroll :: line 273"  data: no] - got 0, expected 1
*** 63593 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_tree_single.xul | Error thrown during test: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindowUtils.sendMouseScrollEvent]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: http://localhost:8888/tests/SimpleTest/EventUtils.js :: synthesizeMouseScroll :: line 273"  data: no] - got 0, expected 1

These happened on all Tinderbox platforms. But sadly they don't happen on my machine.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
My Linux build doesn't show the errors either.
This might help localize the issue.
Component: XP Toolkit/Widgets: Trees → XUL
QA Contact: xptoolkit.trees → xptoolkit.widgets
Markus, can you try running those tests to see if you can reproduce the failures with this patch?
Yes! I can reproduce the failures :-)
I'll look into this now.
Attached patch fix testSplinter Review
That should do it. I wonder why the test wasn't failing for you...
Attachment #333401 - Flags: review?(roc)
Attachment #331293 - Attachment is patch: true
Attachment #331293 - Attachment mime type: application/text → text/plain
Comment on attachment 333401 [details] [diff] [review]
fix test

Thanks Markus!
Attachment #333401 - Flags: review?(roc) → review+
Pushed a02e6af74ab8.
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
1. Is this still the way to use it?
if (event.axis == event.HORIZONTAL_AXIS) ...

2. When will the public get a Firefox version that supports this enhancement? 3.0.x? 3.1?

thx
(In reply to comment #48)
> 1. Is this still the way to use it?
> if (event.axis == event.HORIZONTAL_AXIS) ...

Yes it is. You can try it in a recent nightly: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/

> 2. When will the public get a Firefox version that supports this enhancement?
> 3.0.x? 3.1?

3.1.
Depends on: 454472
Just thought I'd add this here. For the sake of backward compatibility (at least within extensions but probably on other places too), instead of testing the axis as in comment #48, it's preferred to use this form:

if (event.HORIZONTAL_AXIS!=undefined && event.axis==e.HORIZONTAL_AXIS){ <horizontal axis handling code> }

or else, the test in comment #48 will always yield true (both tested values are undefined and in this case, will always execute horizontal handler) when, in fact, the test should be skipped because the attribute is not supported.
Flags: wanted1.9.1? → wanted1.9.1+
Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090123 Shiretoko/3.1b3pre
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090123 Minefield/3.2a1pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: