512235 - bug 462809 should be implemented in content/events, not widget/src/*

Status: RESOLVED FIXED

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

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0

I suggested this bug in bug 462809 comment 37, but it's ignored :-(

Bug 462809 changes the widget/src/windows/nsWindow.cpp. However, it shouldn't be so. The bug wanted to make the consistency between platforms. So, the software acceleration must be implemented in XP level.

The attached patch ports the patch of bug 462809 to nsEventStateManager (ESM). By it, fixing following problems:

* bug 508747
* bug 462809 will be fixed on *all* platforms.
* The acceleration is *only* applied to the scrolling, so, not applied to zoom and DOM events.

All other works should be based on this implementation.

# The patch doesn't change the actual behavior on Windows. So, the all regressions (excepting bug 508747) are kept.

Comment 1

[Dão Gottwald [:dao]]

(In reply to comment #0)
> The bug wanted to make the consistency between platforms. So, the
> software acceleration must be implemented in XP level.

How does this affect native acceleration on OS X?

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #1)
> (In reply to comment #0)
> > The bug wanted to make the consistency between platforms. So, the
> > software acceleration must be implemented in XP level.
> 
> How does this affect native acceleration on OS X?

I don't know. But if so, the current implementation is wrong. Some mouse drivers on Windows also have the native acceleration.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

And also the patch implements the acceleration for the horizontal scrolling.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 396199 [details] [diff] [review]
Patch v1.0

oops, cannot build on Linux. Sorry for the spam.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.1

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 396218 [details] [diff] [review]
Patch v1.1

ok, passed all tests.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

test builds:
https://build.mozilla.org/tryserver-builds/[e-mail]-bug512235-3/

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah,

In my patch:

> +PRInt32
> +nsMouseWheelTransaction::ComputeWheelDelta(PRInt32 aDelta, PRInt32 aFactor)
> +{
> +  if (aDelta == 0)
> +    return 0;
> +
> +  return PRInt32(0.5 + (aDelta * sScrollSeriesCounter * (double)aFactor / 10));
> +}

But currently:

> -// If the scroll notification is part of a series of notifications we should
> -// increase scollEvent.delta to create an acceleration effect
> -int nsWindow::ComputeMouseWheelDelta(int currentVDelta,
> -                                     int iDeltaPerLine,
> -                                     ULONG ulScrollLines)
> -{
> -  // scrollAccelerationStart: click number at which acceleration starts
> -  int scrollAccelerationStart = gScrollPrefObserver->GetScrollAccelerationStart();
> -  // scrollNumlines: number of lines per scroll before acceleration
> -  int scrollNumLines = gScrollPrefObserver->GetScrollNumLines();
> -  // scrollAccelerationFactor: factor muliplied for constant acceleration
> -  int scrollAccelerationFactor = gScrollPrefObserver->GetScrollAccelerationFactor();
> -
> -  // compute delta that obeys numlines pref
> -  int ulScrollLinesInt = static_cast<int>(ulScrollLines);
> -  // currentVDelta is a multiple of (iDeltaPerLine * ulScrollLinesInt)
> -  int delta = scrollNumLines * currentVDelta / (iDeltaPerLine * ulScrollLinesInt);
> -
> -  // mScrollSeriesCounter: the index of the scroll notification in a series
> -  if (mScrollSeriesCounter < scrollAccelerationStart ||
> -      scrollAccelerationStart < 0 ||
> -      scrollAccelerationFactor < 0)
> -    return delta;
> -  else
> -    return int(0.5 + delta * mScrollSeriesCounter *
> -           (double) scrollAccelerationFactor / 10);
> -}

So, the current |delta| was additionally divided by |ulScrollLinesInt| which is a value of |SPI_GETWHEELSCROLLLINES|. So, the my patch is little faster than the current implementation if the value is larger than 1. (But I don't think this is not a new problem, because current implementation (or current pref value) is very high speed settings as most people cannot use it :-p

I think that we need additional tuning for this code by the workers of the app level acceleration. But it should be worked on bug 508785. It's known bug.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> But I don't think this is not a new problem

I meant "I *think* this is not a new problem". Sorry for the spam.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This repairs the DOM mouse wheel event behavior changes, we should fix this bug until 1.9.2b1 which is a first release after bug 462809.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Additional information for comment 8.

The |ulScrollLines| is a system setting value. And |iDeltaPerLine| is computed from the |ulScrollLines| and a const |WHEEL_DELTA| (the value is 120). So, they cannot be referred from XP level code. Therefore, my patch can *not* accelerate to same result under current settings (and without new APIs for getting them). Therefore, I'll not modify the patch before the reviews. But in fact, we need a tuning *after* this bug.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

vlad:

Would you review the patch?

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 396218 [details] [diff] [review]
Patch v1.1

Sorry; roc's review is good enough here, but this looks fine.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/7475d595c48a

Landed to trunk. The accelerated scrolling speed may be to fast. If so, we should change the default pref settings, Don't backout this patch by the reason.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.1.1

Actual checked-in patch.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> I don't know. But if so, the current implementation is wrong. Some mouse
> drivers on Windows also have the native acceleration.

Yes, the current implementation is wrong if the driver has native acceleration.  That's been a major cause of complaints recently.

It was apparently the plan to NOT enable acceleration on non-Windows by default.  what happened to that?

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #16)
> It was apparently the plan to NOT enable acceleration on non-Windows by
> default.  what happened to that?

I don't think the acceleration should be implemented only on Windows. Because the function was implemented for the constancy between platforms. And implementing in widget/src/* doesn't make sense for me, see comment 0.

And I posted a patch which disables the wheel acceleration in default settings, see bug 513817.

Comment 18

[Justin Dolske [:Dolske]]

Yes, it was surprising to suddenly find acceleration enabled for OS X (and presumably Linux, too).

Comment 19

[Boris Zbarsky [:bzbarsky]]

For what it's worth, the mac builds that have acceleration are completely unusable.  I've had to downgrade to a build from before this bug landed.  If nothing else, I couldn't use bugzilla (kept ending up at the end of the bug, not at the next comment).

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

If you cannot use the accelerated scrolling, set "mousewheel.acceleration.start" to -1 until bug 513817 will be fixed.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

beltzner:

If we will land  the patch of bug 513817, we need this patch before it.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Nominating b1 blocker.

-> P1
-> target to 3.6b1

See bug 513817 comment 63 for the reason. Note that if we land this patch without the patch of bug 518317, we need to change the default settings of the app level mouse acceleration, see comment 20.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.1.1 (for 1.9.2 branch)

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/57bef4de57dd