Implement bounce suppression in ScrollAnchor.

Implement bounce suppression in ScrollAnchor.

An element can "bounce", by making the inverse of its last movement, when a
scroll anchoring adjustment interacts with a scroll event handler that changes
layout at a scroll offset threshold.

ScrollAnchor now detects bounces by tracking the most recent adjustment and the
element that triggered it (even if we have anchored to a different element since
then), and suppresses repeat bounces.

BUG=598233, 601906, 603376
Committed: https://crrev.com/2fd28b5102e73c49b3891d9bc7c05ffda979e2ea
Cr-Commit-Position: refs/heads/master@{#394552}

[skobes, 2016-05-13 01:30:01]

Patchset #1 (id:1) has been deleted

[skobes, 2016-05-13 23:38:02]

Patchset #2 (id:40001) has been deleted

[skobes, 2016-05-13 23:38:07]

Patchset #1 (id:20001) has been deleted

[skobes, 2016-05-13 23:46:24]

Description was changed from

==========
Implement bounce suppression in ScrollAnchor.

BUG=598233,601906,603376
==========

to

==========
Implement bounce suppression in ScrollAnchor.

An element can "bounce", by making the inverse of its last movement, when a
scroll anchoring adjustment interacts with a scroll event handler that changes
layout at a scroll offset threshold.

ScrollAnchor now detects bounces by tracking the most recent adjustment and the
element that triggered it (even if we have anchored to a different element since
then), and suppresses repeat bounces.

BUG=598233,601906,603376
==========

[skobes, 2016-05-14 00:56:05]

skobes@chromium.org changed reviewers:
+ ojan@chromium.org, ymalik@chromium.org

[ymalik, 2016-05-16 17:34:41]

https://codereview.chromium.org/1971423002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/layout/scroll-anchoring/onscroll-bouncing.html
(right):

https://codereview.chromium.org/1971423002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/layout/scroll-anchoring/onscroll-bouncing.html:2:
<script src="../../../resources/js-test.js"></script>
I think we want to write tests using testharness.js to be web-platform-tests
compliant when we ship scroll anchoring. I know there are calls to internals for
scroll events, but we can change that once we have webdriver support rather than
changing the entire test then.

https://codereview.chromium.org/1971423002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/layout/scroll-anchoring/onscroll-bouncing.html:64:
shouldBe("scrollY", expectedScrollY);
Do we want to check intermediate values as well?

https://codereview.chromium.org/1971423002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/layout/scroll-anchoring/onscroll-bouncing.html:80:
eventSender.mouseScrollBy(0, -8);
Nit: continuousMouseScrollBy to make the pixel count more clear?

https://codereview.chromium.org/1971423002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (right):

https://codereview.chromium.org/1971423002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:190: // user may have
scrolled since the last adjustment.
Could you please elaborate moe on why we update m_lastAdjusted here in response
to the user scrolls?

https://codereview.chromium.org/1971423002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:194: &&
m_lastAdjusted.m_corner == m_current.m_corner) {
Should we override the == operator in AnchorPoint? This is also used in ::adjust
L260

[skobes, 2016-05-18 01:24:31]

https://codereview.chromium.org/1971423002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/layout/scroll-anchoring/onscroll-bouncing.html
(right):

https://codereview.chromium.org/1971423002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/layout/scroll-anchoring/onscroll-bouncing.html:2:
<script src="../../../resources/js-test.js"></script>
On 2016/05/16 17:34:40, ymalik1 wrote:
> I think we want to write tests using testharness.js to be web-platform-tests
> compliant when we ship scroll anchoring. I know there are calls to internals
for
> scroll events, but we can change that once we have webdriver support rather
than
> changing the entire test then.

Done.

https://codereview.chromium.org/1971423002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/layout/scroll-anchoring/onscroll-bouncing.html:64:
shouldBe("scrollY", expectedScrollY);
On 2016/05/16 17:34:40, ymalik1 wrote:
> Do we want to check intermediate values as well?

I'm not sure we are guaranteed to get raf calls in perfect sync with
ScrollAnchor's adjustments during the bounce.  This tests the important aspects
of the behavior, namely that we reach the desired value and stay there after a
few frames.

https://codereview.chromium.org/1971423002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/layout/scroll-anchoring/onscroll-bouncing.html:80:
eventSender.mouseScrollBy(0, -8);
On 2016/05/16 17:34:40, ymalik1 wrote:
> Nit: continuousMouseScrollBy to make the pixel count more clear?

Done.

https://codereview.chromium.org/1971423002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (right):

https://codereview.chromium.org/1971423002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:190: // user may have
scrolled since the last adjustment.
On 2016/05/16 17:34:40, ymalik1 wrote:
> Could you please elaborate moe on why we update m_lastAdjusted here in
response
> to the user scrolls?

Clarified the comment.  The issue is that the offset is relative to the visible
rect.  If the user has scrolled, the visible rect has moved.  We don't have the
same issue with m_current because it is cleared on every scroll.

https://codereview.chromium.org/1971423002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:194: &&
m_lastAdjusted.m_corner == m_current.m_corner) {
On 2016/05/16 17:34:40, ymalik1 wrote:
> Should we override the == operator in AnchorPoint? This is also used in
::adjust
> L260

I don't think operator== should ignore m_savedRelativeOffset, which it would
have to do to be useful here.

[ymalik, 2016-05-18 18:07:45]

Thanks, lgtm!

[skobes, 2016-05-18 19:21:42]

Thanks, will wait for Ojan before submitting.

[ojan, 2016-05-18 21:20:46]

The CQ bit was checked by ojan@chromium.org

[ojan, 2016-05-18 21:20:47]

lgtm

[commit-bot: I haz the power, 2016-05-18 21:21:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1971423002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1971423002/80001

[commit-bot: I haz the power, 2016-05-18 21:27:29]

Description was changed from

==========
Implement bounce suppression in ScrollAnchor.

An element can "bounce", by making the inverse of its last movement, when a
scroll anchoring adjustment interacts with a scroll event handler that changes
layout at a scroll offset threshold.

ScrollAnchor now detects bounces by tracking the most recent adjustment and the
element that triggered it (even if we have anchored to a different element since
then), and suppresses repeat bounces.

BUG=598233,601906,603376
==========

to

==========
Implement bounce suppression in ScrollAnchor.

An element can "bounce", by making the inverse of its last movement, when a
scroll anchoring adjustment interacts with a scroll event handler that changes
layout at a scroll offset threshold.

ScrollAnchor now detects bounces by tracking the most recent adjustment and the
element that triggered it (even if we have anchored to a different element since
then), and suppresses repeat bounces.

BUG=598233,601906,603376
==========

[commit-bot: I haz the power, 2016-05-18 21:27:30]

Committed patchset #2 (id:80001)

[commit-bot: I haz the power, 2016-05-18 21:30:29]

Description was changed from

==========
Implement bounce suppression in ScrollAnchor.

An element can "bounce", by making the inverse of its last movement, when a
scroll anchoring adjustment interacts with a scroll event handler that changes
layout at a scroll offset threshold.

ScrollAnchor now detects bounces by tracking the most recent adjustment and the
element that triggered it (even if we have anchored to a different element since
then), and suppresses repeat bounces.

BUG=598233,601906,603376
==========

to

==========
Implement bounce suppression in ScrollAnchor.

An element can "bounce", by making the inverse of its last movement, when a
scroll anchoring adjustment interacts with a scroll event handler that changes
layout at a scroll offset threshold.

ScrollAnchor now detects bounces by tracking the most recent adjustment and the
element that triggered it (even if we have anchored to a different element since
then), and suppresses repeat bounces.

BUG=598233,601906,603376

Committed: https://crrev.com/2fd28b5102e73c49b3891d9bc7c05ffda979e2ea
Cr-Commit-Position: refs/heads/master@{#394552}
==========

[commit-bot: I haz the power, 2016-05-18 21:30:30]

Patchset 2 (id:??) landed as
https://crrev.com/2fd28b5102e73c49b3891d9bc7c05ffda979e2ea
Cr-Commit-Position: refs/heads/master@{#394552}