haarp Reporter
	Description • 11 years ago

X has recently introduced smooth scrolling support, which allows scrolling with pixel-precision. It would provide a better alternative to the "Use smooth scrolling" option in Firefox or the big number of "smooth" scrolling extensions, on this platform.

Some drivers already support it (e. g. Synaptics) and others are currently being worked on (e.g. Evdev Wheel Emulation). Gnome 3 has already implemented support for it in its applications, and scrolling simply feels excellent.

More information: http://who-t.blogspot.de/2011/09/whats-new-in-xi-21-smooth-scrolling.html

	Paul Rouget [:paul]
	Comment 1 • 10 years ago

Could anyone give a quick overview of what would need to be done on gecko's side? Would it work with GTK2 too?

	Karl Tomlinson (:karlt)
	Comment 2 • 10 years ago

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #1)
> Could anyone give a quick overview of what would need to be done on gecko's
> side?

Probably something like attachment 8408918 [details] [diff] [review] for GTK3.

> Would it work with GTK2 too?

No.  GTK2 would require considerable work, which wouldn't be sensible when weighed against moving to GTK3.

	John Schoenick [:johns]
	Comment 3 • 10 years ago

I think this would also allow for async pan/zoom on linux, see e.g. bug 944938 (no idea how close the GFX stack is for that, though)

	Andrew Comminos [:acomminos] Assignee
	Comment 4 • 10 years ago

Hi, I've made a patch that implements the GDK_SCROLL_SMOOTH scroll direction when using the gtk3 backend on a gtk version >= 3.4. Synchronous scrolling is used on touchpad devices to improve responsiveness, much like the high resolution scrolling on the Windows and Cocoa widget backends. It emulates legacy behaviour on non-touch input devices.

	Cameron Kaiser [:spectre]
	Comment 5 • 10 years ago

Andrew, you might want to ask for review (set the review? flag to a peer for the Gtk widget module).

	Andrew Comminos [:acomminos] Assignee
	Comment 6 • 10 years ago

This fixes some issues with scrollable areas that require an integer value provided by the wheel event's lineOrPageDelta values. Hope you don't mind reviewing this karlt, I noticed your activity in this thread and in the commit history.

	Karl Tomlinson (:karlt)
	Comment 7 • 10 years ago

Thanks, Andrew.  I'm happy to review, but I might need your help explaining things.

Others have reported both smooth and up/down/... events (bug 996678) but you don't seem to be getting the up/down events.  Is that right?

(In reply to Andrew Comminos from comment #4)
> Synchronous scrolling
> is used on touchpad devices to improve responsiveness, much like the high
> resolution scrolling on the Windows and Cocoa widget backends.

Are you able to point me at the code or discussion of synchronous behavior for those backends, please?

(In reply to Andrew Comminos from comment #6)
> This fixes some issues with scrollable areas that require an integer value
> provided by the wheel event's lineOrPageDelta values. Hope you don't mind
> reviewing this karlt, I noticed your activity in this thread and in the
> commit history.

It seems odd that it is necessary to keep track of this here.
I wonder whether isPixelOnlyDevice is helpful, or perhaps the GDK UP/DOWN events are what we want.

http://dxr.mozilla.org/mozilla-central/search?q=isPixelOnlyDevice&case=true

	Andrew Comminos [:acomminos] Assignee
	Comment 8 • 10 years ago

(In reply to Karl Tomlinson (needinfo?:karlt) from comment #7)
> Others have reported both smooth and up/down/... events (bug 996678) but you
> don't seem to be getting the up/down events.  Is that right?
When the GTK version is above 3.4, my patch omits handling for non-smooth scroll events (they are still received however, likely for legacy support). From my testing these events provide no additional scroll data.

(In reply to Karl Tomlinson (needinfo?:karlt) from comment #7)
> Are you able to point me at the code or discussion of synchronous behavior
> for those backends, please?
I found the one for windows here: http://dxr.mozilla.org/mozilla-central/source/widget/windows/nsWinGesture.cpp#578.

The main reason I included SCROLL_SYNCHRONOUS was to override the smooth scrolling setting. You'll note that if we actually could use pixel deltas (instead of the line deltas that GTK reports), this would not be necessary- see http://dxr.mozilla.org/mozilla-central/source/dom/events/EventStateManager.cpp#2471. More on this later.

(In reply to Karl Tomlinson (needinfo?:karlt) from comment #7)
> It seems odd that it is necessary to keep track of this here.
> I wonder whether isPixelOnlyDevice is helpful, or perhaps the GDK UP/DOWN
> events are what we want.
> 
> http://dxr.mozilla.org/mozilla-central/search?q=isPixelOnlyDevice&case=true
You're right; upon further investigation, isPixelOnlyDevice is supposed to handle this.
http://dxr.mozilla.org/mozilla-central/source/widget/MouseEvents.h#461

However, this is *not* done if the delta mode is nsIDOMWheelEvent::DOM_DELTA_LINE, which is the way GTK reports delta information. This check is performed here: http://dxr.mozilla.org/mozilla-central/source/dom/events/EventStateManager.cpp#5073

I think we could do 2 things to address this issue:
1) Add support for partial line deltas being accumulated in EventStateManager.
2) Convert line deltas to pixels in the GTK event handler.

I believe the first solution is the most robust, as layout info is probably best kept out of the event handler. I'll see if I can hack together a patch.

Thanks for your response!

	Andrew Comminos [:acomminos] Assignee
	Comment 9 • 10 years ago

Use EventStateManager's delta accumulator, changing it to allow line deltas to be used. Mark GDK_SCROLL_SMOOTH events as isPixelOnlyDevice. Warning cleanup.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 10 • 10 years ago

Comment on attachment 8438916 [details] [diff] [review]
Added delta scroll accumulation to fix some scrollable windows

>@@ -3079,6 +3085,40 @@ nsWindow::OnScrollEvent(GdkEventScroll *aEvent)
>     WidgetWheelEvent wheelEvent(true, NS_WHEEL_WHEEL, this);
>     wheelEvent.deltaMode = nsIDOMWheelEvent::DOM_DELTA_LINE;
>     switch (aEvent->direction) {
>+#if GTK_CHECK_VERSION(3,4,0)
>+    case GDK_SCROLL_SMOOTH:
>+        // As of GTK 3.4, all directional scroll events are provided by
>+        // the GDK_SCROLL_SMOOTH direction, using line scrolling deltas.
>+        if(!gdk_event_get_scroll_deltas((GdkEvent*)aEvent, &wheelEvent.deltaX, &wheelEvent.deltaY) ||
>+            (wheelEvent.deltaX == 0 && wheelEvent.deltaY == 0)) {
>+            return;
>+        }
>+        // Accumulate scroll deltas, so when we reach a full line we can report it.
>+        mLastLineScrollY += wheelEvent.deltaY;
>+        mLastLineScrollX += wheelEvent.deltaX;
>+        if(abs(mLastLineScrollY) > 1) {
>+            wheelEvent.lineOrPageDeltaY = floor(mLastLineScrollY);

How about the native delta value case?

>+            mLastLineScrollY = 0;

I think that this should be: mLastLineScrollY -= wheelEvent.lineOrPageDeltaY;

>+        }
>+        if(abs(mLastLineScrollX) > 1) {
>+            wheelEvent.lineOrPageDeltaX = floor(mLastLineScrollX);
>+            mLastLineScrollX = 0;
>+        }

Same above.

>+        // Get slave device (if available) to determine source type.
>+        GdkDevice *device = gdk_event_get_source_device((GdkEvent*)aEvent);
>+        GdkInputSource source = device ? gdk_device_get_source(device) : GDK_SOURCE_MOUSE;
>+        switch(source) {
>+            case GDK_SOURCE_TOUCHPAD:
>+                // Use synchronous scrolling for direct touch events, improves responsiveness.
>+                wheelEvent.scrollType = WidgetWheelEvent::SCROLL_SYNCHRONOUSLY;
>+                break;
>+            default:
>+                wheelEvent.scrollType = WidgetWheelEvent::SCROLL_DEFAULT;
>+                wheelEvent.lineOrPageDeltaY *= 3;
>+                wheelEvent.deltaY *= 3;

Oh... they still need the magic number, 3...

Cannot we retrieve the system preferred value with something API?

And I feel odd. If they are multiplied by 3, mLastLineScrollY shouldn't be computed here?

And also, touch pad case's scroll amount is really indicates line amount? Isn't it scroll amount in pixels? I don't fine the document which defines the delta values explicitly, though.

(In reply to Andrew Comminos from comment #8)
> (In reply to Karl Tomlinson (needinfo?:karlt) from comment #7)
> > It seems odd that it is necessary to keep track of this here.
> > I wonder whether isPixelOnlyDevice is helpful, or perhaps the GDK UP/DOWN
> > events are what we want.
> > 
> > http://dxr.mozilla.org/mozilla-central/search?q=isPixelOnlyDevice&case=true
> You're right; upon further investigation, isPixelOnlyDevice is supposed to
> handle this.
> http://dxr.mozilla.org/mozilla-central/source/widget/MouseEvents.h#461
> 
> However, this is *not* done if the delta mode is
> nsIDOMWheelEvent::DOM_DELTA_LINE, which is the way GTK reports delta
> information. This check is performed here:
> http://dxr.mozilla.org/mozilla-central/source/dom/events/EventStateManager.
> cpp#5073
> 
> I think we could do 2 things to address this issue:
> 1) Add support for partial line deltas being accumulated in
> EventStateManager.
> 2) Convert line deltas to pixels in the GTK event handler.

I recomment #1. I implemented it. So, I can review around ESM.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 11 • 10 years ago

Comment on attachment 8439063 [details] [diff] [review]
gtk3-scroll-smooth+esm.patch

>@@ -5070,11 +5070,9 @@ EventStateManager::DeltaAccumulator::InitLineOrPageDelta(
>   mHandlingDeltaMode = aEvent->deltaMode;
>   mHandlingPixelOnlyDevice = aEvent->isPixelOnlyDevice;
> 
>-  // If it's handling neither pixel scroll mode for pixel only device nor
>-  // delta values multiplied by prefs, we must not modify lineOrPageDelta
>-  // values.
>-  if (!(mHandlingDeltaMode == nsIDOMWheelEvent::DOM_DELTA_PIXEL &&
>-        mHandlingPixelOnlyDevice) &&
>+  // If it's handling neither a pixel only device nor delta values multiplied by
>+  // prefs, we must not modify lineOrPageDelta values.
>+  if (!mHandlingPixelOnlyDevice &&
>       !EventStateManager::WheelPrefs::GetInstance()->
>         NeedToComputeLineOrPageDelta(aEvent)) {
>     // Set the delta values to mX and mY.  They would be used when above block

WidgetWheelEvent::isPixelOnlyDevice and DeltaAccumulator::mHandlingPixelOnlyDevice should be renamed to mIsNoLineOrPageDelta and mIsNoLineOrPageDeltaDevice.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 12 • 10 years ago

And I'd like to suggest that you should separate two patches. One is renaming the member names and changing ESM. The other is changing widget part.

	Andrew Comminos [:acomminos] Assignee
	Comment 13 • 10 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #10)
> How about the native delta value case?
Haven't tried it before, this implementation was based on the information here: https://developer.gnome.org/gdk3/stable/gdk3-Event-Structures.html#GdkEventScroll.

I checked out the gtk+ source and found that all that function does is access the native delta_x and delta_y properties with a safety check for GDK_SCROLL_SMOOTH. So that can be removed :)

> Oh... they still need the magic number, 3...
> 
> Cannot we retrieve the system preferred value with something API?
I don't think GNOME manages this. Scroll events are one-shot X11 button events. I kept the constant as-is for non-precision scrolling events so the UX wouldn't change for mouse wheel users.

> And also, touch pad case's scroll amount is really indicates line amount?
> Isn't it scroll amount in pixels? I don't fine the document which defines
> the delta values explicitly, though.
I got this information off of WebKit's implementation (I believe https://bug-88070-attachments.webkit.org/attachment.cgi?id=161165 got merged), which sets the wheel ticks on each axis to equal the delta values. The Gdk documentation for this API is notoriously poor.

> WidgetWheelEvent::isPixelOnlyDevice and DeltaAccumulator::mHandlingPixelOnlyDevice should be renamed to mIsNoLineOrPageDelta and mIsNoLineOrPageDeltaDevice.

Thanks, I'll update the patch with your suggestions (as well as split it into two).

	Andrew Comminos [:acomminos] Assignee
	Comment 14 • 10 years ago

Stripped out ESM changes for other patch.

	Makoto Kato [:m_kato]
	Comment 16 • 10 years ago

Andrew, your fix will break wheel scroll when mouse doesn't support high resolution wheel.

When using Ubuntu 14.04 on VMWare Workstation, mouse driver doesn't support high resolution unfortunately.  So GTK3 doesn't send GDK_SMOOTH_SCROLL even if set GDK_SMOOTH_SCROLL_MASK.

Even if both events (GDK_SCROLL_UP/DOWN and GDK_SMOOTH_SCROLL) are suppored, https://bugzilla.gnome.org/show_bug.cgi?id=726878 will occurs.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 17 • 10 years ago

Comment on attachment 8439130 [details] [diff] [review]
Allow accumulation of non-pixel delta amount in EventStateManager, rename WidgetWheelEvent::isPixelOnlyDevice and DeltaAccumulator::mHandlingPixelOnlyDevice appropriately

>diff --git a/dom/base/nsDOMWindowUtils.cpp b/dom/base/nsDOMWindowUtils.cpp
>index 20a8672..400c496 100644
>--- a/dom/base/nsDOMWindowUtils.cpp
>+++ b/dom/base/nsDOMWindowUtils.cpp
>@@ -879,10 +879,10 @@ nsDOMWindowUtils::SendWheelEvent(float aX,
>   wheelEvent.deltaMode = aDeltaMode;
>   wheelEvent.isMomentum =
>     (aOptions & WHEEL_EVENT_CAUSED_BY_MOMENTUM) != 0;
>-  wheelEvent.isPixelOnlyDevice =
>+  wheelEvent.mIsNoLineOrPageDelta =
>     (aOptions & WHEEL_EVENT_CAUSED_BY_PIXEL_ONLY_DEVICE) != 0;
>   NS_ENSURE_TRUE(
>-    !wheelEvent.isPixelOnlyDevice ||
>+    !wheelEvent.mIsNoLineOrPageDelta ||
>       aDeltaMode == nsIDOMWheelEvent::DOM_DELTA_PIXEL,
>     NS_ERROR_INVALID_ARG);
>   wheelEvent.customizedByUserPrefs =

Could you attach a patch which is generated with |-U 8 -p| next time? That's the basic rule for requesting reviews. But it's ok for this.

>diff --git a/dom/events/test/test_continuous_wheel_events.html b/dom/events/test/test_continuous_wheel_events.html
>index 58ddd0d..dfad6b8 100644
>--- a/dom/events/test/test_continuous_wheel_events.html
>+++ b/dom/events/test/test_continuous_wheel_events.html
>@@ -108,7 +108,7 @@ function testContinuousTrustedEvents()
>     { description: "Simple horizontal wheel event by pixels (16.0 - 1) #1",
>       event: { deltaMode: WheelEvent.DOM_DELTA_PIXEL,
>                deltaX: 16.0, deltaY: 0.0, deltaZ: 0.0, isMomentum: false,
>-               lineOrPageDeltaX: 1, lineOrPageDeltaY: 0, isPixelOnlyDevice: false,
>+               lineOrPageDeltaX: 1, lineOrPageDeltaY: 0, mIsNoLineOrPageDelta: false,

The changes in test_continuous_wheel_events.html.

Please replace mIsNoLineOrPageDelta in this file with isNoLineOrPageDelta.

>diff --git a/services/sync/tps/extensions/mozmill/resource/stdlib/EventUtils.js b/services/sync/tps/extensions/mozmill/resource/stdlib/EventUtils.js
>index dcc5e72..4297750 100644
>--- a/services/sync/tps/extensions/mozmill/resource/stdlib/EventUtils.js
>+++ b/services/sync/tps/extensions/mozmill/resource/stdlib/EventUtils.js
>@@ -317,7 +317,7 @@ function synthesizeTouchAtCenter(aTarget, aEvent, aWindow)
>  *
>  * aEvent is an object which may contain the properties:
>  *   shiftKey, ctrlKey, altKey, metaKey, accessKey, deltaX, deltaY, deltaZ,
>- *   deltaMode, lineOrPageDeltaX, lineOrPageDeltaY, isMomentum, isPixelOnlyDevice,
>+ *   deltaMode, lineOrPageDeltaX, lineOrPageDeltaY, isMomentum, mIsNoLineOrPageDelta,
>  *   isCustomizedByPrefs, expectedOverflowDeltaX, expectedOverflowDeltaY
>  *
>  * deltaMode must be defined, others are ok even if undefined.

I think that you don't need to update under mozmill. I've never modified this.

>diff --git a/testing/mochitest/tests/SimpleTest/EventUtils.js b/testing/mochitest/tests/SimpleTest/EventUtils.js
>index 5809142..13b62cb 100644
>--- a/testing/mochitest/tests/SimpleTest/EventUtils.js
>+++ b/testing/mochitest/tests/SimpleTest/EventUtils.js
>@@ -342,7 +342,7 @@ function synthesizeTouchAtCenter(aTarget, aEvent, aWindow)
>  *
>  * aEvent is an object which may contain the properties:
>  *   shiftKey, ctrlKey, altKey, metaKey, accessKey, deltaX, deltaY, deltaZ,
>- *   deltaMode, lineOrPageDeltaX, lineOrPageDeltaY, isMomentum, isPixelOnlyDevice,
>+ *   deltaMode, lineOrPageDeltaX, lineOrPageDeltaY, isMomentum, mIsNoLineOrPageDelta,
>  *   isCustomizedByPrefs, expectedOverflowDeltaX, expectedOverflowDeltaY
>  *
>  * deltaMode must be defined, others are ok even if undefined.

Replace mIsNoLineOrPageDelta with isNoLineOrPageDelta in this file too.

Members of JS object shouldn't be names with m prefix.

>@@ -361,7 +361,7 @@ function synthesizeWheel(aTarget, aOffsetX, aOffsetY, aEvent, aWindow)
> 
>   var modifiers = _parseModifiers(aEvent);
>   var options = 0;
>-  if (aEvent.isPixelOnlyDevice &&
>+  if (aEvent.mIsNoLineOrPageDelta &&
>       (aEvent.deltaMode == WheelEvent.DOM_DELTA_PIXEL)) {
>     options |= utils.WHEEL_EVENT_CAUSED_BY_PIXEL_ONLY_DEVICE;
>   }

So, you need to remove the deltaMode check here. And nsIDOMWindowUtils::WHEEL_EVENT_CAUSED_BY_PIXEL_ONLY_DEVICE should be renamed with nsIDOMWindowUtils::WHEEL_EVENT_CAUSED_BY_NO_LINE_OR_PAGE_DELTA_DEVICE.

However, it could cause breaking existing addons even though there is no addons using this flag in addons.mozilla.org. So, you just add this flag with same value as WHEEL_EVENT_CAUSED_BY_PIXEL_ONLY_DEVICE. And please document it is deprecated.
http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindowUtils.idl#471

>@@ -389,8 +389,8 @@ function synthesizeWheel(aTarget, aOffsetX, aOffsetY, aEvent, aWindow)
>       options |= utils.WHEEL_EVENT_EXPECTED_OVERFLOW_DELTA_Y_NEGATIVE;
>     }
>   }
>-  var isPixelOnlyDevice =
>-    aEvent.isPixelOnlyDevice && aEvent.deltaMode == WheelEvent.DOM_DELTA_PIXEL;
>+  var mIsNoLineOrPageDelta =
>+    aEvent.mIsNoLineOrPageDelta && aEvent.deltaMode == WheelEvent.DOM_DELTA_PIXEL;

So, delta mode check should be removed here too.

Otherwise, looks ok to me. Could you recreate a patch with above changes? Thanks in advance!

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 18 • 10 years ago

(In reply to Makoto Kato (:m_kato) from comment #16)
> Andrew, your fix will break wheel scroll when mouse doesn't support high
> resolution wheel.
> 
> When using Ubuntu 14.04 on VMWare Workstation, mouse driver doesn't support
> high resolution unfortunately.  So GTK3 doesn't send GDK_SMOOTH_SCROLL even
> if set GDK_SMOOTH_SCROLL_MASK.
> 
> Even if both events (GDK_SCROLL_UP/DOWN and GDK_SMOOTH_SCROLL) are suppored,
> https://bugzilla.gnome.org/show_bug.cgi?id=726878 will occurs.

I think that even if there is no solution for now, we should land the patch with a pref which can switch the smooth scroll support and disable the feature in the default settings.

	Karl Tomlinson (:karlt)
	Comment 19 • 10 years ago

(In reply to Makoto Kato (:m_kato) from comment #16)
> When using Ubuntu 14.04 on VMWare Workstation, mouse driver doesn't support
> high resolution unfortunately.  So GTK3 doesn't send GDK_SMOOTH_SCROLL even
> if set GDK_SMOOTH_SCROLL_MASK.

Thank you.  That explains why native GTK widgets that listen for smooth scroll events also handle (the old) discrete events.  The situation will be the same will older X servers.

	Karl Tomlinson (:karlt)
	Comment 20 • 10 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #10)
> And also, touch pad case's scroll amount is really indicates line amount?
> Isn't it scroll amount in pixels? I don't fine the document which defines
> the delta values explicitly, though.

Most GTK widgets treat the scroll deltas as having units corresponding to the number of discrete events.  This includes resize_by_scroll_cb in gtkfontchooserwidget.c, gtk_scrolled_window_scroll_event, _gtk_range_get_wheel_delta.

gtk_menu_scroll treats delta_y as pixel values, differing from the treatment
of discrete events by a ratio of 1:15, but I suspect that is a bug.

http://lists.x.org/archives/xorg-devel/2011-June/023499.html
says "One smooth-scrolling event will generate floor(abs(value)) * 4
emulated button events" about the native X events, and I doubt GDK changes those values.

Perhaps lines was always an odd scroll unit, and the viewport size might be more relevant, but let's not change that here.

	Karl Tomlinson (:karlt)
	Comment 21 • 10 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #18)
> I think that even if there is no solution for now, we should land the patch
> with a pref which can switch the smooth scroll support and disable the
> feature in the default settings.

https://bugzilla.gnome.org/show_bug.cgi?id=726878 should be simple to fix I think.

But IIUC emulated discrete events always arrive after the smooth events, so they can be detected and filtered, similar to attachment 8408918 [details] [diff] [review], but I expect we don't need to filter out any smooth scroll events.

	Andrew Comminos [:acomminos] Assignee
	Comment 22 • 10 years ago

I've discovered some additional information from the scroll event handler of gtkscrolledwindow.c- the 'scroll unit' is defined as below. This value appears to be a multiplier that GTK uses to find pixel deltas from a smooth scroll event. Some initial testing by myself has shown scrolling with these deltas with DOM_DELTA_PIXEL to be similar to other GNOME applications (tested with gedit, evince, and documents). In which case, it may be best not to touch ESM at all, and use the existing pixel scrolling code.

(From gtkscrolledwindow.c's scroll event, handling delta_x)
> page_size = gtk_adjustment_get_page_size (adj);
> scroll_unit = pow (page_size, 2.0 / 3.0);
> new_value = CLAMP (gtk_adjustment_get_value (adj) + delta_x * scroll_unit,
>                    gtk_adjustment_get_lower (adj),
>                    gtk_adjustment_get_upper (adj) -
>                    gtk_adjustment_get_page_size (adj));
> 
> gtk_adjustment_set_value (adj, new_value);

Thank you very much for the detailed review anyway masayuki! I am new to the Mozilla codebase, so any tips are appreciated.

m_kato: Your issue appears to be an caused by drivers that do not support xinput2. I tested the patch on a mouse wheel myself, the driver I used (evdev) supports it. I suppose that means we simply cannot omit legacy events under GTK3.4.

(In reply to Karl Tomlinson (needinfo?:karlt) from comment #21)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #18)
> > I think that even if there is no solution for now, we should land the patch
> > with a pref which can switch the smooth scroll support and disable the
> > feature in the default settings.
> 
> https://bugzilla.gnome.org/show_bug.cgi?id=726878 should be simple to fix I
> think.
> 
> But IIUC emulated discrete events always arrive after the smooth events, so
> they can be detected and filtered, similar to attachment 8408918 [details] [diff] [review]
> [diff] [review], but I expect we don't need to filter out any smooth scroll
> events.

I think the best bet at the moment would be to perform the timing check you mentioned. Thank you for referring me to that attachment, I wasn't sure if any prior work had been done on implementing this feature.

I'll work on another patch including the timing check as well as using the scroll multiplier mentioned above.

	Andrew Comminos [:acomminos] Assignee
	Comment 23 • 10 years ago

Here's the patch using the pixel scrolling multiplication technique that GTK uses, with a workaround for mouse wheel emulation and smooth scrolling as well as input devices that do not support XInput2.

	Karl Tomlinson (:karlt)
	Comment 24 • 10 years ago

Comment on attachment 8439769 [details] [diff] [review]
GTK3 smooth scroll with duplicate scroll event omission, mouse wheel emulation with smooth scrolling

I agree that lines is not an appropriate unit for scrolling but this code
doesn't know the size of the viewport and so is not the right place to be
determining the number of pixels to scroll.

Gecko uses a single window, and so viewports in textareas, etc. may occupy only
a small region on the window.  (The notable exception is windowed plugins,
which get their own child windows.)  The rate of scrolling on a textarea
should not depend on the height or width of the browser window. 

The best way to make progress is to break things into smaller tasks.  In this
bug, support for smooth scroll events can be added, and they should have the
same units as discrete events [1].  If you like, another bug can track picking
better scroll units on a viewport.  That will require viewport code changes, I
assume, and perhaps a new units on the WheelEvent, but both discrete and
smooth events can have similar fixes there.

mLastScrollEventTime should be initialized somewhere if it is a non-static variable.  GDK_CURRENT_TIME is a good value because there should be no events with this timestamp.

Note also that Gecko coding style wraps at 80 (or 79) columns and has a space
between control structure keywords and the opening parenthesis. e.g. "if ("

[1] http://cgit.freedesktop.org/xorg/proto/inputproto/tree/specs/XI2proto.txt?id=3c1ebd1cfe71029ebc6a732a6b55308861e549e0#n164

"One unit of scrolling in either direction is considered to be equivalent to
one button event, e.g. for a unit size of 1.0, -2.0 on an valuator type
Vertical sends two button press/release events for button 4. Likewise, a
button press event for button 7 generates an event on the Horizontal valuator
with a value of +1.0."

	Karl Tomlinson (:karlt)
	Comment 25 • 10 years ago

(In reply to Andrew Comminos from comment #8)
> (In reply to Karl Tomlinson (needinfo?:karlt) from comment #7)
> I found the one for windows here:
> http://dxr.mozilla.org/mozilla-central/source/widget/windows/nsWinGesture.cpp#578.

WINNT often uses a more synchronous event model than X11.
I suspect it may coalesce mouse events that arrive while processing an event.

> The main reason I included SCROLL_SYNCHRONOUS was to override the smooth
> scrolling setting. You'll note that if we actually could use pixel deltas
> (instead of the line deltas that GTK reports), this would not be necessary-
> see
> http://dxr.mozilla.org/mozilla-central/source/dom/events/EventStateManager.cpp#2471.

The behavior with SCROLL_DEFAULT and DOM_DELTA_PIXEL is slightly different in
that it corresponds to SCROLL_ASYNCHRONOUSELY, which I think would be suitable
here.

The problem with scrolling synchronously is that, if the application cannot
keep up with drawing in response to a stream of scroll events, then the events will build up in a queue and scrolling will continue after the user has stopped using the touch pad.

With asynchronous scrolling, if several events build up in the queue, then the
application with process them all to determine the new position, before
drawing.  The idea is that accumulating info from the events is fast, and so
the queue will empty and there will be a chance to draw.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 26 • 10 years ago

(In reply to Andrew Comminos from comment #22)
> (From gtkscrolledwindow.c's scroll event, handling delta_x)
> > page_size = gtk_adjustment_get_page_size (adj);
> > scroll_unit = pow (page_size, 2.0 / 3.0);
> > new_value = CLAMP (gtk_adjustment_get_value (adj) + delta_x * scroll_unit,
> >                    gtk_adjustment_get_lower (adj),
> >                    gtk_adjustment_get_upper (adj) -
> >                    gtk_adjustment_get_page_size (adj));
> > 
> > gtk_adjustment_set_value (adj, new_value);

It's similar to page scroll, but it's not exactly same...

(In reply to Karl Tomlinson (needinfo?:karlt) from comment #24)
> Gecko uses a single window, and so viewports in textareas, etc. may occupy
> only
> a small region on the window.  (The notable exception is windowed plugins,
> which get their own child windows.)  The rate of scrolling on a textarea
> should not depend on the height or width of the browser window. 

Exactly.

> The best way to make progress is to break things into smaller tasks.  In this
> bug, support for smooth scroll events can be added, and they should have the
> same units as discrete events [1].  If you like, another bug can track
> picking
> better scroll units on a viewport.  That will require viewport code changes,
> I
> assume, and perhaps a new units on the WheelEvent, but both discrete and
> smooth events can have similar fixes there.

Adding new units to WheelEvent should be careful. The deltaMode is exposed as WheelEvent.deltaMode defined on D3E (DOM Level 3 Events). Although, we could add it and hide the deltaMode and convert the delta values in EventStateManager::PreHandleEvent().

If we would do it, we need to compute temporary scroll target from mouse cursor position and mouse wheel transaction (see https://wiki.mozilla.org/Gecko:Mouse_Wheel_Scrolling#Mouse_wheel_transaction). Next, we should convert the deltaMode and delta values as pixel. Finally, we should recompute the scroll target and scroll amount in ESM::PostHandleEvent with the latest DOM tree and layout information since some of them have been changed by wheel event handlers or something.

However, this is too complicated. If we want to emulate the other GTK applications' scroll amount, I guess that it's enough to use page scroll mode with preprocessed delta values. Although, pow(page_size, 2.0/3.0) makes this difficult...

	Andrew Comminos [:acomminos] Assignee
	Comment 27 • 10 years ago

Here's an updated patch with review suggestions, again back to using lines. In order to support legacy NS_WHEEL_SCROLL events, the other patch attached to this bug must be applied.

	Andrew Comminos [:acomminos] Assignee
	Comment 28 • 10 years ago

Renames and line delta accumulation support with suggestions from https://bugzilla.mozilla.org/show_bug.cgi?id=958868#c17.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 29 • 10 years ago

Comment on attachment 8441809 [details] [diff] [review]
Support for delta line accumulation and renames w/ review feedback

LGTM, thanks.

Smaug, could you do sr for nsIDOMWindowUtils.idl change?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 30 • 10 years ago

Comment on attachment 8441809 [details] [diff] [review]
Support for delta line accumulation and renames w/ review feedback

sr+ for the interface change.

	Karl Tomlinson (:karlt)
	Comment 31 • 10 years ago

Comment on attachment 8441807 [details] [diff] [review]
GTK3 smooth scrolling using lines. Requires ESM isNoLineOrPageDelta patch.

>+#if GTK_CHECK_VERSION(3,4,0)
>+                     GDK_SMOOTH_SCROLL_MASK |
>+#else
>                      GDK_SCROLL_MASK |
>+#endif

Please keep GDK_SCROLL_MASK, even when GDK_SMOOTH_SCROLL_MASK is present, for
non-inputproto-2.1 devices.  This may happen to work now, but won't when
https://bugzilla.gnome.org/show_bug.cgi?id=726878 is fixed.

>+    if (mLastScrollEventTime == aEvent->time) return; 

Gecko style usually has statements for single-statement conditional blocks on
a new line.  This makes it easier to place breakpoints in a debugger, and
makes jump statements, such as early returns more obvious.

>+        }
>+
>+    }
>+        break;

The indentation here would look more sane with the "break" inside the block.

Looks good, thank you!

	Andrew Comminos [:acomminos] Assignee
	Comment 32 • 10 years ago

Here's the updated patch with the review comments. Thanks.

	Andrew Comminos [:acomminos] Assignee
	Comment 33 • 10 years ago

Attached is the commit ready delta line scrolling patch.

	Andrew Comminos [:acomminos] Assignee
	Comment 35 • 10 years ago

The patches have been updated to be commit-ready with mq and flagged as checkin. Hope I did this right.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 36 • 10 years ago

I pushed them to tryserver for pre-checkin tests:

For all tests on Linux:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=ebef4e54b5d4

For test_continuous_wheel_events.html
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=e921bb51ddff

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 37 • 10 years ago

Comment on attachment 8444958 [details] [diff] [review]
Add support for delta line and page accumulation. Commit-ready.

Although, I don't know the formal rules about checkin? per patch, I landed them to m-i.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8499eefa342e
https://hg.mozilla.org/integration/mozilla-inbound/rev/f809a67a796e

# I changed "r=smaug" to "sr=smaug"

	Carsten Book [:Tomcat]
	Comment 38 • 10 years ago

sorry had to backout this changes for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=42344847&tree=Mozilla-Inbound

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 39 • 10 years ago

(In reply to Carsten Book [:Tomcat] from comment #38)
> sorry had to backout this changes for test failures like
> https://tbpl.mozilla.org/php/getParsedLog.php?id=42344847&tree=Mozilla-
> Inbound

What?? It was green on tryserver... (https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=e921bb51ddff)

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 40 • 10 years ago

It's odd, the failure occurs randomly...

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=61400645a576
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=dd603ecc46fd

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 41 • 10 years ago

Ugh... I don't reproduce the random orange on tryserver with m-c.

I'll retry it with tomorrow's m-c (i.e., after next merge with m-i).

	Andrew Comminos [:acomminos] Assignee
	Comment 42 • 10 years ago

Strange; using today's m-c (changeset 190502:1e7a765ac181) with both patches applied and

> mach mochitest dom/events/test/test_continuous_wheel_events.html --run-until-failure --repeat 100

yields no failures after 100 loops on x86_64 GNU/Linux. I can't find any issues with the patch either that might cause platform-specific regressions such as on OS X.

	Karl Tomlinson (:karlt)
	Comment 43 • 10 years ago

It might be worth trying valgrind with the --debugger and --debugger-args to see whether there's an uninitialized variable usage.
(The problem may not even be in this patch but the effects may be triggered by changes in this patch.)

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 44 • 10 years ago

Still not reproduce the random orange on tryserver with tomorrow's m-c.
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=f125574431a8

The test is testing EventStateManager behavior. So, I think that if there is an uninitialized variable in it, it should cause compile error on Mac or Linux...

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 45 • 10 years ago

Hmm, with yesterday's m-c, I don't see the random orange...
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=86a98d99de6c

If the result is same on today's m-c too, should we try to land them again?
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=9c766a2fd86e

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 46 • 10 years ago

Updated for the latest m-c.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 47 • 10 years ago

Oops, updated patch is this one.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 48 • 10 years ago

Comment on attachment 8446911 [details] [diff] [review]
Part 1 - Add support for GDK_SCROLL_SMOOTH

Ugh, I'm confused by the patch summary. They should have part XX. Sorry for the spam.

	Andrew Comminos [:acomminos] Assignee
	Comment 49 • 10 years ago

Thanks for updating the patch for the latest m-c. Looks like the tryserver's reporting success:

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=9c766a2fd86e

Is there anything else that can be done to verify the patch's integrity before pushing to m-i? I've tried several times on different platforms with clean trees, haven't encountered any failures.

	Makoto Kato [:m_kato]
	Comment 50 • 10 years ago

try server always uses CLOBBER build, but simple push isn't dependency build.

I think Part 2 changes IDL (but method are property aren't modified) without UUID change, so XPT file may not be updated, then mochi test may be failure.

	Makoto Kato [:m_kato]
	Comment 51 • 10 years ago

So, I think we must modify UUID of nsIDOMWindowUtils.idl or land with updating CLOBBER file...

	Andrew Comminos [:acomminos] Assignee
	Comment 52 • 10 years ago

Thanks for letting me know- I'll update the patch with a new UUID.

	Andrew Comminos [:acomminos] Assignee
	Comment 53 • 10 years ago

Updated with newly generated interface UUID.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 54 • 10 years ago

Hmm, in my understanding, we don't need to change UUID when we add new constant. But I'm not sure, let's try with it.

The first patch is rejected. I'll land them after I confirm that they are green on tryserver.
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=83ec347ca965

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 55 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/eebc0e885bb3
https://hg.mozilla.org/integration/mozilla-inbound/rev/1113df431eab

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 56 • 10 years ago

Comment on attachment 8451458 [details] [diff] [review]
Part 2 - Add support for delta line and page accumulation

You don't need to make checkin flag ?.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 57 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/eebc0e885bb3
https://hg.mozilla.org/mozilla-central/rev/1113df431eab

	vtrakiyski
	Comment 59 • 9 years ago

How can one actually enable and use this functionality, assuming that it's currently shipping in Firefox? For me scrolling is still jumpy in Firefox while a lot of gnome apps have pixel-perfect scrolling.

Thanks

	Jan Niklas Hasse
	Comment 60 • 9 years ago

(In reply to vtrakiyski from comment #59)
> How can one actually enable and use this functionality, assuming that it's
> currently shipping in Firefox? For me scrolling is still jumpy in Firefox
> while a lot of gnome apps have pixel-perfect scrolling.
> 
> Thanks

AFAIK this is only fixed in Gtk3 builds of Firefox. Most Linux distros are shipping the Gtk2 version, so unfortunately you'll have to wait a little bit. Fedora 22 seems to be one of the first distros to ship the Gtk3 version by default. In my testings you don't need to enable anything when running the Gtk3 version, it should work out of the box.

	vtrakiyski
	Comment 61 • 9 years ago

Wow, thanks a lot, I wasn't aware of that - I just downloaded the gtk3 version and I'm so glad to actually have a good browser with smooth scrolling! Props to everyone who worked on this :)

	Ernst Sjostrand
	Comment 63 • 9 years ago

Did this break mousewheel.acceleration.* and all forms of changing scroll speed like mousewheel.withnokey.numlines ? I really think it's slow without those...

	Karl Tomlinson (:karlt)
	Comment 64 • 9 years ago

I doubt it.  Bug 1143856 seems more likely.
If toggling layers.async-pan-zoom.enabled resolves the issue, then file a new bug blocking Bug 1143856 rather than commenting in that bug.

	Anders Kaseorg
	Comment 65 • 9 years ago

This seems to have been disabled a few weeks ago (bug 1207700).

	Me
	Comment 66 • 9 years ago

Hi, has this  "real smooth scrolling" been removed ? I don't have it in Aurora since a few weeks.

Is there a way to get it back ? Browsing was really better with it.

If it is related with Anders Kaseorg's link, the tweak given in 1207700 didn't work for me (setting MOZ_USE_XINPUT2=1).

	Me
	Comment 67 • 9 years ago

OK, sorry, I didn't see it was only available in Nightly.

	Me
	Comment 68 • 9 years ago

Shouldn't this bug loose its "RESOLVED" status since it has been disabled ?

	Cedric Wehrum
	Comment 69 • 9 years ago

Also it should depend on Bug 1207700

	Ernst Sjostrand
	Comment 70 • 9 years ago

After whining about mousewheel acceleration support for APZ in bug 1214170 I realize that acceleration should probably _NOT_ be applied to "true smooth scrolling", it's two very different inputs...