Limit ScrollAnchor to 20 adjustments between user scrolls. (issue 2118683002 by skobes@chromium.org)

0 views
Skip to first unread message

sko...@chromium.org

unread,
Jun 30, 2016, 8:10:04 PM6/30/16
to yma...@chromium.org, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org, oj...@chromium.org
Reviewers: ymalik
CL: https://codereview.chromium.org/2118683002/

Description:
Limit ScrollAnchor to 20 adjustments between user scrolls.

This somewhat mitigates the impact of pathological feedback loops with scroll
event handlers.

BUG=601906

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+62, -1 lines):
A third_party/WebKit/LayoutTests/fast/layout/scroll-anchoring/feedback-loop.html
M third_party/WebKit/Source/core/layout/ScrollAnchor.h
M third_party/WebKit/Source/core/layout/ScrollAnchor.cpp


Index: third_party/WebKit/LayoutTests/fast/layout/scroll-anchoring/feedback-loop.html
diff --git a/third_party/WebKit/LayoutTests/fast/layout/scroll-anchoring/feedback-loop.html b/third_party/WebKit/LayoutTests/fast/layout/scroll-anchoring/feedback-loop.html
new file mode 100644
index 0000000000000000000000000000000000000000..2bfe126d2c264769398928f19f913f217f593c61
--- /dev/null
+++ b/third_party/WebKit/LayoutTests/fast/layout/scroll-anchoring/feedback-loop.html
@@ -0,0 +1,53 @@
+<!DOCTYPE html>
+<script src="../../../resources/testharness.js"></script>
+<script src="../../../resources/testharnessreport.js"></script>
+<style>
+
+body { margin: 0; height: 5000px; }
+#evil { height: 200px; width: 200px; background-color: #fcc; }
+
+</style>
+<div id="evil"></div>
+<script>
+
+internals.runtimeFlags.scrollAnchoringEnabled = true;
+
+var kMaxAdjustments = 20; // Keep in sync with kMaxAdjustments in ScrollAnchor.cpp
+
+var evil = document.querySelector('#evil');
+onscroll = () => { evil.style.marginTop = scrollY + "px"; };
+
+var frame = () => new Promise((resolve) => { requestAnimationFrame(resolve); });
+
+var waitFor = function(condition, failmsg, deadline) {
+ if (!deadline) deadline = Date.now() + 1000;
+ if (condition()) return Promise.resolve();
+ else if (Date.now() > deadline) return Promise.reject(failmsg);
+ else return frame().then(() => waitFor(condition, failmsg, deadline));
+};
+
+var waitFrames = function(n, condition, failmsg) {
+ var p = Promise.resolve();
+ var check = () => (!condition || condition() ?
+ Promise.resolve() : Promise.reject(failmsg));
+ while (n--)
+ p = p.then(check).then(frame);
+ return p.then(check);
+};
+
+var scrollSettlesAt = function(expectedY) {
+ return waitFor(() => (scrollY == expectedY),
+ "scroll did not reach " + expectedY)
+ .then(() => waitFrames(3))
+ .then(() => waitFrames(3, () => (scrollY == expectedY),
+ "scroll did not stay at " + expectedY));
+};
+
+promise_test(function() {
+ var y = 10;
+ return Promise.resolve()
+ .then(() => { scrollTo(0, y); })
+ .then(() => scrollSettlesAt(y + kMaxAdjustments * y));
+}, "Scroll anchoring with scroll event handler feedback loop");
+
+</script>
Index: third_party/WebKit/Source/core/layout/ScrollAnchor.cpp
diff --git a/third_party/WebKit/Source/core/layout/ScrollAnchor.cpp b/third_party/WebKit/Source/core/layout/ScrollAnchor.cpp
index 8d1de54353f6f1b6df13ce4e08afc9fb9725ba54..48b648de6111c3ad0c8eb8f2742b2dce9cf49b3d 100644
--- a/third_party/WebKit/Source/core/layout/ScrollAnchor.cpp
+++ b/third_party/WebKit/Source/core/layout/ScrollAnchor.cpp
@@ -14,9 +14,12 @@ namespace blink {

using Corner = ScrollAnchor::Corner;

+static const int kMaxAdjustments = 20;
+
ScrollAnchor::ScrollAnchor(ScrollableArea* scroller)
: m_scroller(scroller)
, m_hasBounced(false)
+ , m_adjustmentCount(0)
{
ASSERT(m_scroller);
ASSERT(m_scroller->isFrameView() || m_scroller->isPaintLayerScrollableArea());
@@ -251,7 +254,8 @@ void ScrollAnchor::restore()
m_lastAdjusted.clear();
return;
}
- adjust(adjustment);
+ if (++m_adjustmentCount <= kMaxAdjustments)
+ adjust(adjustment);
}

void ScrollAnchor::adjust(IntSize adjustment)
@@ -274,6 +278,7 @@ void ScrollAnchor::adjust(IntSize adjustment)

void ScrollAnchor::clear()
{
+ m_adjustmentCount = 0;
m_current.clear();
}

Index: third_party/WebKit/Source/core/layout/ScrollAnchor.h
diff --git a/third_party/WebKit/Source/core/layout/ScrollAnchor.h b/third_party/WebKit/Source/core/layout/ScrollAnchor.h
index 0847701788399f3293dd5eeee4c35101e104469f..df5d4c45e3a8151374c8bae0c36cbc4fae4201ca 100644
--- a/third_party/WebKit/Source/core/layout/ScrollAnchor.h
+++ b/third_party/WebKit/Source/core/layout/ScrollAnchor.h
@@ -121,6 +121,9 @@ private:
// True iff the last adjustment was the exact opposite of the one before it.
// A bounce suggests a circular interaction with a scroll event handler.
bool m_hasBounced;
+
+ // Number of adjustments made since the last clear().
+ int m_adjustmentCount;
};

} // namespace blink


yma...@chromium.org

unread,
Jun 30, 2016, 9:07:11 PM6/30/16
to sko...@chromium.org, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org, oj...@chromium.org
lgtm with questions


https://codereview.chromium.org/2118683002/diff/1/third_party/WebKit/Source/core/layout/ScrollAnchor.cpp
File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (right):

https://codereview.chromium.org/2118683002/diff/1/third_party/WebKit/Source/core/layout/ScrollAnchor.cpp#newcode17
third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:17: static const
int kMaxAdjustments = 20;
How did you come up with this number? We do have some UMA metrics that
may be helpful in picking this.

If I understand correctly, with this change, in the techcunch header
bounce case, we will now see it stop after bouncing for a little bit?

https://codereview.chromium.org/2118683002/diff/1/third_party/WebKit/Source/core/layout/ScrollAnchor.cpp#newcode257
third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:257: if
(++m_adjustmentCount <= kMaxAdjustments)
Brief comment explaining the rationale behind this?

https://codereview.chromium.org/2118683002/

sko...@chromium.org

unread,
Jul 1, 2016, 1:28:27 PM7/1/16
to yma...@chromium.org, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org, oj...@chromium.org

https://codereview.chromium.org/2118683002/diff/1/third_party/WebKit/Source/core/layout/ScrollAnchor.cpp
File third_party/WebKit/Source/core/layout/ScrollAnchor.cpp (right):

https://codereview.chromium.org/2118683002/diff/1/third_party/WebKit/Source/core/layout/ScrollAnchor.cpp#newcode17
third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:17: static const
int kMaxAdjustments = 20;
On 2016/07/01 01:07:11, ymalik wrote:
> How did you come up with this number? We do have some UMA metrics that
may be
> helpful in picking this.
>
> If I understand correctly, with this change, in the techcunch header
bounce
> case, we will now see it stop after bouncing for a little bit?

I tried to pick something high enough to preserve adjustments when an ad
animates its expansion, but low enough that you don't have to wait a
long time to break out of the infinite loop case. The video I posted to
the bug shows what it looks like.


https://codereview.chromium.org/2118683002/diff/1/third_party/WebKit/Source/core/layout/ScrollAnchor.cpp#newcode257
third_party/WebKit/Source/core/layout/ScrollAnchor.cpp:257: if
(++m_adjustmentCount <= kMaxAdjustments)
On 2016/07/01 01:07:11, ymalik wrote:
> Brief comment explaining the rationale behind this?

commit-bot@chromium.org via codereview.chromium.org

unread,
Jul 1, 2016, 1:29:06 PM7/1/16
to sko...@chromium.org, yma...@chromium.org, commi...@chromium.org, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org, oj...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Jul 1, 2016, 2:49:35 PM7/1/16
to sko...@chromium.org, yma...@chromium.org, commi...@chromium.org, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org, oj...@chromium.org
Committed patchset #2 (id:20001)

https://codereview.chromium.org/2118683002/

commit-bot@chromium.org via codereview.chromium.org

unread,
Jul 1, 2016, 2:51:58 PM7/1/16
to sko...@chromium.org, yma...@chromium.org, commi...@chromium.org, chromium...@chromium.org, szager+la...@chromium.org, zol...@webkit.org, blink-revi...@chromium.org, pdr+renderi...@chromium.org, eae+bli...@chromium.org, leviw+re...@chromium.org, jchaffraix...@chromium.org, blink-...@chromium.org, oj...@chromium.org
Patchset 2 (id:??) landed as
https://crrev.com/34ae0389f0c660c23f691595341188acc5444e00
Cr-Commit-Position: refs/heads/master@{#403491}

https://codereview.chromium.org/2118683002/
Reply all
Reply to author
Forward
0 new messages