350471 - Reenable pixel scrolling (two-finger touchpad)

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P2)

Description

[Mark Mentovai]

We had to turn off pixel scrolling in bug 347626 due to problems with XUL trees and listboxes, and some web apps.  The exact problems are described in detail in that bug.  We should come up with some kind of plan to intelligently support pixel-scrolling, at least for Gecko content areas.

Comment 1

[Damon Sicore (:damons)]

Any chance the disabled code now behaves differently as we are using cocoa?  I think the lack of one pixel scrolling on mac is something we really should fix--almost as much as any theme change.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

I have no idea what this mac-style pixel-scrolling is about, but NS_MOUSE_SCROLL
event handling has also changed in 1.9 so that it is processed like any other 
event.
If there are problems handling xul trees/listboxes and web apps, perhaps 
nsEventStateManager::DoScrollText could check those and do pixel scrolling
only on main content area (only top level scrolling view of a view manager).

Comment 3

[Mark Mentovai]

Cocoa natively uses a different model to get scroll events, one which is probably slightly harder to work cleanly into Gecko.

Comment 4

[Markus Stange [:mstange]]

Attachment: Fix v0.8: First try

This is going to rock!

This patch makes pixel scrolling work, without any problems for trees or Google Maps. However, there are several drawbacks:

1. Blocking 0-line scroll events from reaching Google Maps is quite hacky. However, I don't know where it should be put instead.
2. Our chrome can't access the pixel deltas. We should rethink our DOMMouseScroll implementation.
3. Calling deviceDeltaX/Y when the device does not support it prints an assertion. I don't know how to get rid of it.

Other than that, it works, and I can't live without it any more. :)

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 302030 [details] [diff] [review]
Fix v0.8: First try

>+    case NS_MOUSE_SCROLL_EVENT:
>+      // Scroll events with .delta==0 should not be sent as DOMEvent. Doing
>+      // so breaks Google Maps.
>+      // XXX Is this the right place to block DOMEvents?
>+      if (static_cast<nsMouseScrollEvent*>(aEvent)->delta==0)
>+        return NS_ERROR_DOM_NOT_SUPPORTED_ERR;
>+      return NS_NewDOMMouseEvent(aDOMEvent, aPresContext,
>+                                 static_cast<nsInputEvent*>(aEvent));
>+      break;
This is way too ugly hack. Why Google Maps needs special handling?

Could you re-use .delta for deltaPixels and then depending on some
preference interpret the value correctly?

Comment 6

[Markus Stange [:mstange]]

Attachment: Example data

Comment 7

[Markus Stange [:mstange]]

Attachment: Illustration of example data

Comment 8

[Markus Stange [:mstange]]

(In reply to comment #5)
> Why Google Maps needs special handling?

When Google Maps receives a mouse scroll event, it checks event.detail. When
event.detail is > 0, it zooms out, otherwise it zooms in. So it even zooms in
when event.detail == 0.
You could say that this is a bug in Google Maps. Making Google fix their
behaviour would certainly be more desirable than introducing a hack into our
code.

Webkit blocks 0-delta scroll events as well, see
WebCore/dom/EventTargetNode.cpp, line 499:
http://www.koders.com/cpp/fidF29B9E177EF238EFCE90ACF9644357142305517D.aspx#L499

> Could you re-use .delta for deltaPixels and then depending on some
> preference interpret the value correctly?

This is exactly what I was going to avoid. I'll try to explain.

Mozilla's scrolling architecture is used to dealing with line scroll events.
Some widgets, like trees, can only be scrolled by line.

When pixel scrolling was implemented last time (bug 319078), this led to
problems: trees did a line scroll when they should only do a pixel scroll
(which they can't). This was caused by the fact that nsMouseScrollEvent::delta,
and likewise nsDOMMouseEvent::detail, was reused with a different meaning. Of
course, the kIsPixels flag was meant to differentiate between pixel and
line scrolls, but due to the fact that some parts of mozilla can't handle pixel
scrolls, this did not work reliably.

That's why I chose another approach (which happens to be the approach Cocoa
uses).

nsMouseScrollEvent::delta should keep its meaning, that is, it contains the
amount of lines to scroll. This way, everything that works now (like trees)
will keep working. And widgets that know how to scroll by pixel can be updated
incrementally to check for kHasPixels and utilise deltaPixels when they want
to.

As you can see, this leads to a problem: We now need to create an event for
every pixel that has been scrolled, even if there haven't been enough scrolls
to amount for a line scroll. Thus, most of the pixel scroll events carry a
delta that is zero.
Ideally, this shouldn't be a problem - if delta==0, you just don't scroll.
However, Google Maps gets it wrong and zooms in regardless.

And there is one more problem. Currently, for every nsMouseScrollEvent, a
nsDOMMouseEvent is created, with domevent.detail = nsEvent->delta. So on the
way to the DOMEvent, deltaPixels is lost. This means that even things like the
tabbar, which could support pixel scrolling easily, can't scroll by pixel
because they have no access to the data. It's the same for trees, should they
support pixel scrolling one day.

So what can be done? I can imagine three alternatives.

One alternative is the one I implemented (sadly, with an ugly hack): Simply
block all events with delta==0 from being sent as DOMEvent. This means no
problems with Google Maps and no chance for supporting pixel scrolling in
things like the tabbar.

The second alternative would be to add an attribute to the DOMEvent interface
(like "deltaPixels"), and then mark events with delta==0 as chrome-only
DOMEvents. (Is that possible? Would it be hard to implement?)

The third alternative is to switch to an IE/Safari compatible implementation:
- "DOMMouseScroll" --> "mousewheel"
- event.detail     --> event.wheelDelta
Morover, we would add the wheelDeltaPixels attribute, document it, and expose
it to scripts on the web. Then Google might be attracted to fixing Google Maps,
because smooth zooming is quite nice. ;)

Let me hear your thoughts.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

What about a new event for pixel scrolling:
NS_MOUSE_PIXEL_SCROLL/DOMMousePixelScroll?
.detail / .delta would mean then pixels
nsEventStateManager could check whether the current scrollable view supports
pixel scrolling.

Events could be dispatched perhaps so that if there is normal scrolling,
dispatch DOMMouseScroll and if there is pixel scrolling, dispatch another event.
So in case of line-scrolling, only DOMMouseScroll, and in case of pixel-scrolling, dispatch either only DOMMousePixelScroll, or if the line delta != 0, dispatch first DOMMouseScroll and then additional DOMMousePixelScroll.
This all depends on how OSX generates those events.
Note, because I don't have a mac, I can't test this at all.

Comment 10

[Markus Stange [:mstange]]

(In reply to comment #9)
> What about a new event for pixel scrolling:
> NS_MOUSE_PIXEL_SCROLL/DOMMousePixelScroll?
> .detail / .delta would mean then pixels
> nsEventStateManager could check whether the current scrollable view supports
> pixel scrolling.

Right now I can't think of an easy way to do this. Remember, the nsEvent is created in nsChildView.mm. nsChildView.mm does not know whether the current scrollable view supports pixel scrolling. 
The decision what event to dispatch is up to nsEventStateManager. But in order to be able to send the appropriate event, nsEventStateManager needs to know both delta and deltaPixels. So we need to send both deltas anyway and can't simple reuse .delta.

> So in case of line-scrolling, only DOMMouseScroll, and in case of
> pixel-scrolling, dispatch either only DOMMousePixelScroll, or if the line
> delta != 0, dispatch first DOMMouseScroll and then additional
> DOMMousePixelScroll.

I don't think I understand what you are trying to say here. If we dispatched both DOMMouseScroll and DOMMousePixelScroll, we'd end up scrolling twice.

Take a look at the first 4 events in the example data in the attachments.
Let's start at scroll position 0px, and let's assume one line equals 5 pixels.

In the first three events, deltaPixels == 1px and delta == 0.
--> New scroll position: (1 + 0) + (1 + 0) + (1 + 0) = 3.
In the fourth event, enough pixel scrolls have been accumulated to make up for a line scroll, so delta == 1.
--> New scroll position: 3 + (2 + 1*5) = 10. Intended scroll position: 5.
This is certainly not what we want to have.

Dispatching both events is not an option. In bug 347626, Mark tried to make this work by using the following approach:
When a pixel scroll has occured, send a pixel scroll event. If dispatching this event fails, send a line scroll event.
However, I think this makes things unnecessarily complex and we're much better off using the approach I outlined.

> This all depends on how OSX generates those events.
> Note, because I don't have a mac, I can't test this at all.

The table of "example data" and the graph should make clear what OSX does.

Comment 11

[Markus Stange [:mstange]]

Hah! Now I see the solution. You're right, introducing a new event is the key. We can simple mark all line scroll events that are pixel scroll replacement events as such.
I'll try to come up with a patch soon.

Comment 12

[Josh Aas]

Comment on attachment 302030 [details] [diff] [review]
Fix v0.8: First try

Assuming you don't want my review any more since you're going to create a new patch.

Comment 13

[Markus Stange [:mstange]]

Attachment: Fix v0.9: Introduce new event

Well, this has turned out to be more challenging than I anticipated.

The current strategy is like this:
- Cocoa reports a scroll event
  - Is it a line scroll event?
    - Yes:
      - Send an NS_MOUSE_SCROLL event to gecko
    - No:
      - Send an NS_MOUSE_PIXEL_SCROLL event to gecko
      - If delta != 0:
         - Send an NS_MOUSE_SCROLL event to gecko, marked with kHasPixels
           so that it doesn't get dispatched by pixel scrollable targets
           (we don't want to scroll twice, see previous comments)

This works quite well. Trees work, Google Maps work, I don't see any ugly hacks.

However:
1. I still haven't been able to figure out how to get rid of the failing
   assertion. There doesn't seem to be a way I can predict it. I need help
   here.
2. I haven't created a DOMEvent for NS_MOUSE_PIXEL_SCROLL yet. I think
   this should be done in a follow-up bug.

The thing is: I could implement a DOMMousePixelScroll event the same way the DOMMouseScroll event is implemented, but that wouldn't be of any use - DOMMouseScroll events haven't got a "hasPixels" attribute like nsMouseScroll, so there's again the problem of scrolling twice.

So DOMMouseScroll needs to get a new attribute, "hasPixels". And, for that matter, an attribute "axis" as well, so we're able to fix wrong-axis-bugs like bug 378028.
Why is this a problem?
Currently, DOMMouseScroll inherits from nsIDOMMouseEvent and thus from nsIDOMUIEvent, so it gets the "detail" attribute which we use to store the delta in. However, now we need an attribute that hasn't been declared there yet. As far as I can see, in order to be able to extend this interface, we have to create a new IDL-file and do the XPConnect thing... is this correct?
That sounds like a major undertaking.

My current plan is to restrict this bug to the functionality that my patch already provides (ideally, without a failing assertion...).
Then I'll file new bugs:
- Extend DOMMouseScroll event by axis and hasPixels attribute
- Add DOMMousePixelScroll event
- Make the toolkit scrollbox widget understand pixel scrolling
- the same for trees and listboxes

Do you think that's ok?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Don't add NS_MOUSE_PIXEL_SCROLL_EVENT. You're not adding new nsEvent structure 
type anyway. NS_MOUSE_PIXEL_SCROLL is enough, it uses nsMouseScrollEvent

I don't think bug 378028 has anything to do with this. nsMouseScrollEvent
may contain kIsVertical/kIsHorizontal flags.

If the device supports pixel scrolling, in which case deltaX is 0 but 
deviceDeltaX != 0? I mean in practice - what is the difference in user 
interaction. (I really should get a mac for testing)
Would it be terrible bad to always dispatch MouseScroll, so that
if deltaX is 0, but deviceDeltaX > 0, deltaX = 1 (or -1 if deviceDeltaX < 0)?
EventStateManager shouldn't handle those MouseScroll events, because they have 
the kHasPixels flag, but content (== web page/chrome) could still get a 
normal-like MouseScroll event.
... I'm just trying to figure out the least-regression-risky way to implement
this.

> 3. Calling deviceDeltaX/Y when the device does not support it prints an
> assertion. I don't know how to get rid of it.
Please figure out how to get rid of this.

More comments and questions perhaps coming...

Comment 15

[Markus Stange [:mstange]]

Attachment: Fix v0.10: Get rid of failed assertions

I've finally figured out how to prevent the failed assertions from being printed to the console. I had to define a custom NSAssertionHandler that ignores all errors and declare that as the thread's new default assertion handler.

(In reply to comment #14)
> Don't add NS_MOUSE_PIXEL_SCROLL_EVENT. You're not adding new nsEvent structure 
> type anyway. NS_MOUSE_PIXEL_SCROLL is enough, it uses nsMouseScrollEvent

First I had added nsMousePixelScrollEvent but then I figured that I could simply reuse nsMouseScrollEvent. I think we do need NS_MOUSE_PIXEL_SCROLL_EVENT. Otherwise, the pixel scroll events would get dispatched as DOMMouseScroll events, which would be bad.

> I don't think bug 378028 has anything to do with this. nsMouseScrollEvent
> may contain kIsVertical/kIsHorizontal flags.

nMouseScrollEvent does - nsDOMMouseEvent doesn't. However, scrolling trees works via DOM events.
See http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/content/widgets/tree.xml&rev=1.54&root=/cvsroot#627


> If the device supports pixel scrolling, in which case deltaX is 0 but 
> deviceDeltaX != 0? I mean in practice - what is the difference in user 
> interaction. (I really should get a mac for testing)

Hu?

> Would it be terrible bad to always dispatch MouseScroll, so that
> if deltaX is 0, but deviceDeltaX > 0, deltaX = 1 (or -1 if deviceDeltaX < 0)?

I think it would be quite bad, and moreover, I don't think it is necessary.
deltaX:       number of lines. 1 line == 16 pixels (at least that's what I get)
deviceDeltaX: number of pixels.

If we scrolled for a line every time we received a pixel scroll, that would be way too fast.

> EventStateManager shouldn't handle those MouseScroll events, because they have 
> the kHasPixels flag, but content (== web page/chrome) could still get a 
> normal-like MouseScroll event.

Way too often. That's what caused bug 347626.
And with my patch, they do receive "normal-like" MouseScroll events - again, have a look at the illustration I uploaded, that should make it clear.

> ... I'm just trying to figure out the least-regression-risky way to implement
> this.

I'm pretty sure that what I'm doing now is quite regression-free. We can't use pixel scroll event in DOM-content yet, but that's not a regression.

> > 3. Calling deviceDeltaX/Y when the device does not support it prints an
> > assertion. I don't know how to get rid of it.
> Please figure out how to get rid of this.
Done. :)

OK, please keep the questions coming, I really want to get this in. ;)

Comment 16

[Markus Stange [:mstange]]

Attachment: Fix v0.11: remove garbage, update comments

Comment 17

[Markus Stange [:mstange]]

> have a look at the illustration I uploaded, that should make it clear.

Ah I see, that could be misleading... I used different scales for delta and deltaPixels there, so that's why you could think a delta of 1/-1 wouldn't be different from a pixel scroll with an equal deltaPixels...

However, mapping all positive/negative deltas to 1/-1 would certainly result in a loss of precision and in a perceivable difference to native Cocoa apps.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 302802 [details] [diff] [review]
Fix v0.11: remove garbage, update comments

>+#define NS_MOUSE_PIXEL_SCROLL_EVENT       17
Still this? Remove it, since you don't have the new event struct type.
And I don't think there is any need for a new struct type.

>+  case NS_MOUSE_PIXEL_SCROLL:
>+    {
>+      if (mCurrentFocus) {
>+        mCurrentTargetContent = mCurrentFocus;
>+      }
>+    }
>+    break;
How should pixel scroll work if Ctrl, Shift or some other modifier is pressed?
What does OSX do in those cases? What does Safari do?

About that assertion, please ask Josh or someone to make sure it is ok.

> I think we do need
> NS_MOUSE_PIXEL_SCROLL_EVENT. Otherwise, the pixel scroll events would get
> dispatched as DOMMouseScroll events, which would be bad.
No. DOM events aren't even created for NS_MOUSE_PIXEL_SCROLL, because
there isn't just any need for that. Scripts may listen DOMMousePixelScroll, but
because nsEvent->message <-> nsDOMEvent::GetType conversion is missing, scripts don't
get access to NS_MOUSE_PIXEL_SCROLL events.

Comment 19

[Markus Stange [:mstange]]

Attachment: Fix v0.12: address review comments

(In reply to comment #18)
> (From update of attachment 302802 [details] [diff] [review])
> >+#define NS_MOUSE_PIXEL_SCROLL_EVENT       17
> Still this? Remove it, since you don't have the new event struct type.

Oops! Now I finally understand. Sorry.


> >+  case NS_MOUSE_PIXEL_SCROLL:
> >+    {
> >+      if (mCurrentFocus) {
> >+        mCurrentTargetContent = mCurrentFocus;
> >+      }
> >+    }
> >+    break;
> How should pixel scroll work if Ctrl, Shift or some other modifier is pressed?
> What does OSX do in those cases? What does Safari do?

Usually, pressing a modifier key doesn't have any influence on scrolling. However, you can turn on screen zooming for a modifier key: http://docs.info.apple.com/article.html?artnum=304645
But when screen zooming is turned on for a modifier key, Cocoa doesn't send a scroll event while that key is pressed.
So Firefox now behaves the same way as Safari and other OS X applications.

Thanks for your patience!

Comment 20

[Markus Stange [:mstange]]

Attachment: Fix v1.0: fix line lengths

> <josh> mstange: those lines are a little long
Fixed.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Hmm, realized still one problem. DOMMouseScroll can be prevented (.preventDefault())
The new event can't be.
If we just could dispatch that DOMMouseScroll and check if it was prevented
and then not dispatch pixelscroll. Or only dispatch NS_MOUSE_SCROLL, 
(extended with pixel data), but .delta set always something else than 0.
But I understand that doesn't work. What would be the best solution?

And note, in gecko NS_MOUSE_SCROLL may be set up to do something else than
scroll. That is the reason for GetBasePrefKeyForMouseWheel in ESM.
What we really don't want is for example that ctrl+NS_MOUSE_SCROLL zooms-in
but because there is also NS_MOUSE_PIXEL_SCROLL dispatched, page is scrolled too.

Sorry for not thinking all these immediately, but there are quite many cases the mouse scrolling needs to handle.

Comment 22

[Markus Stange [:mstange]]

Ah well, you're right again.

> What we really don't want is for example that ctrl+NS_MOUSE_SCROLL zooms-in
> but because there is also NS_MOUSE_PIXEL_SCROLL dispatched, page is scrolled
> too.

That's exactly what's happening.

I'll have a look at what Safari is doing.

Comment 23

[Markus Stange [:mstange]]

Safari is doing the same as we are - pixel scroll events aren't cancelable.

So how can we fix the zoom problem? Currently, I can only think of one solution: creating a DOMEvent for NS_MOUSE_PIXEL_SCROLL. Page zooming could simply .preventDefault() all of them as long as the zoom modifier key is pressed.

But in order to make good use of DOMMousePixelScroll events, there needs to be a hasPixels attribute for DOMMouseScroll. So I can't really see a way around extending DOMMouseScroll.

What do you think?

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #23)
> Safari is doing the same as we are - pixel scroll events aren't cancelable.
Unfortunately DOMMouseScroll has always been cancelable. So some content or 
extensions probably relies on that behavior.

> So how can we fix the zoom problem? Currently, I can only think of one
> solution: creating a DOMEvent for NS_MOUSE_PIXEL_SCROLL. Page zooming could
> simply .preventDefault() all of them as long as the zoom modifier key is
> pressed.
Just for that reason the DOM Event isn't needed. You can always set NS_EVENT_FLAG_NO_DEFAULT and nsEventStatus_eConsumeNoDefault in EventStateManager::PreHandleEvent or ::PostHandleEvent, whatever effect is wanted.

Still thinking how to get this working without breaking too many things.

Comment 25

[Markus Stange [:mstange]]

> > So how can we fix the zoom problem? Currently, I can only think of one
> > solution: creating a DOMEvent for NS_MOUSE_PIXEL_SCROLL. Page zooming could
> > simply .preventDefault() all of them as long as the zoom modifier key is
> > pressed.
> Just for that reason the DOM Event isn't needed. You can always set
> NS_EVENT_FLAG_NO_DEFAULT and nsEventStatus_eConsumeNoDefault in
> EventStateManager::PreHandleEvent or ::PostHandleEvent, whatever effect is
> wanted.
> 

Aha, ok. I think I've found out by now how to get zoom to work again without introducing the DOMMousePixelScroll event, a new patch is in the works.

The problem with the extensions is really tricky. I'm thinking about it too... the thing is, the first pixel scroll event is sent before the first line scroll event, so we can't just test if the last line scroll event has been canceled - which would also have been pretty unstable.

Comment 26

[Markus Stange [:mstange]]

Attachment: Fix v1.1: Don't scroll while zooming

OK. What I'm doing here is to scroll pixels only when the standard scroll would have been a line scroll (and not a zoom, history scroll, or page scroll).

This doesn't solve the preventDefault() problem.

Comment 27

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

Assigning this to Markus because he's generating patches :)

Comment 28

[Markus Stange [:mstange]]

Attachment: Fix v2.0: added DOMMouseScroll event and scrollFlags attribute

(In reply to comment #24)
> (In reply to comment #23)
> > Safari is doing the same as we are - pixel scroll events aren't cancelable.
> Unfortunately DOMMouseScroll has always been cancelable. So some content or 
> extensions probably relies on that behavior.

Well, let's break them. After all, that's what extension compatibility checks are for. I can't think of a different way of implementing this that wouldn't break them.
However, if we break extensions, we must give them a way to be fixed. So I introduced the DOMMousePixelScroll event, which can be .preventDefault()ed.
And in order to make proper use of DOMMousePixelScroll events, there has to be a way to tell if the DOMMouseScroll events are replacement events (and thus should be ignored when interpreting pixel scroll events). So the DOM event needs to carry the scrollFlags.
That's why I added nsIDOMMouseScrollEvent.idl (and the rest of the cruft that needs to be adjusted when adding a new DOM event...).

Giving DOM content access to the scrollFlags is actually really great, as they carry the scroll axis as well.

And slowly, but surely, this rocks.

Comment 29

[Markus Stange [:mstange]]

Attachment: Playground for scroll events

Comment 30

[Markus Stange [:mstange]]

Attachment: Example screenshot with patch

Comment 31

[Markus Stange [:mstange]]

Attachment: Fix v2.0, diff updated to trunk

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Sorry about not reviewing this yet, but since this isn't blocking1.9+ and I've
been quite busy with other things... :(
(And I'm not yet absolutely sure the solution is the right one. I'm trying to
avoid all possible regressions this late in 1.9)

Comment 33

[Markus Stange [:mstange]]

No problem. However, the sooner we break compatibility, the better...

Comment 35

[Markus Stange [:mstange]]

Comment on attachment 305390 [details] [diff] [review]
Fix v2.0, diff updated to trunk

Marking as obsolete, since smaug and I decided to try another approach that hopefully does not break anything.

In comment 25, I said:

> the thing is, the first pixel scroll event is sent before the first line scroll
> event, so we can't just test if the last line scroll event has been canceled

However that's exactly what we're going to do. We're taking over the decision when a line scroll is sent, so we can just send it before the first pixel scroll.
When the line scroll event has got through without being preventDefault()ed, we unlock a certain amount of pixels that may be scrolled by subsequent pixel scroll events. If the line scroll has been canceled, we consume those pixel scroll events without sending them into Gecko.

Comment 36

[Markus Stange [:mstange]]

Attachment: Fix v3.0: Don't break anything, add pref

This is the master patch.

1. It honors .preventDefault().
2. It introduces a pref (mousewheel.enabel_pixel_scrolling) to address bug 351105.
3. Otherwise, it feels equally awesome.

That means the patch passes the gmaps-iframe test (in contrast to Safari): http://mozilla.pettay.fi/moztests/gmaps.html

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Markus, thanks for the patch and I hope we can eventually take it or something like it, but I need to let you know that it's too late to take a change like this for Firefox3.

Comment 38

[Markus Stange [:mstange]]

That's too bad. I don't like that choice but... well, ok.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Olli says that the DOM3 Events working group is going to specify a standard mousewheel event. If we make sure that that event supports pixel scrolling, that will be the way to to go for the long term.

Markus, right now I'm stealing part of this patch for bug 378028 --- the nsDOMMouseScrollEvent class and related code, but not the actual pixel scrolling event. We can add the pixel scrolling event code post-1.9 (which is very very soon now!). I have made one change which is that instead of exposing scrollFlags directly to Javascript, I'm exposing an "axis" attribute.

Comment 40

[Markus Stange [:mstange]]

That's great!

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

IMO this might be possible for 1.9.1. Pixel scrolling isn't yet defined in
DOM 3 Events, but I hope the draft will be updated within next few weeks.
And if not, we could use DOMMousePixelScroll and add the standard event later.

Comment 43

[Markus Stange [:mstange]]

Comment on attachment 311060 [details] [diff] [review]
Fix v3.0: Don't break anything, add pref

Cool!
I'll create a new patch when bug 378028 lands.

Comment 44

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Not blocking, but really wanted

Comment 45

[Markus Stange [:mstange]]

Attachment: Fix v4.0: Updated patch

This patch is on top of that for bug 378028.

Short summary of what this patch does:
 - Adds NS_MOUSE_PIXEL_SCROLL / DOMMousePixelScroll event type
 - Adds isFallback attribute for DOMMouseScroll events
     If this is true it means that you shouldn't process this line scroll
     event if you're processing pixel scroll events (to avoid double
     scrolling).
 - Makes .preventDefault() on line scroll events cancel subsequent pixel scroll
   events
 - Adds Mac OS X implementation

Comment 46

[Markus Stange [:mstange]]

Comment on attachment 332786 [details] [diff] [review]
Fix v4.0: Updated patch

Steven, could you review the widget/cocoa part?

Comment 47

[Markus Stange [:mstange]]

Attachment: Updated event playground

Comment 48

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 332786 [details] [diff] [review]
Fix v4.0: Updated patch

Please use -p when generating patches.

>+NS_IMETHODIMP
>+nsDOMMouseScrollEvent::GetIsFallback(PRBool* aResult)
>+{
>+  NS_ENSURE_ARG_POINTER(aResult);
>+
>+  if (mEvent->eventStructType == NS_MOUSE_SCROLL_EVENT) {
>+    PRUint32 flags = static_cast<nsMouseScrollEvent*>(mEvent)->scrollFlags;
>+    *aResult = (flags & nsMouseScrollEvent::kHasPixels);
This may assign non-zero-or-one to *aResult.
*aResult = !!(flags & nsMouseScrollEvent::kHasPixels) is better
>+  } else {
>+    *aResult = PR_FALSE;
>+  }
>+  return NS_OK;
But I'm not sure about the isFallback, do we really need it in DOM?


>-    mTabbedThroughDocument(PR_FALSE)
>+    mTabbedThroughDocument(PR_FALSE),
>+    mLastLineScrollSucceededX(PR_TRUE),
>+    mLastLineScrollSucceededY(PR_TRUE)
So did you try to add some code which clears mLastLineScrollSucceededX/Y when focusing out
from the ESM?

>+      // When the last line scroll has been canceled, eat the pixel scroll event
>+      nsMouseScrollEvent *msEvent = (nsMouseScrollEvent*) aEvent;
>+      if (msEvent->scrollFlags & nsMouseScrollEvent::kIsHorizontal) {
>+        if (!mLastLineScrollSucceededX)
>+          *aStatus = nsEventStatus_eConsumeNoDefault;
>+      } else if (msEvent->scrollFlags & nsMouseScrollEvent::kIsVertical) {
>+        if (!mLastLineScrollSucceededY)
>+          *aStatus = nsEventStatus_eConsumeNoDefault;
>+      }
>+    }
I prefer {...} after |if|

> interface nsIDOMMouseScrollEvent : nsISupports
> {
>   const long HORIZONTAL_AXIS = 1;
>   const long VERTICAL_AXIS = 2;
> 
>   readonly attribute long axis;
>+  readonly attribute boolean isFallback;
If we have isFallback attribute, it really should be documented properly.
And the name isn't perhaps the best :/

> class nsMouseScrollEvent : public nsMouseEvent_base
> {
> public:
>   enum nsMouseScrollFlags {
>     kIsFullPage =   1 << 0,
>     kIsVertical =   1 << 1,
>     kIsHorizontal = 1 << 2,
>-    kIsPixels =     1 << 3
>+    kHasPixels =    1 << 3
>   };
Please document what kHasPixels means

>+  if (scrollDelta != 0) {
>+    // Send line scroll event
>+    nsMouseScrollEvent geckoEvent(PR_TRUE, NS_MOUSE_SCROLL, nsnull);
>+    [self convertCocoaMouseEvent:theEvent toGeckoEvent:&geckoEvent];
>+    geckoEvent.scrollFlags |= inAxis;
>+
>+    if (hasPixels) {
>+      // Mark as pixel scroll replacement
>+      geckoEvent.scrollFlags |= nsMouseScrollEvent::kHasPixels;
>+    }
But what if scrollDeltaPixels is 0? Do we still want to add kHasPixels flag?
The DOMMousePixelScroll event isn't dispatched in that case.

Comment 49

[Markus Stange [:mstange]]

(In reply to comment #48)
> (From update of attachment 332786 [details] [diff] [review])
> Please use -p when generating patches.

Yeah, sorry, I didn't know how to do that with qdiff since it doesn't accept -p. But now I've found that you can generate mq queue patches with hg diff, too :-)

> 
> >+NS_IMETHODIMP
> >+nsDOMMouseScrollEvent::GetIsFallback(PRBool* aResult)
> >+{
> >+  NS_ENSURE_ARG_POINTER(aResult);
> >+
> >+  if (mEvent->eventStructType == NS_MOUSE_SCROLL_EVENT) {
> >+    PRUint32 flags = static_cast<nsMouseScrollEvent*>(mEvent)->scrollFlags;
> >+    *aResult = (flags & nsMouseScrollEvent::kHasPixels);
> This may assign non-zero-or-one to *aResult.
> *aResult = !!(flags & nsMouseScrollEvent::kHasPixels) is better

OK.

> >+  } else {
> >+    *aResult = PR_FALSE;
> >+  }
> >+  return NS_OK;
> But I'm not sure about the isFallback, do we really need it in DOM?

Without isFallback the pixel scroll events are basically unusable (the double-scroll problem), so I think we do need it.

> >-    mTabbedThroughDocument(PR_FALSE)
> >+    mTabbedThroughDocument(PR_FALSE),
> >+    mLastLineScrollSucceededX(PR_TRUE),
> >+    mLastLineScrollSucceededY(PR_TRUE)
> So did you try to add some code which clears mLastLineScrollSucceededX/Y when
> focusing out
> from the ESM?

I didn't because I forgot why I should do it. What was the use case again?

And I made some experiments. If you leave the google maps zoom area after zooming and try to scroll, only the first 3 or so pixels of your scroll gesture are ignored which you don't really notice.

> 
> >+      // When the last line scroll has been canceled, eat the pixel scroll event
> >+      nsMouseScrollEvent *msEvent = (nsMouseScrollEvent*) aEvent;
> >+      if (msEvent->scrollFlags & nsMouseScrollEvent::kIsHorizontal) {
> >+        if (!mLastLineScrollSucceededX)
> >+          *aStatus = nsEventStatus_eConsumeNoDefault;
> >+      } else if (msEvent->scrollFlags & nsMouseScrollEvent::kIsVertical) {
> >+        if (!mLastLineScrollSucceededY)
> >+          *aStatus = nsEventStatus_eConsumeNoDefault;
> >+      }
> >+    }
> I prefer {...} after |if|

OK.

> 
> > interface nsIDOMMouseScrollEvent : nsISupports
> > {
> >   const long HORIZONTAL_AXIS = 1;
> >   const long VERTICAL_AXIS = 2;
> > 
> >   readonly attribute long axis;
> >+  readonly attribute boolean isFallback;
> If we have isFallback attribute, it really should be documented properly.

OK. Where? On MDC? At the nsMouseScrollFlags enum?

> And the name isn't perhaps the best :/

Yeah. Is hasPixels better? Or should I rename kHasPixels to kIsFallback?

> 
> > class nsMouseScrollEvent : public nsMouseEvent_base
> > {
> > public:
> >   enum nsMouseScrollFlags {
> >     kIsFullPage =   1 << 0,
> >     kIsVertical =   1 << 1,
> >     kIsHorizontal = 1 << 2,
> >-    kIsPixels =     1 << 3
> >+    kHasPixels =    1 << 3
> >   };
> Please document what kHasPixels means

OK.

> 
> >+  if (scrollDelta != 0) {
> >+    // Send line scroll event
> >+    nsMouseScrollEvent geckoEvent(PR_TRUE, NS_MOUSE_SCROLL, nsnull);
> >+    [self convertCocoaMouseEvent:theEvent toGeckoEvent:&geckoEvent];
> >+    geckoEvent.scrollFlags |= inAxis;
> >+
> >+    if (hasPixels) {
> >+      // Mark as pixel scroll replacement
> >+      geckoEvent.scrollFlags |= nsMouseScrollEvent::kHasPixels;
> >+    }
> But what if scrollDeltaPixels is 0?

During my tests, deviceDeltaX/Y was always non-zero (if they exist; if they don't, hasPixels == false). But I can add another check so we're safe.

Comment 50

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #49)
> Without isFallback the pixel scroll events are basically unusable (the
> double-scroll problem), so I think we do need it.
Hmm, right. But the name is a bit strange.

> I didn't because I forgot why I should do it. What was the use case again?
Scrolling and changing tab and then back to the original tab.

> And I made some experiments. If you leave the google maps zoom area after
> zooming and try to scroll, only the first 3 or so pixels of your scroll gesture
> are ignored which you don't really notice.
But if even those 3 pixels could be there with a small change to the patch....

> > > interface nsIDOMMouseScrollEvent : nsISupports
> > > {
> > >   const long HORIZONTAL_AXIS = 1;
> > >   const long VERTICAL_AXIS = 2;
> > > 
> > >   readonly attribute long axis;
> > >+  readonly attribute boolean isFallback;
> > If we have isFallback attribute, it really should be documented properly.
> 
> OK. Where? On MDC? At the nsMouseScrollFlags enum?
In this interface.

> Yeah. Is hasPixels better? Or should I rename kHasPixels to kIsFallback?
Neither of those, isFallback or hasPixels, say much to the user of the interface. Hard to say what could be a good name.

Comment 51

[Markus Stange [:mstange]]

Comment on attachment 332786 [details] [diff] [review]
Fix v4.0: Updated patch

On IRC smaug and I decided to drop the isFallback attribute and make the EventStateManager dispatch additional DOMMousePixelScroll events in that case.

Comment 52

[Markus Stange [:mstange]]

Attachment: updated patch

Comment 53

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 333929 [details] [diff] [review]
updated patch

>+  case NS_MOUSE_PIXEL_SCROLL:
>+    {
>+      if (mCurrentFocus) {
>+        mCurrentTargetContent = mCurrentFocus;
>+      }
>+
>+      // When the last line scroll has been canceled, eat the pixel scroll event
>+      nsMouseScrollEvent *msEvent = (nsMouseScrollEvent*) aEvent;
Use C++ static_cast<>

> nsresult
>+nsEventStateManager::SendDOMPixelScrollEvent(nsIFrame* aTargetFrame,
>+                                             nscoord aDelta,
>+                                             nsInputEvent* aEvent,
>+                                             nsPresContext* aPresContext)
>+{
>+  nsCOMPtr<nsIContent> targetContent = aTargetFrame->GetContent();
>+  if (!targetContent)
>+    GetFocusedContent(getter_AddRefs(targetContent));
>+  if (!targetContent) return NS_OK;
>+
>+  nsMouseScrollEvent event(*(reinterpret_cast<nsMouseScrollEvent*>(aEvent)));
Hmm, you rely on implicit copy-constructor, but does that know
how to handle nsCOMPtr members? I think it does - at least it should.
What I'm a bit worried that if someone adds a member variable which explicitly
needs AddRef/Release, this would be broken. But if you add enough tests
for pixel scrolling, testsuites should catch such error pretty easily.
Anyway, shouldn't static_cast<> be enough?
And make sure targetContent isn't #text node, but some element.

>+  nsCOMPtr<nsIDOMEvent> domEvent;
>+  NS_NewDOMMouseScrollEvent(getter_AddRefs(domEvent), aPresContext, &event);
>+  nsCOMPtr<nsIDOMMouseScrollEvent> domScrollEvent(do_QueryInterface(domEvent));
>+  nsCOMPtr<nsIPrivateDOMEvent> privateEvent(do_QueryInterface(domScrollEvent));
>+
>+  if (privateEvent && NS_IS_TRUSTED_EVENT(aEvent)) {
>+    privateEvent->SetTrusted(PR_TRUE);
>+  }
>+
>+  nsCOMPtr<nsIDOMEventTarget> target(do_QueryInterface(targetContent));
>+  if (target) {
>+    PRBool defaultActionEnabled;
>+    target->DispatchEvent(domEvent, &defaultActionEnabled);
If we want that pixelscroll actually causes the scrolling, the event should be dispatched
via presshell. See nsIPresShell::HandleEventWithTarget
That also means that there isn't need to explicitly create nsIDOMEvent.

>@@ -2199,16 +2251,21 @@ nsEventStateManager::DoScrollText(nsPres
...
>     if (lineHeight != 0) {
>+      if (aSendPixelScrollEvent && aScrollQuantity == eScrollByLine) {
>+        SendDOMPixelScrollEvent(aTargetFrame, aNumLines * lineHeight, aEvent,
>+                                aPresContext);
>+      }
>+
I think DoScrollText should really scroll the text, not fire pixelscrollevent.
To get the lineHeight info from scrollview, you could perhaps make a simple
helper method which returns the current scrollable view.


>@@ -2459,21 +2516,32 @@ nsEventStateManager::PostHandleEvent(nsP
...
>   case NS_MOUSE_SCROLL:
>+  case NS_MOUSE_PIXEL_SCROLL:
>+    nsMouseScrollEvent *msEvent = reinterpret_cast<nsMouseScrollEvent*>(aEvent);
Is reinterpret_cast really needed? Why not static_cast<>?


>@@ -830,17 +831,18 @@ public:
> 
> class nsMouseScrollEvent : public nsMouseEvent_base
> {
> public:
>   enum nsMouseScrollFlags {
>     kIsFullPage =   1 << 0,
>     kIsVertical =   1 << 1,
>     kIsHorizontal = 1 << 2,
>-    kIsPixels =     1 << 3
>+    kHasPixels =    1 << 3  // marks line scroll events that are provided as
>+                            // a fallback for pixel scroll events
If you still explain what "fallback" means in this context...

Comment 54

[Simon Strandman]

Hello

Firefox 3.1a2pre (checkout from today) fail to build with this patch and gcc 4.2. It works with gcc 4.0. This is the error:

g++-4.2 -o nsEventStateManager.o -c -I../../../dist/include/system_wrappers -include /Users/simon/Desktop/mozilla/firefox3.1/src/config/gcc_hidden.h -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_COM_OBSOLETE -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DZLIB_INTERNAL -DOSTYPE=\"Darwin9.4.0\" -DOSARCH=Darwin -D_IMPL_NS_LAYOUT -I/Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/../../base/src -I/Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/../../html/base/src -I/Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/../../xul/content/src -I/Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/../../xml/content/src -I/Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/../../../dom/src/base -I/Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/../../../layout/generic -I/Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/../../../layout/xul/base/src  -I/Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src -I. -I../../../dist/include/xpcom -I../../../dist/include/string -I../../../dist/include/dom -I../../../dist/include/js -I../../../dist/include/locale -I../../../dist/include/gfx -I../../../dist/include/thebes -I../../../dist/include/layout -I../../../dist/include/widget -I../../../dist/include/caps -I../../../dist/include/xpconnect -I../../../dist/include/webshell -I../../../dist/include/docshell -I../../../dist/include/pref -I../../../dist/include/view -I../../../dist/include/necko -I../../../dist/include/unicharutil -I../../../dist/include/imglib2 -I../../../dist/include/editor -I../../../dist/include   -I../../../dist/include/content -I../../../dist/include/nspr     -I../../../dist/sdk/include -I/usr/X11/include   -fPIC  -I/usr/X11/include -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-long-long -fno-strict-aliasing -fpascal-strings -fno-common -fshort-wchar -pthread -I/Developer/Headers/FlatCarbon -pipe  -DNDEBUG -DTRIMMED -O2  -I/usr/X11/include -DMOZILLA_CLIENT -include ../../../mozilla-config.h -Wp,-MD,.deps/nsEventStateManager.pp /Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/nsEventStateManager.cpp
/Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/nsEventStateManager.cpp: In member function ‘virtual nsresult nsEventStateManager::PreHandleEvent(nsPresContext*, nsEvent*, nsIFrame*, nsEventStatus*, nsIView*)’:
/Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/nsEventStateManager.cpp:1398: warning: enumeral mismatch in conditional expression: ‘nsIDOMNSUIEvent::<anonymous enum>’ vs ‘nsIDOMNSUIEvent::<anonymous enum>’
/Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/nsEventStateManager.cpp: In member function ‘virtual nsresult nsEventStateManager::PostHandleEvent(nsPresContext*, nsEvent*, nsIFrame*, nsEventStatus*, nsIView*)’:
/Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/nsEventStateManager.cpp:2624: error: jump to case label
/Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/nsEventStateManager.cpp:2525: error:   crosses initialization of ‘nsMouseScrollEvent* msEvent’
/Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/nsEventStateManager.cpp:2625: error: jump to case label
/Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/nsEventStateManager.cpp:2525: error:   crosses initialization of ‘nsMouseScrollEvent* msEvent’
/Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/nsEventStateManager.cpp:2631: error: jump to case label
/Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/nsEventStateManager.cpp:2525: error:   crosses initialization of ‘nsMouseScrollEvent* msEvent’
/Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/nsEventStateManager.cpp:2634: error: jump to case label
/Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/nsEventStateManager.cpp:2525: error:   crosses initialization of ‘nsMouseScrollEvent* msEvent’
/Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/nsEventStateManager.cpp:2736: error: jump to case label
/Users/simon/Desktop/mozilla/firefox3.1/src/content/events/src/nsEventStateManager.cpp:2525: error:   crosses initialization of ‘nsMouseScrollEvent* msEvent’
make[6]: *** [nsEventStateManager.o] Error 1
make[5]: *** [libs] Error 2
make[4]: *** [libs] Error 2
make[3]: *** [libs_tier_gecko] Error 2
make[2]: *** [tier_gecko] Error 2
make[1]: *** [default] Error 2
make: *** [build] Error 2

My mozconfig only has ".$topsrcdir/browser/config/mozconfig" and the compilers are from Xcode 3.1.

Comment 55

[Simon Howes]

You beat me to it Simon.  I can repro the same.

Comment 56

[Markus Stange [:mstange]]

Simons: Thanks, looks like there needs to be another set of curly braces around the stuff between "case NS_MOUSE_PIXEL_SCROLL:" and the corresponding "break;".

Comment 57

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 333929 [details] [diff] [review]
updated patch

>+  nsMouseScrollEvent event(*(reinterpret_cast<nsMouseScrollEvent*>(aEvent)));
Actually, because this copies .target, .originalTarget and .currentTarget, those
may point to wrong values when dispatching event.
So at least set all those values to nsnull before dispatching

Comment 58

[Markus Stange [:mstange]]

Attachment: updated patch

Comment 59

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 334493 [details] [diff] [review]
updated patch

Josh or someone should review the cocoa part, though I think it looks ok.

Comment 60

[Steven Michaud [:smichaud] (Retired)]

I'll try to review the cocoa part of attachment 334493 [details] [diff] [review] by the end of the week.

Comment 61

[Olli Pettay [:smaug][[e-mail]]]

we need some mochitests here.

Comment 62

[Simon Howes]

Builds fine with GCC 4.2 now.

Comment 63

[Steven Michaud [:smichaud] (Retired)]

(Reviewing Cocoa part of current patch, attachment 334493 [details] [diff] [review])

I've read through the patch several times, and am preparing to test
with it.

It mostly looks fine to me, but there's one thing that makes me very
uncomfortable -- permanently loading your own, empty,
NSAssertionHandler on the main thread.  As best I can tell, this stops
all Cocoa assertions (of any kind) from being logged (to the system
console) -- which may make certain kinds of problems/crashes harder to
debug.

In my tests I'm going to try to come up with alternative ways to stop
errors from being displayed when you send deviceDeltaX or deviceDeltaY
to an NSEvent generated from a device that doesn't support them.

You'll help me a lot, Markus, by telling me how to make these
assertions happen (after removing the code that loads your empty
NSAssertionHandler).

Comment 64

[Markus Stange [:mstange]]

(In reply to comment #63)
> It mostly looks fine to me, but there's one thing that makes me very
> uncomfortable -- permanently loading your own, empty,
> NSAssertionHandler on the main thread.  As best I can tell, this stops
> all Cocoa assertions (of any kind) from being logged (to the system
> console) -- which may make certain kinds of problems/crashes harder to
> debug.

Could be. Have there been crashes / problems in the past where an assertion was helpful?

> You'll help me a lot, Markus, by telling me how to make these
> assertions happen (after removing the code that loads your empty
> NSAssertionHandler).

I saw them with a Logitech MX Revolution on a Macbook with the default Logitech driver (Logitech Control Center) when using the mouse wheel.
Sorry that I don't have easier STR... :(

Comment 65

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #64)

> Have there been crashes / problems in the past where an assertion
> was helpful?

I don't remember any, off the top of my head.  But, on general
principles, I still think your empty NSAssertionHandler is a bad idea.

> I saw them with a Logitech MX Revolution on a Macbook with the
> default Logitech driver (Logitech Control Center) when using the
> mouse wheel.  Sorry that I don't have easier STR... :(

I've got an idea how to avoid using the empty NSAssertionHandler
... but it looks like I'm going to have to ask you to test it.  If I
can get it to work on my side, I'll post my code here for you to test
(hopefully in a few hours).

Comment 66

[Chris Lawson (gone)]

I happen to have one of those mice here, although I wasn't planning on installing that horrid excuse for a driver they call "Logitech Control Center", so I may be able to help test things too.

Comment 67

[Steven Michaud [:smichaud] (Retired)]

Attachment: Get rid of NSAssertionHandler

My idea worked!  Here's a version of Markus' latest patch (updated to
current trunk code) that gets rid of the replacement
NSAssertionHandler and the try/catch block around calls to [NSEvent
deviceDeltaX] and [NSEvent deviceDeltaY].

Markus, Chris and others, please try it out.

Turns out I _was_ able to test it (and to see that it gets rid of the
assertion and NSInternalInconsistency exception).  I have both a
Microsoft mouse (with a scroll wheel) and a Mighty Mouse.  Without the
NSAssertionHandler and try/catch block (and without my workaround),
using the Microsoft mouse's scroll wheel triggers the assertion and
exception (and crash).  My workaround gets rid of all this.

I'll continue my testing (and review) tomorrow.

Comment 68

[Markus Stange [:mstange]]

Nice!

Maybe add a comment along the lines of "For these events, the event kind will be kEventMouseWheelMoved instead of kEventMouseScroll."?

I haven't tested your patch yet, but it sounds pretty promising.

Comment 69

[Steven Michaud [:smichaud] (Retired)]

> Maybe add a comment along the lines of "For these events, the event
> kind will be kEventMouseWheelMoved instead of kEventMouseScroll."?

Makes sense.  Feel free to do this yourself, in the next update of
your patch.

Comment 70

[Steven Michaud [:smichaud] (Retired)]

Markus, please tell me how I should go about testing pixel scrolling
(as opposed to line scrolling).

What pages to visit (or parts of the browser chrome to hover over),
and what to do there.

I've got a Mighty Mouse and a MacBook Pro with a trackpad.

Comment 71

[Markus Stange [:mstange]]

0. Make sure that "smooth scrolling" is disabled:
   Preferences » Advanced » General » uncheck "Use smooth scrolling"

1. Go to any page that needs a scrollbar, e.g. this bug.
2. Hover your mouse over any scrollable part of the page, e.g. this comment.
3. Scroll very slowly using your trackpad.
4. Closely observe how the scroll position responds to your fingers' movement.

Results without pixel scrolling: "Falling down the stairs"
The first pixels of your scroll gesture don't result in any scrolling. When a certain threshold is crossed, the page's scroll position jumps at least 10px.

Results with pixel scrolling: "Taking the elevator"
Every slight movement of your fingers is accurately reflected in the scroll position. No jumping, but smooth, pixel-wise motions.

When testing with the Mighty Mouse, the difference is much more subtle: With pixel scrolling, the first "tick" of a wheel scroll gesture moves the scroll position by only one pixel, subsequent "ticks" move it by at least 10px.
Without pixel scrolling, there's no difference between the first tick of a scroll gesture and the rest of the ticks.

Comment 72

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 335578 [details] [diff] [review]
Get rid of NSAssertionHandler

Thanks, Markus, for your detailed description of how to test pixel
scrolling.  I found it very useful, and so will others who want to do
manual tests (like QA).  It should also provide clues how to write an
automated test.

My tests went as you predicted, and I had no problems.  The improved
responsiveness of two-fingered scrolling (on my MacBook Pro's
trackpad) was really quite dramatic.

I also went over the Cocoa code (in your patch) another time, and
found no problems.

I do think it makes sense, though, to follow your own suggestion from
comment #68, and expand the comment, in [ChildView scrollWheel:...],
above the following:

  if (carbonEventKind != kEventMouseScroll)
    checkPixels = PR_FALSE;

Comment 73

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #72)

Something I forgot to mention:  I tested both on OS X 10.5.4 (with my
Mighty Mouse) and on OS X 10.4.11 (with my MacBook Pro's trackpad).

Comment 74

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #72)

And another thing:  As best I can tell, bug 347626 hasn't been
re-regressed by this patch.

Comment 75

[Markus Stange [:mstange]]

Comment on attachment 335578 [details] [diff] [review]
Get rid of NSAssertionHandler

I'll work on tests soon.

Comment 76

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we should have some documentation somewhere describing what the strategy is here, and how events flow in the common cases ... it's a bit tricky.

+    kHasPixels =    1 << 3  // Marks line scroll events that are provided as
+                            // a fallback for pixel scroll events.
+                            // These scroll events are used by things that can't
+                            // be scrolled pixel-wise, like trees. You should
+                            // ignore them when processing pixel scroll events
+                            // to avoid double-processing the same scroll gesture.

I think this comment should say that when kHasPixels is set, we promise that there will be a follow-up event which contains pixel scrolling information.

Instead of mLastLineScrollSucceededX/Y, I'd call it mLastLineScrollConsumedX/Y. It's ambiguous whether "succeeded" means the event was consumed or it wasn't.

+      // Reset scroll event consumption.
+      mLastLineScrollSucceededX = mLastLineScrollSucceededY = PR_TRUE;

Why is this under NS_LOSTFOCUS? Why is it needed at all?

+            // We shouldn't scroll lines when the event is a replacement event
+            // for pixel scroll events.

I'd say, "we shouldn't scroll lines when a pixel scroll event will follow".

+              // There are no pixel scroll events for this line scroll event.
+              // We must create and send them now.

"No generated pixel scroll event will follow. Create and send a pixel-scroll DOM event now."

I'm not sure why we're doing that, though. Is this to make it easier to write client code, so that you can listen for pixel-scroll events and get them even if the platform doesn't support it?

Also, I know we haven't done this before, but should we call the event MozDOMPixelScroll? This is really something that's always going to be only for us...

Comment 77

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #76)
> Also, I know we haven't done this before, but should we call the event
> MozDOMPixelScroll? This is really something that's always going to be only for
> us...
Maybe, though, we have been adding new events using DOM prefix.
DOMContentLoaded, DOMMouseScroll, etc.

Comment 78

[Robert O'Callahan (:roc) (email my personal email if necessary)]

True, but those were added long ago and maybe were a mistake.

Comment 79

[Markus Stange [:mstange]]

Attachment: rev6, with tests

This patch contains lots of changes:
 - tests!
 - documented the event flow in nsGUIEvent.h
 - renamed DOMMousePixelScroll to MozMousePixelScroll
 - renamed mLastLineScrollSucceededX/Y to mLastLineScrollConsumedX/Y
 - updated comments (Thanks for rewriting them, roc!)
 - removed the NS_LOSTFOCUS reset: I think it's unnecessary, and smaug agreed
   to remove it when I told him that NS_LOSTFOCUS fires each time the map in
   Google Maps is clicked.
 - SendDOMPixelScrollEvent changes:
    - no longer fails if the mouse is over a text node (looks for the next
      non-text parent element)
    - no longer fails if the mouse isn't in a scrollable view
    - no longer uses the implicit copy constructor trick -
      MOZ_COUNT_CTOR(nsEvent) ended up not being called, so the leak stats
      were messed up
    - initializes the MozMousePixelScroll event's status with the
      DOMMouseScroll event's status
 - preventDefault on a DOMMouseScroll event without hasPixels no longer
   prevents the pixel scroll DOM event from being sent - now it only cancels
   its default handling
 - nsChildView changes:
    - removed nsIPrefBranch memory leak: Turns out that using a nsCOMPtr in an
      Objective C field doesn't go down too well because its destructor is
      never called.
    - defined mozkEventMouseScroll instead of kEventMouseScroll because of
      symbol conflicts when building on 10.5 without using the 10.4 SDK.
      According to Stuart Morgan, there's no simple future-proof way to #ifdef
      around it. (It would break on some combination of build and runtime SDK)

(In reply to comment #76)
> +      // Reset scroll event consumption.
> +      mLastLineScrollSucceededX = mLastLineScrollSucceededY = PR_TRUE;
> 
> Why is this under NS_LOSTFOCUS? Why is it needed at all?

The idea was that the mouse might be outside of a event-eating box after switching to another tab and back, so we shouldn't drop the pixels that come before the first line scroll. But it's not worth the trouble.

> "No generated pixel scroll event will follow. Create and send a pixel-scroll
> DOM event now."
> 
> I'm not sure why we're doing that, though. Is this to make it easier to write
> client code, so that you can listen for pixel-scroll events and get them even
> if the platform doesn't support it?

Right. And, unlike the nsMouseScrollEvent, DOMMouseScroll events don't have any kHasPixels flag, so you'd have no way of telling which events to ignore and which not. So the strategy is to ignore all DOMMouseScroll events and only process MozMousePixelScroll events.

> Also, I know we haven't done this before, but should we call the event
> MozDOMPixelScroll? This is really something that's always going to be only for
> us...

On IRC, smaug and I agreed on MozMousePixelScroll. Are you OK with that?

Comment 80

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 338307 [details] [diff] [review]
rev6, with tests

>+  PRBool isTrusted = !!(aEvent->flags & NS_EVENT_FLAG_TRUSTED);
>+  nsMouseScrollEvent event(isTrusted, NS_MOUSE_PIXEL_SCROLL, nsnull);
>+  event.refPoint = aEvent->refPoint;
>+  event.widget = aEvent->widget;
>+  event.time = aEvent->time;
>+  event.nativeMsg = aEvent->nativeMsg;
Hmm, I don't think we want to copy nativeMsg, although in practice in shouldn't
make any difference.

>+  void SendDOMPixelScrollEvent(nsIFrame* aTargetFrame,
>+                               nsMouseScrollEvent* aEvent,
>+                               nsPresContext* aPresContext,
>+                               nsEventStatus* aStatus);
Nit, maybe this could be called SendPixelScrollEvent

>+ * This event flow model satisfies several requirements:
>+ *  - DOMMouseScroll handlers don't need to know about the existence of pixel
>+ *    scrolling.
>+ *  - preventDefault on a DOMMouseScroll event results in no scrolling.
You could mention also MozMousePixelScroll here.

Comment 81

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 338307 [details] [diff] [review]
rev6, with tests

+  PRBool isTrusted = !!(aEvent->flags & NS_EVENT_FLAG_TRUSTED);

In most places I think we use != 0. Either way's fine.

+/* Mouse Scroll Events: Line Scrolling, Pixel Scrolling and Common Event Flows

Nice! I think you should add something here to mention that if a DOMMouseScroll event is preventDefaulted, then we suppress the associated following MozMousePixelScroll events.

Comment 82

[Markus Stange [:mstange]]

pushed: http://hg.mozilla.org/mozilla-central/rev/93f23e3efbb4

(In reply to comment #81)
> I think you should add something here to mention that if a DOMMouseScroll
> event is preventDefaulted, then we suppress the associated following
> MozMousePixelScroll events.

I added a comment that clarifies that they're sent but don't cause any scrolling.

Comment 83

[S Woodside]

Is there any way this could be dropped into 1.9.0? That way Camino 2.0 could pick it up, which would be a big win IMO.

Comment 84

[Jesse Ruderman]

This introduced some lag issues, which I think are covered by bug 418351.