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

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

(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.

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)

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.

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

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.

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

(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!

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

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.

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.

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.

(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).

Stripped out ESM changes for other patch.

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.

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!

(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.

(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.

(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.

(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.

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.

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.

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."

(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.

(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...

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.

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

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?

Comment on attachment 8441809 [details] [diff] [review] Support for delta line accumulation and renames w/ review feedback sr+ for the interface change.

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!

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

Attached is the commit ready delta line scrolling patch.

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

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

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"

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

(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)

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

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).

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.

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.)

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...

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

Updated for the latest m-c.

Oops, updated patch is this one.

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.

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.

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.

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

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

Updated with newly generated interface UUID.

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

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

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 ?.

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

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

(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.

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 :)

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

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.

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

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).

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

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

Also it should depend on Bug 1207700

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...