Diff
Modified: trunk/LayoutTests/ChangeLog (264890 => 264891)
--- trunk/LayoutTests/ChangeLog 2020-07-25 20:02:23 UTC (rev 264890)
+++ trunk/LayoutTests/ChangeLog 2020-07-25 22:33:12 UTC (rev 264891)
@@ -1,3 +1,17 @@
+2020-07-25 Simon Fraser <[email protected]>
+
+ Some scroll snap cleanup
+ https://bugs.webkit.org/show_bug.cgi?id=214788
+
+ Reviewed by Wenson Hsieh.
+
+ New results.
+
+ * css3/scroll-snap/scroll-snap-children-with-padding-expected.txt:
+ * css3/scroll-snap/scroll-snap-children-with-transforms-expected.txt:
+ * css3/scroll-snap/scroll-snap-offsets-expected.txt:
+ * css3/scroll-snap/scroll-snap-positions-expected.txt:
+
2020-07-24 Alex Christensen <[email protected]>
Null check frame in Document::dispatchDisabledAdaptationsDidChangeForMainFrame and a few other places
Modified: trunk/LayoutTests/css3/scroll-snap/scroll-snap-children-with-padding-expected.txt (264890 => 264891)
--- trunk/LayoutTests/css3/scroll-snap/scroll-snap-children-with-padding-expected.txt 2020-07-25 20:02:23 UTC (rev 264890)
+++ trunk/LayoutTests/css3/scroll-snap/scroll-snap-children-with-padding-expected.txt 2020-07-25 22:33:12 UTC (rev 264891)
@@ -1,2 +1,2 @@
-Scroll-snap offsets are: vertical = { 0, 10, 625, 1240, 1660, 2275, 2890, 2920 }
+Scroll-snap offsets are: vertical = { 0, 10, 625, 1240, 1660, 2275, 2890, 2900 }
Modified: trunk/LayoutTests/css3/scroll-snap/scroll-snap-children-with-transforms-expected.txt (264890 => 264891)
--- trunk/LayoutTests/css3/scroll-snap/scroll-snap-children-with-transforms-expected.txt 2020-07-25 20:02:23 UTC (rev 264890)
+++ trunk/LayoutTests/css3/scroll-snap/scroll-snap-children-with-transforms-expected.txt 2020-07-25 22:33:12 UTC (rev 264891)
@@ -1,5 +1,5 @@
-Scroll-snap offsets are: vertical = { 0, 82, 751, 1456, 2233, 2880, 3673 }
-After resizing, the offsets should not have changed: vertical = { 0, 82, 751, 1456, 2233, 2880, 3673 }
-After changing alignment to `start`, the offsets are: vertical = { 0, 688, 1423, 2086, 2880, 3526, 3673 }
-After resizing, the offsets should not have changed: vertical = { 0, 688, 1423, 2086, 2880, 3526, 3673 }
+Scroll-snap offsets are: vertical = { 0, 83, 752, 1457, 2233, 2880, 3673 }
+After resizing, the offsets should not have changed: vertical = { 0, 83, 752, 1457, 2233, 2880, 3673 }
+After changing alignment to `start`, the offsets are: vertical = { 0, 688, 1423, 2087, 2880, 3527, 3673 }
+After resizing, the offsets should not have changed: vertical = { 0, 688, 1423, 2087, 2880, 3527, 3673 }
Modified: trunk/LayoutTests/css3/scroll-snap/scroll-snap-offsets-expected.txt (264890 => 264891)
--- trunk/LayoutTests/css3/scroll-snap/scroll-snap-offsets-expected.txt 2020-07-25 20:02:23 UTC (rev 264890)
+++ trunk/LayoutTests/css3/scroll-snap/scroll-snap-offsets-expected.txt 2020-07-25 22:33:12 UTC (rev 264891)
@@ -3,12 +3,12 @@
Scroll-snap offsets for verticalTarget: vertical = { 0, 30, 60, 90, 120, 150 }
Scroll-snap offsets for horizontalBorderedTarget: horizontal = { 0, 30, 60, 90, 120, 150 }
Scroll-snap offsets for verticalBorderedTarget: vertical = { 0, 30, 60, 90, 120, 150 }
-Scroll-snap offsets for horizontalPaddedTarget: horizontal = { 0, 20, 50, 80, 110, 140, 170 }
-Scroll-snap offsets for verticalPaddedTarget: vertical = { 0, 15, 45, 75, 105, 135, 165, 174 }
-Scroll-snap offsets for horizontalBorderedPaddedTarget: horizontal = { 0, 20, 50, 80, 110, 140, 170 }
-Scroll-snap offsets for verticalBorderedPaddedTarget: vertical = { 0, 15, 45, 75, 105, 135, 165, 174 }
-Scroll-snap offsets for horizontalRotatedTarget: horizontal = { 0, 20, 50, 80, 110, 140, 170 }
-Scroll-snap offsets for verticalRotatedTarget: vertical = { 0, 15, 45, 75, 105, 135, 165, 174 }
+Scroll-snap offsets for horizontalPaddedTarget: horizontal = { 0, 20, 50, 80, 110, 140 }
+Scroll-snap offsets for verticalPaddedTarget: vertical = { 0, 15, 45, 75, 105, 135, 150 }
+Scroll-snap offsets for horizontalBorderedPaddedTarget: horizontal = { 0, 20, 50, 80, 110, 140 }
+Scroll-snap offsets for verticalBorderedPaddedTarget: vertical = { 0, 15, 45, 75, 105, 135, 150 }
+Scroll-snap offsets for horizontalRotatedTarget: horizontal = { 0, 20, 50, 80, 110, 140 }
+Scroll-snap offsets for verticalRotatedTarget: vertical = { 0, 15, 45, 75, 105, 135, 150 }
PASS successfullyParsed is true
TEST COMPLETE
Modified: trunk/LayoutTests/css3/scroll-snap/scroll-snap-positions-expected.txt (264890 => 264891)
--- trunk/LayoutTests/css3/scroll-snap/scroll-snap-positions-expected.txt 2020-07-25 20:02:23 UTC (rev 264890)
+++ trunk/LayoutTests/css3/scroll-snap/scroll-snap-positions-expected.txt 2020-07-25 22:33:12 UTC (rev 264891)
@@ -1,4 +1,4 @@
-Scroll-snap offsets for 'container': vertical = { 0, 72, 100, 330, 445, 560, 675, 762, 790, 992, 1020, 1250, 1310, 1365, 1425, 1480, 4425 }
+Scroll-snap offsets for 'container': vertical = { 0, 73, 100, 330, 445, 560, 675, 763, 790, 993, 1020, 1250, 1310, 1365, 1425, 1480, 4425 }
PASS successfullyParsed is true
TEST COMPLETE
Modified: trunk/Source/WebCore/ChangeLog (264890 => 264891)
--- trunk/Source/WebCore/ChangeLog 2020-07-25 20:02:23 UTC (rev 264890)
+++ trunk/Source/WebCore/ChangeLog 2020-07-25 22:33:12 UTC (rev 264891)
@@ -1,3 +1,32 @@
+2020-07-25 Simon Fraser <[email protected]>
+
+ Some scroll snap cleanup
+ https://bugs.webkit.org/show_bug.cgi?id=214788
+
+ Reviewed by Wenson Hsieh.
+
+ Mostly cleanup in updateSnapOffsetsForScrollableArea() related to getting the scroll limits,
+ which changes behavior.
+
+ updateSnapOffsetsForScrollableArea() was manually computing the scroll limits with
+ scrollingElementBox.scrollWidth() - scrollingElementBox.contentWidth(), but needs to use
+ the same computation that scrolling uses, namely scrollableArea.maximumScrollOffset().
+ This changes results, since the old code could compute offsets outside of the scroll limits.
+ It can also get the scrollPosition from the scrollableArea, instead of going via Element.
+
+ When computing scroll offsets, explicitly round now (which changes some test results).
+
+ * page/scrolling/AxisScrollSnapOffsets.cpp:
+ (WebCore::updateSnapOffsetsForScrollableArea):
+ * platform/ScrollAnimator.cpp:
+ (WebCore::ScrollAnimator::updateActiveScrollSnapIndexForOffset): Explicitly round.
+ * platform/ScrollTypes.h:
+ * platform/ScrollableArea.h:
+ * platform/cocoa/ScrollController.h:
+ * platform/cocoa/ScrollController.mm:
+ (WebCore::ScrollController::setActiveScrollSnapIndicesForOffset): Use ScrollOffset instead
+ of x,y.
+
2020-07-25 Kenneth Russell <[email protected]>
[WebGL2] Context state updates
Modified: trunk/Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp (264890 => 264891)
--- trunk/Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp 2020-07-25 20:02:23 UTC (rev 264890)
+++ trunk/Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp 2020-07-25 22:33:12 UTC (rev 264891)
@@ -190,10 +190,10 @@
HashSet<float> seenHorizontalSnapOffsets;
bool hasHorizontalSnapOffsets = scrollSnapType.axis == ScrollSnapAxis::Both || scrollSnapType.axis == ScrollSnapAxis::XAxis || scrollSnapType.axis == ScrollSnapAxis::Inline;
bool hasVerticalSnapOffsets = scrollSnapType.axis == ScrollSnapAxis::Both || scrollSnapType.axis == ScrollSnapAxis::YAxis || scrollSnapType.axis == ScrollSnapAxis::Block;
- auto maxScrollLeft = scrollingElementBox.scrollWidth() - scrollingElementBox.contentWidth();
- auto maxScrollTop = scrollingElementBox.scrollHeight() - scrollingElementBox.contentHeight();
- LayoutPoint containerScrollOffset(scrollingElementBox.scrollLeft(), scrollingElementBox.scrollTop());
+ auto maxScrollOffset = scrollableArea.maximumScrollOffset();
+ auto scrollPosition = LayoutPoint { scrollableArea.scrollPosition() };
+
// The bounds of the scrolling container's snap port, where the top left of the scrolling container's border box is the origin.
auto scrollSnapPort = computeScrollSnapPortOrAreaRect(scrollingElementBox.paddingBoxRect(), scrollingElementStyle.scrollPadding(), InsetOrOutset::Inset);
LOG_WITH_STREAM(ScrollSnap, stream << "Computing scroll snap offsets for " << scrollableArea << " in snap port " << scrollSnapPort);
@@ -204,12 +204,13 @@
// The bounds of the child element's snap area, where the top left of the scrolling container's border box is the origin.
// The snap area is the bounding box of the child element's border box, after applying transformations.
auto scrollSnapArea = LayoutRect(child->localToContainerQuad(FloatQuad(child->borderBoundingBox()), scrollingElement.renderBox()).boundingBox());
- scrollSnapArea.moveBy(containerScrollOffset);
+ scrollSnapArea.moveBy(scrollPosition);
scrollSnapArea = computeScrollSnapPortOrAreaRect(scrollSnapArea, child->style().scrollSnapMargin(), InsetOrOutset::Outset);
LOG_WITH_STREAM(ScrollSnap, stream << " Considering scroll snap target area " << scrollSnapArea);
auto alignment = child->style().scrollSnapAlign();
if (hasHorizontalSnapOffsets && alignment.x != ScrollSnapAxisAlignType::None) {
- auto absoluteScrollOffset = clampTo<LayoutUnit>(computeScrollSnapAlignOffset(scrollSnapArea.x(), scrollSnapArea.width(), alignment.x) - computeScrollSnapAlignOffset(scrollSnapPort.x(), scrollSnapPort.width(), alignment.x), 0, maxScrollLeft);
+ auto absoluteScrollXPosition = computeScrollSnapAlignOffset(scrollSnapArea.x(), scrollSnapArea.width(), alignment.x) - computeScrollSnapAlignOffset(scrollSnapPort.x(), scrollSnapPort.width(), alignment.x);
+ auto absoluteScrollOffset = clampTo<int>(scrollableArea.scrollOffsetFromPosition({ roundToInt(absoluteScrollXPosition), 0 }).x(), 0, maxScrollOffset.x());
if (!seenHorizontalSnapOffsets.contains(absoluteScrollOffset)) {
seenHorizontalSnapOffsets.add(absoluteScrollOffset);
horizontalSnapOffsets.append(absoluteScrollOffset);
@@ -216,7 +217,8 @@
}
}
if (hasVerticalSnapOffsets && alignment.y != ScrollSnapAxisAlignType::None) {
- auto absoluteScrollOffset = clampTo<LayoutUnit>(computeScrollSnapAlignOffset(scrollSnapArea.y(), scrollSnapArea.height(), alignment.y) - computeScrollSnapAlignOffset(scrollSnapPort.y(), scrollSnapPort.height(), alignment.y), 0, maxScrollTop);
+ auto absoluteScrollYPosition = computeScrollSnapAlignOffset(scrollSnapArea.y(), scrollSnapArea.height(), alignment.y) - computeScrollSnapAlignOffset(scrollSnapPort.y(), scrollSnapPort.height(), alignment.y);
+ auto absoluteScrollOffset = clampTo<int>(scrollableArea.scrollOffsetFromPosition({ 0, roundToInt(absoluteScrollYPosition) }).y(), 0, maxScrollOffset.y());
if (!seenVerticalSnapOffsets.contains(absoluteScrollOffset)) {
seenVerticalSnapOffsets.add(absoluteScrollOffset);
verticalSnapOffsets.append(absoluteScrollOffset);
@@ -225,7 +227,7 @@
}
if (!horizontalSnapOffsets.isEmpty()) {
- adjustAxisSnapOffsetsForScrollExtent(horizontalSnapOffsets, maxScrollLeft);
+ adjustAxisSnapOffsetsForScrollExtent(horizontalSnapOffsets, maxScrollOffset.x());
LOG_WITH_STREAM(ScrollSnap, stream << " => Computed horizontal scroll snap offsets: " << horizontalSnapOffsets);
LOG_WITH_STREAM(ScrollSnap, stream << " => Computed horizontal scroll snap offset ranges: " << horizontalSnapOffsetRanges);
if (scrollSnapType.strictness == ScrollSnapStrictness::Proximity)
@@ -237,7 +239,7 @@
scrollableArea.clearHorizontalSnapOffsets();
if (!verticalSnapOffsets.isEmpty()) {
- adjustAxisSnapOffsetsForScrollExtent(verticalSnapOffsets, maxScrollTop);
+ adjustAxisSnapOffsetsForScrollExtent(verticalSnapOffsets, maxScrollOffset.y());
LOG_WITH_STREAM(ScrollSnap, stream << " => Computed vertical scroll snap offsets: " << verticalSnapOffsets);
LOG_WITH_STREAM(ScrollSnap, stream << " => Computed vertical scroll snap offset ranges: " << verticalSnapOffsetRanges);
if (scrollSnapType.strictness == ScrollSnapStrictness::Proximity)
Modified: trunk/Source/WebCore/platform/ScrollAnimator.cpp (264890 => 264891)
--- trunk/Source/WebCore/platform/ScrollAnimator.cpp 2020-07-25 20:02:23 UTC (rev 264890)
+++ trunk/Source/WebCore/platform/ScrollAnimator.cpp 2020-07-25 22:33:12 UTC (rev 264891)
@@ -201,7 +201,7 @@
{
#if ENABLE(CSS_SCROLL_SNAP)
// FIXME: Needs offset/position disambiguation.
- m_scrollController.setActiveScrollSnapIndicesForOffset(m_currentPosition.x(), m_currentPosition.y());
+ m_scrollController.setActiveScrollSnapIndicesForOffset(roundedIntPoint(currentPosition()));
if (m_scrollController.activeScrollSnapIndexDidChange()) {
m_scrollableArea.setCurrentHorizontalSnapPointIndex(m_scrollController.activeScrollSnapIndexForAxis(ScrollEventAxis::Horizontal));
m_scrollableArea.setCurrentVerticalSnapPointIndex(m_scrollController.activeScrollSnapIndexForAxis(ScrollEventAxis::Vertical));
Modified: trunk/Source/WebCore/platform/ScrollTypes.h (264890 => 264891)
--- trunk/Source/WebCore/platform/ScrollTypes.h 2020-07-25 20:02:23 UTC (rev 264890)
+++ trunk/Source/WebCore/platform/ScrollTypes.h 2020-07-25 22:33:12 UTC (rev 264891)
@@ -25,6 +25,7 @@
#pragma once
+#include "IntPoint.h"
#include <cstdint>
#include <wtf/Assertions.h>
#include <wtf/EnumTraits.h>
@@ -35,6 +36,11 @@
namespace WebCore {
+// scrollPosition is in content coordinates (0,0 is at scrollOrigin), so may have negative components.
+using ScrollPosition = IntPoint;
+// scrollOffset() is the value used by scrollbars (min is 0,0), and should never have negative components.
+using ScrollOffset = IntPoint;
+
enum class ScrollType : bool {
User,
Programmatic
Modified: trunk/Source/WebCore/platform/ScrollableArea.h (264890 => 264891)
--- trunk/Source/WebCore/platform/ScrollableArea.h 2020-07-25 20:02:23 UTC (rev 264890)
+++ trunk/Source/WebCore/platform/ScrollableArea.h 2020-07-25 22:33:12 UTC (rev 264891)
@@ -47,11 +47,6 @@
class GraphicsLayer;
class TiledBacking;
-// scrollPosition is in content coordinates (0,0 is at scrollOrigin), so may have negative components.
-typedef IntPoint ScrollPosition;
-// scrollOffset() is the value used by scrollbars (min is 0,0), and should never have negative components.
-typedef IntPoint ScrollOffset;
-
inline int offsetForOrientation(ScrollOffset offset, ScrollbarOrientation orientation)
{
switch (orientation) {
Modified: trunk/Source/WebCore/platform/cocoa/ScrollController.h (264890 => 264891)
--- trunk/Source/WebCore/platform/cocoa/ScrollController.h 2020-07-25 20:02:23 UTC (rev 264890)
+++ trunk/Source/WebCore/platform/cocoa/ScrollController.h 2020-07-25 22:33:12 UTC (rev 264891)
@@ -146,7 +146,7 @@
#if ENABLE(CSS_SCROLL_SNAP)
void updateScrollSnapPoints(ScrollEventAxis, const Vector<LayoutUnit>&, const Vector<ScrollOffsetRange<LayoutUnit>>&);
void setActiveScrollSnapIndexForAxis(ScrollEventAxis, unsigned);
- void setActiveScrollSnapIndicesForOffset(int x, int y);
+ void setActiveScrollSnapIndicesForOffset(ScrollOffset);
bool activeScrollSnapIndexDidChange() const { return m_activeScrollSnapIndexDidChange; }
void setScrollSnapIndexDidChange(bool state) { m_activeScrollSnapIndexDidChange = state; }
unsigned activeScrollSnapIndexForAxis(ScrollEventAxis) const;
Modified: trunk/Source/WebCore/platform/cocoa/ScrollController.mm (264890 => 264891)
--- trunk/Source/WebCore/platform/cocoa/ScrollController.mm 2020-07-25 20:02:23 UTC (rev 264890)
+++ trunk/Source/WebCore/platform/cocoa/ScrollController.mm 2020-07-25 22:33:12 UTC (rev 264891)
@@ -841,13 +841,13 @@
setActiveScrollSnapIndexForAxis(axis, activeIndex);
}
-void ScrollController::setActiveScrollSnapIndicesForOffset(int x, int y)
+void ScrollController::setActiveScrollSnapIndicesForOffset(ScrollOffset offset)
{
if (!usesScrollSnap())
return;
- setNearestScrollSnapIndexForAxisAndOffset(ScrollEventAxis::Horizontal, x);
- setNearestScrollSnapIndexForAxisAndOffset(ScrollEventAxis::Vertical, y);
+ setNearestScrollSnapIndexForAxisAndOffset(ScrollEventAxis::Horizontal, offset.x());
+ setNearestScrollSnapIndexForAxisAndOffset(ScrollEventAxis::Vertical, offset.y());
}
#endif
