ScrollTimeline: make endScrollOffset inclusive only when max-scroll-position
Before this CL, we always treated endScrollOffset as inclusive, against
the spec. The spec was recently updated to treat endScrollOffset as
inclusive but only when it is equal to max-offset:
https://github.com/WICG/scroll-animations/pull/37
Bug: 934989
Change-Id: I7cd9cf4619e804a54cef33ef8d3ec7395166bace
Reviewed-on: https://chromium-review.googlesource.com/c/1483682
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Majid Valipour <majidvp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#636673}diff --git a/cc/animation/scroll_timeline.cc b/cc/animation/scroll_timeline.cc
index 14cfc97..1674726 100644
--- a/cc/animation/scroll_timeline.cc
+++ b/cc/animation/scroll_timeline.cc
@@ -90,22 +90,21 @@
return std::numeric_limits<double>::quiet_NaN();
}
- // 4. If current scroll offset is greater than or equal to endScrollOffset,
- // return an unresolved time value if fill is none or backwards, or the
- // effective time range otherwise.
- // TODO(smcgruer): Implement |fill|.
- //
- // Note we deliberately break the spec here by only returning if the current
- // offset is strictly greater, as that is more in line with the web animation
- // spec. See https://github.com/WICG/scroll-animations/issues/19
- if (current_offset > resolved_end_scroll_offset) {
- return std::numeric_limits<double>::quiet_NaN();
+ // 4. If current scroll offset is greater than or equal to endScrollOffset:
+ if (current_offset >= resolved_end_scroll_offset) {
+ // If endScrollOffset is less than the maximum scroll offset of scrollSource
+ // in orientation and fill is none or backwards, return an unresolved time
+ // value.
+ // TODO(smcgruer): Implement |fill|.
+ if (resolved_end_scroll_offset < max_offset)
+ return std::numeric_limits<double>::quiet_NaN();
+
+ // Otherwise, return the effective time range.
+ return time_range_;
}
- // This is not by the spec, but avoids both negative current time and a
- // division by zero issue. See
- // https://github.com/WICG/scroll-animations/issues/20 and
- // https://github.com/WICG/scroll-animations/issues/21
+ // This is not by the spec, but avoids a negative current time.
+ // See https://github.com/WICG/scroll-animations/issues/20
if (resolved_start_scroll_offset >= resolved_end_scroll_offset) {
return std::numeric_limits<double>::quiet_NaN();
}
diff --git a/cc/animation/scroll_timeline_unittest.cc b/cc/animation/scroll_timeline_unittest.cc
index 0611014..3b855ee 100644
--- a/cc/animation/scroll_timeline_unittest.cc
+++ b/cc/animation/scroll_timeline_unittest.cc
@@ -230,9 +230,7 @@
EXPECT_TRUE(std::isnan(timeline.CurrentTime(scroll_tree(), false)));
SetScrollOffset(&property_trees(), scroller_id(),
gfx::ScrollOffset(0, time_range - 20));
- EXPECT_FLOAT_EQ(
- CalculateCurrentTime(time_range - 20, 0, end_scroll_offset, time_range),
- timeline.CurrentTime(scroll_tree(), false));
+ EXPECT_TRUE(std::isnan(timeline.CurrentTime(scroll_tree(), false)));
SetScrollOffset(&property_trees(), scroller_id(),
gfx::ScrollOffset(0, time_range - 50));
EXPECT_FLOAT_EQ(
@@ -245,6 +243,21 @@
timeline.CurrentTime(scroll_tree(), false));
}
+TEST_F(ScrollTimelineTest, CurrentTimeHandlesEndScrollOffsetInclusive) {
+ double time_range = 100;
+ const double end_scroll_offset =
+ content_size().height() - container_size().height();
+ ScrollTimeline timeline(scroller_id(), ScrollTimeline::ScrollDown,
+ base::nullopt, end_scroll_offset, time_range);
+
+ const double current_offset = end_scroll_offset;
+ SetScrollOffset(&property_trees(), scroller_id(),
+ gfx::ScrollOffset(0, current_offset));
+ EXPECT_FLOAT_EQ(
+ CalculateCurrentTime(current_offset, 0, end_scroll_offset, time_range),
+ timeline.CurrentTime(scroll_tree(), false));
+}
+
TEST_F(ScrollTimelineTest, CurrentTimeHandlesCombinedStartAndEndScrollOffset) {
double time_range = content_size().height() - container_size().height();
double start_scroll_offset = 20;
diff --git a/third_party/blink/renderer/core/animation/scroll_timeline.cc b/third_party/blink/renderer/core/animation/scroll_timeline.cc
index 63868fca..52066fd 100644
--- a/third_party/blink/renderer/core/animation/scroll_timeline.cc
+++ b/third_party/blink/renderer/core/animation/scroll_timeline.cc
@@ -157,23 +157,22 @@
return std::numeric_limits<double>::quiet_NaN();
}
- // 4. If current scroll offset is greater than or equal to endScrollOffset,
- // return an unresolved time value if fill is none or backwards, or the
- // effective time range otherwise.
- //
- // TODO(smcgruer): Implement |fill|.
- //
- // Note we deliberately break the spec here by only returning if the current
- // offset is strictly greater, as that is more in line with the web animation
- // spec. See https://github.com/WICG/scroll-animations/issues/19
- if (current_offset > resolved_end_scroll_offset) {
- return std::numeric_limits<double>::quiet_NaN();
+ // 4. If current scroll offset is greater than or equal to endScrollOffset:
+ if (current_offset >= resolved_end_scroll_offset) {
+ // If endScrollOffset is less than the maximum scroll offset of scrollSource
+ // in orientation and fill is none or backwards, return an unresolved time
+ // value.
+ // TODO(smcgruer): Implement |fill|.
+ if (resolved_end_scroll_offset < max_offset)
+ return std::numeric_limits<double>::quiet_NaN();
+
+ // Otherwise, return the effective time range.
+ is_null = false;
+ return time_range_;
}
- // This is not by the spec, but avoids both negative current time and a
- // divsion by zero issue. See
- // https://github.com/WICG/scroll-animations/issues/20 and
- // https://github.com/WICG/scroll-animations/issues/21
+ // This is not by the spec, but avoids a negative current time.
+ // See https://github.com/WICG/scroll-animations/issues/20
if (resolved_start_scroll_offset >= resolved_end_scroll_offset) {
return std::numeric_limits<double>::quiet_NaN();
}
diff --git a/third_party/blink/web_tests/external/wpt/scroll-animations/current-time-writing-modes.html b/third_party/blink/web_tests/external/wpt/scroll-animations/current-time-writing-modes.html
index a7e5551..89b78fc 100644
--- a/third_party/blink/web_tests/external/wpt/scroll-animations/current-time-writing-modes.html
+++ b/third_party/blink/web_tests/external/wpt/scroll-animations/current-time-writing-modes.html
@@ -300,9 +300,7 @@
'Length-based timeline after the endScrollOffset point');
scroller.scrollLeft = 20;
assert_equals(
- lengthScrollTimeline.currentTime,
- calculateCurrentTime(
- scrollerSize - 20, 0, scrollerSize - 20, scrollerSize),
+ lengthScrollTimeline.currentTime, null,
'Length-based timeline at the endScrollOffset point');
scroller.scrollLeft = 50;
assert_equals(
@@ -318,9 +316,7 @@
'Percentage-based timeline after the endScrollOffset point');
scroller.scrollLeft = 0.20 * scrollerSize;
assert_equals(
- percentageScrollTimeline.currentTime,
- calculateCurrentTime(
- 0.8 * scrollerSize, 0, 0.8 * scrollerSize, scrollerSize),
+ percentageScrollTimeline.currentTime, null,
'Percentage-based timeline at the endScrollOffset point');
scroller.scrollLeft = 0.4 * scrollerSize;
assert_equals(
@@ -336,9 +332,7 @@
'Calc-based timeline after the endScrollOffset point');
scroller.scrollLeft = 0.2 * scrollerSize - 5;
assert_equals(
- calcScrollTimeline.currentTime,
- calculateCurrentTime(
- 0.8 * scrollerSize + 5, 0, 0.8 * scrollerSize + 5, scrollerSize),
+ calcScrollTimeline.currentTime, null,
'Calc-based timeline at the endScrollOffset point');
scroller.scrollLeft = 0.2 * scrollerSize;
assert_equals(
@@ -347,4 +341,62 @@
0.8 * scrollerSize, 0, 0.8 * scrollerSize + 5, scrollerSize),
'Calc-based timeline before the endScrollOffset point');
}, 'currentTime handles endScrollOffset with direction: rtl correctly');
+
+test(function() {
+ const scrollerOverrides = new Map([['direction', 'rtl']]);
+ const scroller = setupScrollTimelineTest(scrollerOverrides);
+ // Set the timeRange such that currentTime maps directly to the value
+ // scrolled. The contents and scroller are square, so it suffices to compute
+ // one edge and use it for all the timelines.
+ const scrollerSize = scroller.scrollHeight - scroller.clientHeight;
+
+ // When the endScrollOffset is equal to the maximum scroll offset (and there
+ // are no fill modes), the endScrollOffset is treated as inclusive.
+ const inclusiveAutoScrollTimeline = new ScrollTimeline({
+ scrollSource: scroller,
+ timeRange: scrollerSize,
+ orientation: 'block',
+ endScrollOffset: 'auto'
+ });
+ const inclusiveLengthScrollTimeline = new ScrollTimeline({
+ scrollSource: scroller,
+ timeRange: scrollerSize,
+ orientation: 'block',
+ endScrollOffset: scrollerSize + 'px'
+ });
+ const inclusivePercentageScrollTimeline = new ScrollTimeline({
+ scrollSource: scroller,
+ timeRange: scrollerSize,
+ orientation: 'block',
+ endScrollOffset: '100%'
+ });
+ const inclusiveCalcScrollTimeline = new ScrollTimeline({
+ scrollSource: scroller,
+ timeRange: scrollerSize,
+ orientation: 'block',
+ endScrollOffset: 'calc(80% + ' + (0.2 * scrollerSize) + 'px)'
+ });
+
+ // With direction rtl offsets are inverted, such that scrollLeft ==
+ // scrollerSize is the 'zero' point for currentTime. However the
+ // endScrollOffset is an absolute distance along the offset, so doesn't need
+ // adjusting.
+
+ scroller.scrollLeft = 0;
+ let expectedCurrentTime = calculateCurrentTime(
+ scroller.scrollLeft, 0, scrollerSize, scrollerSize);
+
+ assert_equals(
+ inclusiveAutoScrollTimeline.currentTime, expectedCurrentTime,
+ 'Inclusive auto timeline at the endScrollOffset point');
+ assert_equals(
+ inclusiveLengthScrollTimeline.currentTime, expectedCurrentTime,
+ 'Inclusive length-based timeline at the endScrollOffset point');
+ assert_equals(
+ inclusivePercentageScrollTimeline.currentTime, expectedCurrentTime,
+ 'Inclusive percentage-based timeline at the endScrollOffset point');
+ assert_equals(
+ inclusiveCalcScrollTimeline.currentTime, expectedCurrentTime,
+ 'Inclusive calc-based timeline at the endScrollOffset point');
+}, 'currentTime handles endScrollOffset (inclusive case) with direction: rtl correctly');
</script>
diff --git a/third_party/blink/web_tests/external/wpt/scroll-animations/current-time.html b/third_party/blink/web_tests/external/wpt/scroll-animations/current-time.html
index 0dc2a6f..6ce4b4c 100644
--- a/third_party/blink/web_tests/external/wpt/scroll-animations/current-time.html
+++ b/third_party/blink/web_tests/external/wpt/scroll-animations/current-time.html
@@ -199,9 +199,7 @@
'Length-based timeline after the endScrollOffset point');
scroller.scrollTop = scrollerSize - 20;
assert_equals(
- lengthScrollTimeline.currentTime,
- calculateCurrentTime(
- scrollerSize - 20, 0, scrollerSize - 20, scrollerSize),
+ lengthScrollTimeline.currentTime, null,
'Length-based timeline at the endScrollOffset point');
scroller.scrollTop = scrollerSize - 50;
assert_equals(
@@ -217,9 +215,7 @@
'Percentage-based timeline after the endScrollOffset point');
scroller.scrollTop = 0.80 * scrollerSize;
assert_equals(
- percentageScrollTimeline.currentTime,
- calculateCurrentTime(
- scroller.scrollTop, 0, 0.8 * scrollerSize, scrollerSize),
+ percentageScrollTimeline.currentTime, null,
'Percentage-based timeline at the endScrollOffset point');
scroller.scrollTop = 0.50 * scrollerSize;
assert_equals(
@@ -235,9 +231,7 @@
'Calc-based timeline after the endScrollOffset point');
scroller.scrollTop = 0.8 * scrollerSize + 5;
assert_equals(
- calcScrollTimeline.currentTime,
- calculateCurrentTime(
- scroller.scrollTop, 0, 0.8 * scrollerSize + 5, scrollerSize),
+ calcScrollTimeline.currentTime, null,
'Calc-based timeline at the endScrollOffset point');
scroller.scrollTop = 0.5 * scrollerSize;
assert_equals(
@@ -254,6 +248,58 @@
// one edge and use it for all the timelines.
const scrollerSize = scroller.scrollHeight - scroller.clientHeight;
+ // When the endScrollOffset is equal to the maximum scroll offset (and there
+ // are no fill modes), the endScrollOffset is treated as inclusive.
+ const inclusiveAutoScrollTimeline = new ScrollTimeline({
+ scrollSource: scroller,
+ timeRange: scrollerSize,
+ orientation: 'block',
+ endScrollOffset: 'auto'
+ });
+ const inclusiveLengthScrollTimeline = new ScrollTimeline({
+ scrollSource: scroller,
+ timeRange: scrollerSize,
+ orientation: 'block',
+ endScrollOffset: scrollerSize + 'px'
+ });
+ const inclusivePercentageScrollTimeline = new ScrollTimeline({
+ scrollSource: scroller,
+ timeRange: scrollerSize,
+ orientation: 'block',
+ endScrollOffset: '100%'
+ });
+ const inclusiveCalcScrollTimeline = new ScrollTimeline({
+ scrollSource: scroller,
+ timeRange: scrollerSize,
+ orientation: 'block',
+ endScrollOffset: 'calc(80% + ' + (0.2 * scrollerSize) + 'px)'
+ });
+
+ scroller.scrollTop = scrollerSize;
+ let expectedCurrentTime = calculateCurrentTime(
+ scroller.scrollTop, 0, scrollerSize, scrollerSize);
+
+ assert_equals(
+ inclusiveAutoScrollTimeline.currentTime, expectedCurrentTime,
+ 'Inclusive auto timeline at the endScrollOffset point');
+ assert_equals(
+ inclusiveLengthScrollTimeline.currentTime, expectedCurrentTime,
+ 'Inclusive length-based timeline at the endScrollOffset point');
+ assert_equals(
+ inclusivePercentageScrollTimeline.currentTime, expectedCurrentTime,
+ 'Inclusive percentage-based timeline at the endScrollOffset point');
+ assert_equals(
+ inclusiveCalcScrollTimeline.currentTime, expectedCurrentTime,
+ 'Inclusive calc-based timeline at the endScrollOffset point');
+}, 'currentTime handles endScrollOffset correctly (inclusive cases)');
+
+test(function() {
+ const scroller = setupScrollTimelineTest();
+ // Set the timeRange such that currentTime maps directly to the value
+ // scrolled. The contents and scroller are square, so it suffices to compute
+ // one edge and use it for all the timelines.
+ const scrollerSize = scroller.scrollHeight - scroller.clientHeight;
+
const scrollTimeline = new ScrollTimeline({
scrollSource: scroller,
timeRange: scrollerSize,
