Issue metadata
Sign in to add a comment
|
Fling scrolling doesn't work on Facebook on ChromeOS |
||||||||||||||||||||||
Issue descriptionGoogle Chrome 57.0.2987.146 (Official Build) (64-bit) Revision 0 Platform 9202.64.0 (Official Build) stable-channel samus What steps will reproduce the problem? (1) Go to facebook.com (2) Login (3) Fling on the trackpad. What is the expected result? Should fling scroll. What happens instead? Stops scrolling immediately when you lift your fingers from the track pad. In general scrolling feels very abrupt with no acceleration.
,
May 1 2017
esprehn@ Can you remove the mousewheel event listener in devtools for facebook? I've done this locally and I see that it behaves nicely. I believe they are likely adjusting the scroll position themselves canceling any scroll. This might be fixed via CSS Smooth Scroll.
,
May 1 2017
I've noticed this on lots of websites on my Chromebook recently. Did something change in Chrome?
,
May 2 2017
Adding this instrumentation on the console I can see that, although removing the mousewheel listener does fix the problem, Facebook isn't actually calling preventDefault on the wheel event:
document.addEventListener('wheel', function(e) { console.log(e, e.defaultPrevented); });
Is this perhaps something to do with touchpad latching and DOM node recycling done in infinite list implementations like Facebook?
,
May 2 2017
Seems to work fine on Mac, so perhaps this is triggered somehow by the different touchpad fling implementation in ChromeOS? Using:
document.addEventListener('mousewheel', e => {console.log(e);}, {capture:true});
I don't see wheel events get delivered at all during the fling, so it definitely seems like a chromium bug somewhere causing the fling to be canceled.
Oh, I can reproduce this on any site by adding a passive wheel listener:
document.documentElement.addEventListener('mousewheel', e=>{}, {passive:true});
Perhaps our lack of wheel ACK in the passive wheel listener case is triggering the ChromeOS fling-cancelling logic or something?
Reproduced on 58.0.3029.78 beta
,
May 2 2017
,
May 2 2017
/cc abodenha since this seems like a pretty serious ChromeOS-specific regression to me. I don't think we have any easy way to do ChromeOS-specific per-commit build bisects.
,
May 2 2017
+adlr and bokan for their thoughts on touch and scroll.
,
May 3 2017
sahel@ do you think you could have a look at this?
,
May 3 2017
I could reproduce the issue on 60.0.3078.0 dev as well, but when threaded_scrolling is disabled fling happens. It looks like a compositor side problem. Still working on in to find the cause.
,
May 3 2017
Reproduced on 54.0.2840.53 official build. I couldn't go back any further since this was the oldest version that I could get from here: https://pantheon.corp.google.com/storage/browser/chromeos-image-archive/samus-release rbyers@ could you please give an example website on which you could reproduce the bug by adding a passive mousewheel listener?
,
May 4 2017
I checked the old devices that we have in the office, could reproduce it on M52.0.2743.116 (official build) too. However, on 60.0.3078.0 (developer build) I could reproduce it non-deterministically. Maybe we could reduce the priority since it has been there at least since M52.
,
May 4 2017
The issue happens when both passive and blocking wheel event listeners exist on a webpage. This link is a simplified case that reproduces the problem: http://output.jsbin.com/bomiwoy/quiet Deleting either of the listeners avoids the problem. I couldn't reproduce the issue on 60.0.3078.0 (Developer Build). Occasional abrupt scroll terminations that I see on facebook.com is due to processing a fling cancel event. esprehn@ could you please confirm if you can still see the issue on M60?
,
May 4 2017
sahel@ I wonder if this is fixed by change https://chromium.googlesource.com/chromium/src/+/742bb98afe29eff3d693328bc39a62f680f83a1d%5E%21/#F0
,
May 4 2017
Yes, that is the fix. I commented that line and could reproduce the issue.
,
May 4 2017
I've confirmed that issue is in M59. So I think we are good here that passive event listeners don't cause the fling to stop. Moving to a P2, I think the only thing we need to figure out is why we are generating a Fling Cancel on facebook but I imagine this is due to their infinite scroller.
,
May 4 2017
,
May 4 2017
Ok so the issue I reproduced is really issue 708236 and not connected to the facebook behavior at all?
,
May 4 2017
Yes Rick.. It is fixed in M59 and sahel@ is still investigating facebook.
,
May 9 2017
Abrupt scrollEnds occasionally happen because of a race condition. On ChromeOS wheel events are generated from ui:ET_SCROLL events, and GSU events are generated from wheel events if their ack is not consumed; But, GFS events are generated from ui::ET_SCROLL_FLING_START events directly. Different paths and delays for generating GSU and GFS events sometimes causes a GSU from a older ET_SCROLL event to reach the GestureEventQueue after a GFS from a more recent ET_SCROLL_FLING_START. This reordering causes dropping GFS from the debouncing deferral queue before adding it to GestureEventQueue. Since the race condition is not common (and it will be even less common when async wheel events with immediate acks are implemented) I leave it as it is for now. After discussing with tdresser@ and dtapuska@, one potential solution is to move the fling curve generation higher and generate wheel events with phase info similar to fling on Mac. This will also help with cleaning up parts of the fling handling that is different between trackpad and Touchscreen.
,
May 9 2017
To confirm you're saying that M60 should have this fixed? The current behavior on Facebook is 100% of the time, so it doesn't seem to be an "uncommon race".
,
May 9 2017
The change in #15 reduces this to a 100% occurrence to a not frequent occurrence.
,
Jun 28 2017
There is a bug in the browser where using the touchpad scroll gesture to navigate back/forward will get "stuck" between two pages. I found this bug has the same root cause as explained in #21. It is not that common, but it is obvious and disruptive when it happens. I plan on adding a special case to the GestureEventQueue bounce filter for GFS, since they shouldn't be ignored.
,
Jul 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/29786c71a84fc576bc6a861a1b91d830e2f686c9 commit 29786c71a84fc576bc6a861a1b91d830e2f686c9 Author: Sean O'Brien <seobrien@google.com> Date: Thu Jul 20 19:35:14 2017 Don't filter GestureFlingStart in |GestureEventQueue| Scroll events from the touchpad may arrive in the |GestureEventQueue| out of order, such that a GestureFlingStart arrives before a GestureScrollUpdate. Currently, the debounce filter removes the GestureFlingStart. This CL will forward the GestureFlingStart after emptying the debounce queue. BUG= 717205 Change-Id: I42bd147195b9a2be8e34391c534c625cc785e2e0 Reviewed-on: https://chromium-review.googlesource.com/553424 Commit-Queue: Sean O'Brien <seobrien@chromium.org> Reviewed-by: Dave Tapuska <dtapuska@chromium.org> Cr-Commit-Position: refs/heads/master@{#488353} [modify] https://crrev.com/29786c71a84fc576bc6a861a1b91d830e2f686c9/content/browser/renderer_host/input/gesture_event_queue.cc [modify] https://crrev.com/29786c71a84fc576bc6a861a1b91d830e2f686c9/content/browser/renderer_host/input/gesture_event_queue_unittest.cc
,
Jul 28 2017
,
Oct 3 2017
,
Nov 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2dc65f0d49b20c16af3d00eb1585d0ebf8b14d96 commit 2dc65f0d49b20c16af3d00eb1585d0ebf8b14d96 Author: Sean O'Brien <seobrien@google.com> Date: Thu Nov 30 15:56:47 2017 Filter GestureScrollUpdate during scroll fling In |GestureEventQueue|, filter GestureScrollUpdate events if a scroll fling is ongoing. Scroll events from the touchpad sometimes arrive in the |GestureEventQueue| out of order, and a GestureScrollUpdate may arrive after a GestureFlingStart. These should be filtered so we don't have a GestureScrollUpdate without a subsequent GestureFlingStart. BUG= 717205 Change-Id: I14bf134736269b323d441c5f670601f0d50ed03f Reviewed-on: https://chromium-review.googlesource.com/727464 Reviewed-by: Dave Tapuska <dtapuska@chromium.org> Commit-Queue: Sean O'Brien <seobrien@chromium.org> Cr-Commit-Position: refs/heads/master@{#520556} [modify] https://crrev.com/2dc65f0d49b20c16af3d00eb1585d0ebf8b14d96/content/browser/renderer_host/input/gesture_event_queue.cc [modify] https://crrev.com/2dc65f0d49b20c16af3d00eb1585d0ebf8b14d96/content/browser/renderer_host/input/gesture_event_queue_unittest.cc
,
Jan 22 2018
,
Jan 23 2018
,
Apr 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fe4b9ea5050c23b5eb03258412f02ef42798084e commit fe4b9ea5050c23b5eb03258412f02ef42798084e Author: Sahel Sharify <sahel@chromium.org> Date: Tue Apr 17 14:48:12 2018 Revert "Filter GestureScrollUpdate during scroll fling." Original cl reviewed on https://chromium-review.googlesource.com/727464 Reason for revert: The original cl causes the following regression and with browser side fling and async wheel events it's not needed anymore. Regression: Touchpad scrolling disables wheel scrolling since GSU events generated from wheel scrolling get filtered here: https://cs.chromium.org/chromium/src/content/browser/renderer_host/input/gesture_event_queue.cc?q=Gesture_Event_queue&dr=C&l=130 fling_in_progress_ gets reset on GFC which is received when the user puts their fingers on touchpad, so if the user scrolls with touchpad first and then starts a subsequent wheel scrolling no GFC will be received after the GFS and fling_in_progress_ will be still true while GSU events from wheel scrolling are arriving. Bug: 817479 , 717205 Change-Id: I3997f756430df432aab37e5af77e1409dc8e0225 Reviewed-on: https://chromium-review.googlesource.com/1014415 Reviewed-by: Dave Tapuska <dtapuska@chromium.org> Commit-Queue: Sahel Sharifymoghaddam <sahel@chromium.org> Cr-Commit-Position: refs/heads/master@{#551330} [modify] https://crrev.com/fe4b9ea5050c23b5eb03258412f02ef42798084e/content/browser/renderer_host/input/gesture_event_queue.cc [modify] https://crrev.com/fe4b9ea5050c23b5eb03258412f02ef42798084e/content/browser/renderer_host/input/gesture_event_queue_unittest.cc
,
Apr 17 2018
I keep the bug as fixed since with browser side fling and async wheel events the fix in comment #28 is not needed anymore.
,
Apr 18 2018
This should be picked back to 67 right?
,
Apr 18 2018
Added M67 merge request to 817479 since that bug had a clearer reason for the merge request.
,
Apr 26 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cf859e1cadf1ece236aa114715a9be9de3d364a0 commit cf859e1cadf1ece236aa114715a9be9de3d364a0 Author: Sahel Sharify <sahel@chromium.org> Date: Thu Apr 26 17:05:44 2018 Revert "Filter GestureScrollUpdate during scroll fling." Original cl reviewed on https://chromium-review.googlesource.com/727464 Reason for revert: The original cl causes the following regression and with browser side fling and async wheel events it's not needed anymore. Regression: Touchpad scrolling disables wheel scrolling since GSU events generated from wheel scrolling get filtered here: https://cs.chromium.org/chromium/src/content/browser/renderer_host/input/gesture_event_queue.cc?q=Gesture_Event_queue&dr=C&l=130 fling_in_progress_ gets reset on GFC which is received when the user puts their fingers on touchpad, so if the user scrolls with touchpad first and then starts a subsequent wheel scrolling no GFC will be received after the GFS and fling_in_progress_ will be still true while GSU events from wheel scrolling are arriving. Bug: 817479 , 717205 Change-Id: I3997f756430df432aab37e5af77e1409dc8e0225 Reviewed-on: https://chromium-review.googlesource.com/1014415 Reviewed-by: Dave Tapuska <dtapuska@chromium.org> Commit-Queue: Sahel Sharifymoghaddam <sahel@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#551330}(cherry picked from commit fe4b9ea5050c23b5eb03258412f02ef42798084e) Reviewed-on: https://chromium-review.googlesource.com/1030575 Reviewed-by: Sahel Sharifymoghaddam <sahel@chromium.org> Cr-Commit-Position: refs/branch-heads/3396@{#327} Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428} [modify] https://crrev.com/cf859e1cadf1ece236aa114715a9be9de3d364a0/content/browser/renderer_host/input/gesture_event_queue.cc [modify] https://crrev.com/cf859e1cadf1ece236aa114715a9be9de3d364a0/content/browser/renderer_host/input/gesture_event_queue_unittest.cc
,
May 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/144d151e97463940eff5d36ee76c95925d4b8fa7 commit 144d151e97463940eff5d36ee76c95925d4b8fa7 Author: Sahel Sharify <sahel@chromium.org> Date: Wed May 02 19:43:37 2018 Merge the fix for crbug.com/817479 to M66. To make sure that the merge doesn't break anything, I cherry picked the fix manually, did a clean build and ran content_unittests locally. Revert "Filter GestureScrollUpdate during scroll fling." Original cl reviewed on https://chromium-review.googlesource.com/727464 Reason for revert: The original cl causes the following regression and with browser side fling and async wheel events it's not needed anymore. Regression: Touchpad scrolling disables wheel scrolling since GSU events generated from wheel scrolling get filtered here: https://cs.chromium.org/chromium/src/content/browser/renderer_host/input/gesture_event_queue.cc?q=Gesture_Event_queue&dr=C&l=130 fling_in_progress_ gets reset on GFC which is received when the user puts their fingers on touchpad, so if the user scrolls with touchpad first and then starts a subsequent wheel scrolling no GFC will be received after the GFS and fling_in_progress_ will be still true while GSU events from wheel scrolling are arriving. (cherry picked from commit fe4b9ea5050c23b5eb03258412f02ef42798084e) TBR=dtapuska@chromium.org Bug: 817479 , 717205 Change-Id: I405969389ed51bea5c64dfe82825d3e6c5840624 Reviewed-on: https://chromium-review.googlesource.com/1014415 Reviewed-by: Dave Tapuska <dtapuska@chromium.org> Commit-Queue: Sahel Sharifymoghaddam <sahel@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#551330} Reviewed-on: https://chromium-review.googlesource.com/1040646 Reviewed-by: Sahel Sharifymoghaddam <sahel@chromium.org> Cr-Commit-Position: refs/branch-heads/3359@{#787} Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276} [modify] https://crrev.com/144d151e97463940eff5d36ee76c95925d4b8fa7/content/browser/renderer_host/input/gesture_event_queue.cc [modify] https://crrev.com/144d151e97463940eff5d36ee76c95925d4b8fa7/content/browser/renderer_host/input/gesture_event_queue_unittest.cc |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by esprehn@chromium.org
, May 1 2017