Reverting in https://chromium-review.googlesource.com/862108 .

Note: what appears to be happening is that sometimes the scroll is targeted specifically at one of the diffs. It can be easily reproduced by trying small vertical scrolls over and over again. Sometimes the diff will just jiggle up and down a little rather than causing the entire page to scroll.

The trivial revert failed:
https://chromium-review.googlesource.com/862108

sahel@, please help figure out what the next step is.

I just noticed a possibly related test flaking on Windows and filed  crbug.com/801427 . Feel free to dupe if related or ignore if not.

I tried using Canary (on a macbook pro with touchbar) and I can't get the scroll stuck as kbr@ describes. But I generally don't use it for reviews in this way so I don't know if the feel is slightly different.

Ok spoke too soon. I can't repro it when in fullscreen mode or when the window is maximized but when it is smaller than the full screen it appears to be an issue. It seems to think that one of the divs is scrollable. I showed it to sahel@ she is going to try to debug. 

In terms of reverting it may be that dependent tests have been added since Site isolation needs this feature for oopif to resolve some scrolling issues there.

I haven't been able to reproduce the bug, yet. But there are two possible explanations for what you have described in comment #2: a)Some of the diffs are unintentionally scrollable but only for some tiny amount, and once the scroll has latched to the diff div it doesn't propagate to the page and you see it as scroll getting ignored.
b) The latching happens regardless of the direction of the scrolling, if the scroll delta has a tiny value in horizontal direction then the diff div which is horizontally scrollable will consume the scroll delta and again it won't propagate to the page.

kbr@ would you please sent me the variations on your canary build?

Here they are:

bd23585d-f23d1dea
c134752e-e80f7cd5
fe69e053-94941f92
16e0dd70-3f4a17df
61fba06-ca7d8d80
2c7ce2fa-3f4a17df
f113d3c9-c0345a55
9041608a-f23d1dea
1e528f0f-ca7d8d80
b130ecb8-2e32ee7e
6025934e-3f4a17df
7c1bc906-b5809d46
d52c4ff7-d52c4ff7
3eb101d6-3f4a17df
47e5d3db-3d47f4f4
1c752ce9-ca7d8d80
34d450b1-1410f10
19c1fdaf-ca7d8d80
3042ad4b-ca7d8d80
121ae2bc-65969d67
57f575bb-f23d1dea
d0ecf1da-12de74c0
b72f69e9-f23d1dea
f347910c-65bced95
4b61504a-99913b15
77bbdddc-e08c81ee
5485fc4d-ca7d8d80
9773d3bd-4fbbdb50
93731dca-e89d496c
8e3b2dc5-93702590
a428bf14-5f8b0698
9e5c75f1-fb649e2f
350fabdd-1cf80f06
7e43b069-70ea8f25
2981bcb4-3d47f4f4
3de1fbf2-3f4a17df
f79cb77b-3f4a17df
47edb3-59ba3fef
4ea303a6-1f6c08b
bcc34a89-ca7d8d80
6e6e0c7e-bfd1fe3
d92562a9-ca7d8d80
2b33233e-d8253d6f
14c5a050-ca7d8d80
6973a1cf-3f4a17df
72606c4f-3f4a17df
ad6d27cc-15e2aa9a
757a5d98-215a0907
23496387-4ea78229
b2f0086-870290a7
2d871858-3f4a17df
344833e9-473e8c2e
3f273a97-e3ad1896
4bc337ce-afda42f4
57789a80-2d8bc89
9a2f4e5b-ca7d8d80
3ac60855-3ec2a267
f296190c-477de798
4442aae2-4ad60575
ed1d377-e1cc0f14
75f0f0a0-4ad60575
e2b18481-7158671e
e7e71889-4ad60575
34baa302-c306c9a2
f5fff3a2-3f4a17df
81c6897f-2d9ebb2e

My Chrome window is not maximized but is nearly the entire size of the display on my Retina MacBook Pro.

I was able to reproduce the bug on my Mac:

The root cause of the issue is that some of the diff divs are unintentionally vertically scrollable by one pixel. When wheel scroll latching is disabled you don't notice the bug since the scrolling gets propagated to the page once the div scrolls, but when you enable latching, the scroll latches to the div till the user lifts their fingers from the trackpad.
The issue goes away if you change the OS settings to always show scrollbars since the diff divs won't be vertically scrollable anymore.

Thanks sahel@ for triaging. The OS defaults are to hide scrollbars so Chrome has to work just as well in both modes. Is this mainly a bug in the Gerrit code review, or could it be more widespread? Can you commit to owning this bug and figuring out a solution? We shouldn't leave a P1 in the untriaged and unowned state.

Note that Safari switched to this "latching" model a number of years ago and said they saw only minimal complaints about this issue.  IIRC we also switch touch scrolling a couple years ago and similarly saw the occasional site with a one-pixel scroll bug that previously went unnoticed but was made obvious by this change.  IIRC we discussed this risk in the intent thread for this change and agreed that, barring substantial breakage, it was worth the cost.  Basically it takes a site bug which already shows up in other contexts (touch scrolling, wheel scrolling on Mac) and makes it show up consistently including wheel scrolling in Chrome.

Gerrit is the only example that has occurred to me so far (the feature has been enabled on finch trial since 63), I tested Gerrit on Firefox and Safari and the diff divs didn't scroll vertically on neither of the browsers so I am not sure if it is a website bug or a chrome layout issue.

There is an "overflow-x: auto" style on .gr-file-list-0 gr-diff.gr-file-list  If that rule is removed the issue goes away.

The real question is why does having an overflow-x setting cause vertical scrolling. I imagine this is to do something with the size of the layers and subpixel sizes maybe?

What is the next action on this bug? We need to continue to make progress on it.

It seems like overflow-x:auto forces the y direction to be clipped as well:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/style/ComputedStyle.h?gsn=IsOverflowVisible&l=1897

This is per spec: https://www.w3.org/TR/css-overflow-3/#overflow-properties

> "Computed value: as specified, except with visible computing to auto if one of overflow-x or overflow-y is not visible"

Took a closer look with dtapuska@, scrolling box has a fractional height. My guess would be that this is a floating point precision issue where we're off-by-one. I can't repro when using a LowDPI screen, but using --force-device-scale-factor=2 causes the repro. Blink reports Element.scrollHeight == Element.clientHeight for the scrolling box in question - I'm guessing the compositor rounds in the opposite direction when creating layers. Could someone from CC take a look?

Interestingly, the diffs only become scrollable if overlay scrollbars are enabled. If user using non-overlay scrollbars, the window itself is scrollable instead. If I enable overlay scrollbars on Linux (--enable-features=OverlayScrollbar) I get scrollable boxes for each individual diffs, but they don't have the 1px scroll issue seen on Mac so there's something Mac specific here.

What is the next step on this bug? I don't think we should ship stable with this regression. If PolyGerrit needs to be fixed, then we should make a change there.

This is likely a precision issue between Blink/CC. I'll take a closer look today.

Definitely related to subpixel - turning on --enable-use-zoom-for-dsf solves the issue as I would expect if that's the case. Doing so means Blink gets all the physical pixels so doesn't have to truncate between compositor commits.

I've minimized the link down to a bare repro. If using a low-DPI screen add --force-device-scale-factor=2. Scrolling over the green box causes a 1px scroll. I'll now look at where the truncation is happening.

Deleted: minimized.html 13.6 KB

minimized.html 13.6 KB View Download

Ok, found the cause. In CompositedLayerMapping::UpdateScrollingLayerGeometry (and ScrollingCoordinator also has an equivalent method that has the same issue, not sure why it's duplicated though), we compute the container/clip layer size using subpixel accumulation but the scrolling contents is computed without it. This causes us to round differently and the scrolling contents gets enlarged relative to the container.

For reference, this can be reproduced on Linux (and probably elsewhere) by disabling use-zoom-for-DSF using --enable-use-zoom-for-dsf=false.

I should have a fix up by tomorrow.

Friendly ping to get an update on this issue as it is marked  as stable blocker.

Thanks..!

Fix is in the commit queue and should land shortly. I'll merge to 65 when it hits and gets verified in Canary.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5ac37d8c5ceebfe88b1f6fbee7831b3b494387e6

commit 5ac37d8c5ceebfe88b1f6fbee7831b3b494387e6
Author: David Bokan <bokan@chromium.org>
Date: Tue Jan 30 16:54:58 2018

Make scroll bounds include subpixel accumulation

Bug:  801381 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I27e9f0ac5d372400bc2c72e7904a3908fc546454
Reviewed-on: https://chromium-review.googlesource.com/887647
Reviewed-by: Tien-Ren Chen <trchen@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532919}
[modify] https://crrev.com/5ac37d8c5ceebfe88b1f6fbee7831b3b494387e6/third_party/WebKit/Source/core/paint/compositing/CompositedLayerMapping.cpp
[modify] https://crrev.com/5ac37d8c5ceebfe88b1f6fbee7831b3b494387e6/third_party/WebKit/Source/core/paint/compositing/CompositedLayerMappingTest.cpp

Tried it out in Canary. It looks like my patch missed a case. We avoid the single pixel scroll at first, but if you scroll a box horizontally it then creates the 1px scroll again. I think I know what the issue is - I'll reland a fixed patch today.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/11f9b4b3267310fba209d7aa078032673832523a

commit 11f9b4b3267310fba209d7aa078032673832523a
Author: David Bokan <bokan@chromium.org>
Date: Thu Feb 01 15:20:13 2018

Revert "Make scroll bounds include subpixel accumulation"

This reverts commit 5ac37d8c5ceebfe88b1f6fbee7831b3b494387e6.

Reason for revert: Doesn't fix the issue entirely. Will revert and reland an updated fix.

Original change's description:
> Make scroll bounds include subpixel accumulation
> 
> Bug:  801381 
> Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
> Change-Id: I27e9f0ac5d372400bc2c72e7904a3908fc546454
> Reviewed-on: https://chromium-review.googlesource.com/887647
> Reviewed-by: Tien-Ren Chen <trchen@chromium.org>
> Commit-Queue: David Bokan <bokan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#532919}

TBR=bokan@chromium.org,trchen@chromium.org,chrishtr@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  801381 
Change-Id: I690eec01a6d5d6b999f8ba46de7800b27f06a5fa
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Reviewed-on: https://chromium-review.googlesource.com/896685
Reviewed-by: David Bokan <bokan@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533667}
[modify] https://crrev.com/11f9b4b3267310fba209d7aa078032673832523a/third_party/WebKit/Source/core/paint/compositing/CompositedLayerMapping.cpp
[modify] https://crrev.com/11f9b4b3267310fba209d7aa078032673832523a/third_party/WebKit/Source/core/paint/compositing/CompositedLayerMappingTest.cpp

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/19916d2994c0bcd456b159af78da4ab73af45270

commit 19916d2994c0bcd456b159af78da4ab73af45270
Author: David Bokan <bokan@chromium.org>
Date: Fri Feb 02 02:08:35 2018

[Reland] Make scroll bounds include subpixel accumulation

This relands r532919 which was reverted in r533667

The original patch worked only in the initial case but the corrected
layer bounds were clobbered in
ScrollingCoordinator::ScrollableAreaScrollLayerDidChange. Even though
CompositedLayerMapping::UpdateScrollingLayerGeometry is always called
after this method, in ScrollingCoordinator the bounds are set directly
on the WebLayer. In CLM we set the bounds on a GraphicsLayer which
caches the size itself. It'll only update the WebLayer bounds if the
new size differs from the cached size. Thus, even though the bounds
were changed in ScrollingCoordinator, CLM would early out since it
the GraphicsLayer size hasn't changed.

This CL applies the same fix in ScrollingCoordinator and also adds a
new test that covers the linked bug more end-to-end.

The original patch is uploaded in patchset 1 for easy diff'ing.

Bug:  801381 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I28f7088ef6b429875c6c984c45fd1c53cf813c02
Reviewed-on: https://chromium-review.googlesource.com/897962
Reviewed-by: Tien-Ren Chen <trchen@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533913}
[add] https://crrev.com/19916d2994c0bcd456b159af78da4ab73af45270/third_party/WebKit/LayoutTests/fast/compositor-wheel-scroll-latching/subpixel-accumulation.html
[modify] https://crrev.com/19916d2994c0bcd456b159af78da4ab73af45270/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
[modify] https://crrev.com/19916d2994c0bcd456b159af78da4ab73af45270/third_party/WebKit/Source/core/paint/compositing/CompositedLayerMapping.cpp
[modify] https://crrev.com/19916d2994c0bcd456b159af78da4ab73af45270/third_party/WebKit/Source/core/paint/compositing/CompositedLayerMappingTest.cpp

Confirmed the latest fix in Canary. Requesting merge to 65.

This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

 bokan@, could you pls confirm which cl you're requesting a merge to M65? And it is overall safe to merge? Thank you.

The patch is just the one in #32. I think it's safe, it's a very small, isolated change. 

Approving merge to M65 branch 3325 per comments #33 and #36. Please merge ASAP. Thank you.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cce698596b450f9f03dd480eb6eddb04987c0861

commit cce698596b450f9f03dd480eb6eddb04987c0861
Author: David Bokan <bokan@chromium.org>
Date: Mon Feb 05 18:57:56 2018

[Reland] Make scroll bounds include subpixel accumulation

This relands r532919 which was reverted in r533667

The original patch worked only in the initial case but the corrected
layer bounds were clobbered in
ScrollingCoordinator::ScrollableAreaScrollLayerDidChange. Even though
CompositedLayerMapping::UpdateScrollingLayerGeometry is always called
after this method, in ScrollingCoordinator the bounds are set directly
on the WebLayer. In CLM we set the bounds on a GraphicsLayer which
caches the size itself. It'll only update the WebLayer bounds if the
new size differs from the cached size. Thus, even though the bounds
were changed in ScrollingCoordinator, CLM would early out since it
the GraphicsLayer size hasn't changed.

This CL applies the same fix in ScrollingCoordinator and also adds a
new test that covers the linked bug more end-to-end.

The original patch is uploaded in patchset 1 for easy diff'ing.

Bug:  801381 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I28f7088ef6b429875c6c984c45fd1c53cf813c02
Reviewed-on: https://chromium-review.googlesource.com/897962
Reviewed-by: Tien-Ren Chen <trchen@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#533913}(cherry picked from commit 19916d2994c0bcd456b159af78da4ab73af45270)
Reviewed-on: https://chromium-review.googlesource.com/902303
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/branch-heads/3325@{#310}
Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369}
[add] https://crrev.com/cce698596b450f9f03dd480eb6eddb04987c0861/third_party/WebKit/LayoutTests/fast/compositor-wheel-scroll-latching/subpixel-accumulation.html
[modify] https://crrev.com/cce698596b450f9f03dd480eb6eddb04987c0861/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
[modify] https://crrev.com/cce698596b450f9f03dd480eb6eddb04987c0861/third_party/WebKit/Source/core/paint/compositing/CompositedLayerMapping.cpp
[modify] https://crrev.com/cce698596b450f9f03dd480eb6eddb04987c0861/third_party/WebKit/Source/core/paint/compositing/CompositedLayerMappingTest.cpp

The fix has been merged to beta branch so the issue should be fixed in the next beta build as well as current canary. Please chime in if you're still seeing similar issues.