Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Scroll snap sometimes jumps back to the wrong place on stevejobsarchi… 
…ve.com

https://bugs.webkit.org/show_bug.cgi?id=255492
rdar://107885376

Reviewed by Wenson Hsieh.

259696@main added some logic that attempts to re-snap after layout when multiple boxes were snapped,
adding a `m_currentlySnappedBoxes` member to `ScrollSnapAnimatorState`.

However, `m_currentlySnappedBoxes` was only updated in the `resnapAfterLayout` code path, not when
scrolling moved you to a new snap point. That resulted in `resnapAfterLayout` sometimes returning
you to a stale location if you'd scrolled to a new snap point since the last time
`resnapAfterLayout` was run, especially when hitting the "multiple boxes were snapped" clause.

It's troublesome to have both `m_currentlySnappedBoxes` and a `snapTargetID` in each SnapOffset (a
future patch will clean this up). But for now, ensure that `m_currentlySnappedBoxes` is updated on
each scroll-related snap as well as resnapping after layout.

* LayoutTests/css3/scroll-snap/resnap-after-layout-expected.txt: Added.
* LayoutTests/css3/scroll-snap/resnap-after-layout.html: Added.
* LayoutTests/platform/gtk/TestExpectations:
* LayoutTests/platform/ios-wk2/TestExpectations:
* LayoutTests/platform/wpe/TestExpectations:
* Source/WebCore/platform/ScrollSnapAnimatorState.cpp:
(WebCore::ScrollSnapAnimatorState::setActiveSnapIndexForAxis):
(WebCore::ScrollSnapAnimatorState::updateCurrentlySnappedBoxes):
(WebCore::chooseBoxToResnapTo):
(WebCore::ScrollSnapAnimatorState::resnapAfterLayout):
(WebCore::ScrollSnapAnimatorState::setNearestScrollSnapIndexForAxisAndOffsetInternal):
(WebCore::ScrollSnapAnimatorState::setNearestScrollSnapIndexForOffset):
(WebCore::ScrollSnapAnimatorState::chooseBoxToResnapTo const): Deleted.
(WebCore::ScrollSnapAnimatorState::setNearestScrollSnapIndexForAxisAndOffset): Deleted.
* Source/WebCore/platform/ScrollSnapAnimatorState.h: Some functions can be private.
(WebCore::ScrollSnapAnimatorState::setActiveSnapIndexForAxisInternal): The "internal" implies that it doesn't update m_currentlySnappedBoxes.
(WebCore::ScrollSnapAnimatorState::setActiveSnapIndexForAxis): Deleted.
* Source/WebCore/platform/ScrollableArea.cpp:
(WebCore::ScrollableArea::resnapAfterLayout): Improved logging.
(WebCore::ScrollableArea::doPostThumbMoveSnapping): Improved logging.

Canonical link: https://commits.webkit.org/263097@main
  • Loading branch information
@smfr
smfr committed Apr 18, 2023
1 parent a5421d7 commit ee03689
Show file tree
Hide file tree
Showing 8 changed files with 163 additions and 41 deletions.
6 changes: 6 additions & 0 deletions LayoutTests/css3/scroll-snap/resnap-after-layout-expected.txt
View file
@@ -0,0 +1,6 @@
layout trigger
PASS window.scrollY is 1200
PASS successfullyParsed is true

TEST COMPLETE

90 changes: 90 additions & 0 deletions LayoutTests/css3/scroll-snap/resnap-after-layout.html
View file
@@ -0,0 +1,90 @@
<!DOCTYPE html>
<html>
<head>
<style>
:root {
scroll-snap-type: block mandatory;
font-size: 24pt;
}

* {
box-sizing: border-box;
}

html, body {
width: 100%;
height: 100%;
}

body {
margin: 0;
background-image: repeating-linear-gradient(transparent, silver 400px);
}

.target {
scroll-snap-align: start;
border: 1px solid black;
}

body > div {
padding: 100px;
text-align: center;
color: white;
width: 80%;
height: 100%;
}

.multiple > div {
height: 100%;
width: 50%;
float: left;
}

body.changed .layout-trigger {
height: 20px;
}
</style>
<script src="../../resources/js-test-pre.js"></script>
<script src="../../resources/ui-helper.js"></script>
<script>
jsTestIsAsync = true;

async function startTest()
{
// Scroll to first "multiple" snap point at 700.
await UIHelper.mouseWheelScrollAt(100, 100, 0, -10, 0, -40);
forceLayout();

// Scroll to second "single" snap point at 1200.
await UIHelper.mouseWheelScrollAt(100, 100, 0, -10, 0, -40);
forceLayout();

await UIHelper.renderingUpdate();
shouldBe('window.scrollY', '1200');

finishJSTest();
}

function forceLayout()
{
document.body.classList.toggle('changed');
document.body.offsetHeight;
}

window.addEventListener('load', () => {
setTimeout(startTest, 0);
}, false);
</script>
</head>
<body>
<div class="target" style='background-color: red'></div>
<div class="multiple" style='background-color: orange'>
<div class="target" style='background-color: yellow'></div>
<div class="target" style='background-color: fuchsia'></div>
</div>
<div class="target" style='background-color: green'></div>
<div class="layout-trigger">layout trigger</div>
<div id="console"></div>
<script src="../../resources/js-test-post.js"></script>
</body>
</html>
1 change: 1 addition & 0 deletions LayoutTests/platform/gtk/TestExpectations
View file
Expand Up @@ -1781,6 +1781,7 @@ Bug(GTK) fast/events/wheel/platform-wheelevent-paging-y-in-non-scrolling-div.htm
Bug(GTK) fast/events/wheel/platform-wheelevent-paging-y-in-non-scrolling-page.html [ Failure ]
Bug(GTK) fast/events/wheel/platform-wheelevent-paging-y-in-scrolling-div.html [ Failure ]
Bug(GTK) fast/events/wheel/platform-wheelevent-paging-y-in-scrolling-page.html [ Failure ]
css3/scroll-snap/resnap-after-layout.html [ Skip ]

# Need to add functionality to DumpRenderTree to handle scrollbar policy changes
Bug(GTK) fast/overflow/scrollbar-restored-and-then-locked.html [ Failure ]
Expand Down
1 change: 1 addition & 0 deletions LayoutTests/platform/ios-wk2/TestExpectations
View file
Expand Up @@ -967,6 +967,7 @@ editing/selection/select-out-of-floated-non-editable-12.html [ Skip ]
css3/scroll-snap/scroll-snap-click-scrollbar-gutter.html [ Skip ]
css3/scroll-snap/scroll-snap-wheel-event.html [ Skip ]
css3/scroll-snap/scroll-snap-discrete-wheel-event-in-mainframe.html [ Skip ]
css3/scroll-snap/resnap-after-layout.html [ Skip ]

webkit.org/b/192088 media/no-fullscreen-when-hidden.html [ Failure Pass ]

Expand Down
1 change: 1 addition & 0 deletions LayoutTests/platform/wpe/TestExpectations
View file
Expand Up @@ -617,6 +617,7 @@ fast/events/ondrop-text-html.html
# WTR needs an implementation for eventSender.continuousMouseScrollBy
# https://bugs.webkit.org/show_bug.cgi?id=69417
fast/events/wheel/wheelevent-direction-inverted-from-device.html
css3/scroll-snap/resnap-after-layout.html [ Skip ]

# These are failures that will be enabled once the relevant parts of implementation land.
webkit.org/b/133122 crypto/workers/subtle/aes-indexeddb.html [ Skip ]
Expand Down
70 changes: 42 additions & 28 deletions Source/WebCore/platform/ScrollSnapAnimatorState.cpp
View file
Expand Up @@ -141,21 +141,37 @@ HashSet<ElementIdentifier> ScrollSnapAnimatorState::currentlySnappedBoxes(const
return snappedBoxIDs;
}

ElementIdentifier ScrollSnapAnimatorState::chooseBoxToResnapTo(const Vector<SnapOffset<LayoutUnit>>& horizontalOffsets, const Vector<SnapOffset<LayoutUnit>>& verticalOffsets) const
void ScrollSnapAnimatorState::setActiveSnapIndexForAxis(ScrollEventAxis axis, std::optional<unsigned> index)
{
auto found = std::find_if(horizontalOffsets.begin(), horizontalOffsets.end(), [this](SnapOffset<LayoutUnit> p) -> bool {
return m_currentlySnappedBoxes.contains(p.snapTargetID) && p.isFocused;
setActiveSnapIndexForAxisInternal(axis, index);
updateCurrentlySnappedBoxes();
}

void ScrollSnapAnimatorState::updateCurrentlySnappedBoxes()
{
auto horizontalOffsets = currentlySnappedOffsetsForAxis(ScrollEventAxis::Horizontal);
auto verticalOffsets = currentlySnappedOffsetsForAxis(ScrollEventAxis::Vertical);

m_currentlySnappedBoxes = currentlySnappedBoxes(horizontalOffsets, verticalOffsets);
}

static ElementIdentifier chooseBoxToResnapTo(const HashSet<ElementIdentifier>& snappedBoxes, const Vector<SnapOffset<LayoutUnit>>& horizontalOffsets, const Vector<SnapOffset<LayoutUnit>>& verticalOffsets)
{
ASSERT(snappedBoxes.size());

auto found = std::find_if(horizontalOffsets.begin(), horizontalOffsets.end(), [&snappedBoxes](SnapOffset<LayoutUnit> p) -> bool {
return snappedBoxes.contains(p.snapTargetID) && p.isFocused;
});
if (found != horizontalOffsets.end())
return found->snapTargetID;

found = std::find_if(verticalOffsets.begin(), verticalOffsets.end(), [this](SnapOffset<LayoutUnit> p) -> bool {
return m_currentlySnappedBoxes.contains(p.snapTargetID) && p.isFocused;
found = std::find_if(verticalOffsets.begin(), verticalOffsets.end(), [&snappedBoxes](SnapOffset<LayoutUnit> p) -> bool {
return snappedBoxes.contains(p.snapTargetID) && p.isFocused;
});
if (found != verticalOffsets.end())
return found->snapTargetID;

return *m_currentlySnappedBoxes.begin();
return *snappedBoxes.begin();
}

bool ScrollSnapAnimatorState::resnapAfterLayout(ScrollOffset scrollOffset, const ScrollExtents& scrollExtents, float pageScale)
Expand All @@ -166,56 +182,54 @@ bool ScrollSnapAnimatorState::resnapAfterLayout(ScrollOffset scrollOffset, const
auto snapOffsetsVertical = snapOffsetsForAxis(ScrollEventAxis::Vertical);
auto snapOffsetsHorizontal = snapOffsetsForAxis(ScrollEventAxis::Horizontal);

auto previouslySnappedBoxes = std::exchange(m_currentlySnappedBoxes, { });

// Check if we need to set the current indices
if (!activeVerticalIndex || *activeVerticalIndex >= snapOffsetsForAxis(ScrollEventAxis::Vertical).size()) {
snapPointChanged |= setNearestScrollSnapIndexForAxisAndOffset(ScrollEventAxis::Vertical, scrollOffset, scrollExtents, pageScale);
snapPointChanged |= setNearestScrollSnapIndexForAxisAndOffsetInternal(ScrollEventAxis::Vertical, scrollOffset, scrollExtents, pageScale);
activeVerticalIndex = activeSnapIndexForAxis(ScrollEventAxis::Vertical);
}
if (!activeHorizontalIndex || *activeHorizontalIndex >= snapOffsetsForAxis(ScrollEventAxis::Horizontal).size()) {
snapPointChanged |= setNearestScrollSnapIndexForAxisAndOffset(ScrollEventAxis::Horizontal, scrollOffset, scrollExtents, pageScale);
snapPointChanged |= setNearestScrollSnapIndexForAxisAndOffsetInternal(ScrollEventAxis::Horizontal, scrollOffset, scrollExtents, pageScale);
activeHorizontalIndex = activeSnapIndexForAxis(ScrollEventAxis::Horizontal);
}

auto horizontalOffsets = currentlySnappedOffsetsForAxis(ScrollEventAxis::Horizontal);
auto verticalOffsets = currentlySnappedOffsetsForAxis(ScrollEventAxis::Vertical);

auto currentlySnapped = currentlySnappedBoxes(horizontalOffsets, verticalOffsets);

auto wasSnappedToMultipleBoxes = m_currentlySnappedBoxes.size() > 1;
auto currentlySnappedToMultipleBoxes = currentlySnapped.size() > 1;
updateCurrentlySnappedBoxes();
LOG_WITH_STREAM(ScrollSnap, stream << "ScrollSnapAnimatorState::resnapAfterLayout() - previouslySnappedBoxes " << previouslySnappedBoxes << " m_currentlySnappedBoxes " << m_currentlySnappedBoxes);

auto wasSnappedToMultipleBoxes = previouslySnappedBoxes.size() > 1;
auto currentlySnappedToMultipleBoxes = m_currentlySnappedBoxes.size() > 1;

if (wasSnappedToMultipleBoxes && !currentlySnappedToMultipleBoxes) {
// Pick offset to snap to
auto box = chooseBoxToResnapTo(snapOffsetsHorizontal, snapOffsetsVertical);
auto box = chooseBoxToResnapTo(previouslySnappedBoxes, snapOffsetsHorizontal, snapOffsetsVertical);
snapPointChanged |= preserveCurrentTargetForAxis(ScrollEventAxis::Horizontal, box);
snapPointChanged |= preserveCurrentTargetForAxis(ScrollEventAxis::Vertical, box);
horizontalOffsets = currentlySnappedOffsetsForAxis(ScrollEventAxis::Horizontal);
verticalOffsets = currentlySnappedOffsetsForAxis(ScrollEventAxis::Vertical);
currentlySnapped = currentlySnappedBoxes(horizontalOffsets, verticalOffsets);
currentlySnappedToMultipleBoxes = currentlySnapped.size() > 1;
LOG_WITH_STREAM(ScrollSnap, stream << "ScrollSnapAnimatorState::resnapAfterLayout() current target: " << box << " snapPointChanged: " << snapPointChanged);

updateCurrentlySnappedBoxes();
LOG_WITH_STREAM(ScrollSnap, stream << "ScrollSnapAnimatorState::resnapAfterLayout() - multiple boxes snapped; chose " << box << " (changed " << snapPointChanged << ") m_currentlySnappedBoxes " << m_currentlySnappedBoxes);
}
if (wasSnappedToMultipleBoxes != currentlySnappedToMultipleBoxes)
m_currentlySnappedBoxes = currentlySnapped;

return snapPointChanged;
}

bool ScrollSnapAnimatorState::setNearestScrollSnapIndexForAxisAndOffset(ScrollEventAxis axis, ScrollOffset scrollOffset, const ScrollExtents& scrollExtents, float pageScale)
bool ScrollSnapAnimatorState::setNearestScrollSnapIndexForAxisAndOffsetInternal(ScrollEventAxis axis, ScrollOffset scrollOffset, const ScrollExtents& scrollExtents, float pageScale)
{
auto activeIndex = closestSnapPointForOffset(axis, scrollOffset, scrollExtents, pageScale);
if (activeIndex == activeSnapIndexForAxis(axis))
return false;

setActiveSnapIndexForAxis(axis, activeIndex);
setActiveSnapIndexForAxisInternal(axis, activeIndex);
return true;
}

bool ScrollSnapAnimatorState::setNearestScrollSnapIndexForOffset(ScrollOffset scrollOffset, const ScrollExtents& scrollExtents, float pageScale)
{
bool snapIndexChanged = false;
snapIndexChanged |= setNearestScrollSnapIndexForAxisAndOffset(ScrollEventAxis::Horizontal, scrollOffset, scrollExtents, pageScale);
snapIndexChanged |= setNearestScrollSnapIndexForAxisAndOffset(ScrollEventAxis::Vertical, scrollOffset, scrollExtents, pageScale);
snapIndexChanged |= setNearestScrollSnapIndexForAxisAndOffsetInternal(ScrollEventAxis::Horizontal, scrollOffset, scrollExtents, pageScale);
snapIndexChanged |= setNearestScrollSnapIndexForAxisAndOffsetInternal(ScrollEventAxis::Vertical, scrollOffset, scrollExtents, pageScale);

updateCurrentlySnappedBoxes();

return snapIndexChanged;
}

Expand Down
30 changes: 18 additions & 12 deletions Source/WebCore/platform/ScrollSnapAnimatorState.h
View file
Expand Up @@ -74,20 +74,13 @@ class ScrollSnapAnimatorState {
return axis == ScrollEventAxis::Horizontal ? m_activeSnapIndexX : m_activeSnapIndexY;
}

void setActiveSnapIndexForAxis(ScrollEventAxis axis, std::optional<unsigned> index)
{
if (axis == ScrollEventAxis::Horizontal)
m_activeSnapIndexX = index;
else
m_activeSnapIndexY = index;
}
void setActiveSnapIndexForAxis(ScrollEventAxis, std::optional<unsigned>);

std::optional<unsigned> closestSnapPointForOffset(ScrollEventAxis, ScrollOffset, const ScrollExtents&, float pageScale) const;
float adjustedScrollDestination(ScrollEventAxis, FloatPoint destinationOffset, float velocity, std::optional<float> originalOffset, const ScrollExtents&, float pageScale) const;

// returns true if an active snap index changed.
bool resnapAfterLayout(ScrollOffset, const ScrollExtents&, float pageScale);
bool setNearestScrollSnapIndexForAxisAndOffset(ScrollEventAxis, ScrollOffset, const ScrollExtents&, float pageScale);

bool setNearestScrollSnapIndexForOffset(ScrollOffset, const ScrollExtents&, float pageScale);

Expand All @@ -98,15 +91,28 @@ class ScrollSnapAnimatorState {

void transitionToUserInteractionState();
void transitionToDestinationReachedState();
bool preserveCurrentTargetForAxis(ScrollEventAxis, ElementIdentifier);
Vector<SnapOffset<LayoutUnit>> currentlySnappedOffsetsForAxis(ScrollEventAxis) const;
ElementIdentifier chooseBoxToResnapTo(const Vector<SnapOffset<LayoutUnit>>&, const Vector<SnapOffset<LayoutUnit>>&) const;
HashSet<ElementIdentifier> currentlySnappedBoxes(const Vector<SnapOffset<LayoutUnit>>&, const Vector<SnapOffset<LayoutUnit>>&) const;

private:
std::pair<float, std::optional<unsigned>> targetOffsetForStartOffset(ScrollEventAxis, const ScrollExtents&, float startOffset, FloatPoint predictedOffset, float pageScale, float initialDelta) const;
bool setupAnimationForState(ScrollSnapState, const ScrollExtents&, float pageScale, const FloatPoint& initialOffset, const FloatSize& initialVelocity, const FloatSize& initialDelta);
void teardownAnimationForState(ScrollSnapState);

bool preserveCurrentTargetForAxis(ScrollEventAxis, ElementIdentifier);

Vector<SnapOffset<LayoutUnit>> currentlySnappedOffsetsForAxis(ScrollEventAxis) const;
HashSet<ElementIdentifier> currentlySnappedBoxes(const Vector<SnapOffset<LayoutUnit>>& horizontalOffsets, const Vector<SnapOffset<LayoutUnit>>& verticalOffsets) const;

bool setNearestScrollSnapIndexForAxisAndOffsetInternal(ScrollEventAxis, ScrollOffset, const ScrollExtents&, float pageScale);
void updateCurrentlySnappedBoxes();

void setActiveSnapIndexForAxisInternal(ScrollEventAxis axis, std::optional<unsigned> index)
{
if (axis == ScrollEventAxis::Horizontal)
m_activeSnapIndexX = index;
else
m_activeSnapIndexY = index;
}

ScrollingEffectsController& m_scrollController;

ScrollSnapState m_currentState { ScrollSnapState::UserInteraction };
Expand Down
5 changes: 4 additions & 1 deletion Source/WebCore/platform/ScrollableArea.cpp
View file
Expand Up @@ -542,7 +542,7 @@ void ScrollableArea::setCurrentVerticalSnapPointIndex(std::optional<unsigned> in

void ScrollableArea::resnapAfterLayout()
{
LOG_WITH_STREAM(ScrollSnap, stream << *this << " updateScrollSnapState: isScrollSnapInProgress " << isScrollSnapInProgress() << " isUserScrollInProgress " << isUserScrollInProgress());
LOG_WITH_STREAM(ScrollSnap, stream << *this << " resnapAfterLayout: isScrollSnapInProgress " << isScrollSnapInProgress() << " isUserScrollInProgress " << isUserScrollInProgress());

auto* scrollAnimator = existingScrollAnimator();
if (!scrollAnimator || isScrollSnapInProgress() || isUserScrollInProgress())
Expand Down Expand Up @@ -572,6 +572,7 @@ void ScrollableArea::resnapAfterLayout()
}

if (correctedOffset != currentOffset) {
LOG_WITH_STREAM(ScrollSnap, stream << "ScrollableArea::resnapAfterLayout - adjusting scroll position from " << currentOffset << " to " << correctedOffset << " for snap point at index " << currentVerticalSnapPointIndex());
auto position = scrollPositionFromOffset(correctedOffset);
if (scrollAnimationStatus() == ScrollAnimationStatus::NotAnimating)
scrollToOffsetWithoutAnimation(correctedOffset);
Expand All @@ -596,6 +597,8 @@ void ScrollableArea::doPostThumbMoveSnapping(ScrollbarOrientation orientation)
if (newOffset == currentOffset)
return;

LOG_WITH_STREAM(ScrollSnap, stream << "ScrollableArea::doPostThumbMoveSnapping - adjusting scroll position from " << currentOffset << " to " << newOffset);

auto position = scrollPositionFromOffset(newOffset);
scrollAnimator->scrollToPositionWithAnimation(position);
}
Expand Down

0 comments on commit ee03689

Please sign in to comment.