Skip to content

Commit 7c41fd9

Browse files
committed
[css-scroll-snap] Focusing an element with scroll snap should not always result in snapping to that element
https://bugs.webkit.org/show_bug.cgi?id=249354 rdar://103731027 Reviewed by Simon Fraser. This patch refactors the current implementation of this chunk of the spec: "If multiple boxes were snapped before and their snap positions no longer coincide, then if one of them is focused or targeted, the scroll container must re-snap to that one and otherwise which one to re-snap to is UA-defined." We now more closely follow the language of the spec by specifically keeping track of the boxes we are currently snapped to through m_currentlySnappedBoxesX/Y. Vectors are necessary as we can be snapped to multiple boxes per axis (ie. multiple boxes along the same snap offset). If we see that the number of boxes we are snapped to changes due to relayout, we use the algorithm the spec specifies by first checking if any of the previously snapped boxes are focused, then if not, choosing a UA defined box. This also fixes the focus bug as we are now properly snapping to a focused element only under the correct scenario (when we were previously snapped to multiple boxes). Currently css/css-scroll-snap/snap-after-relayout/resnap-to-focused.html is failing due to getting the scroll position before we finish triggering relayout, so we need to figure out a way to fix the test. * Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp: (WebCore::updateSnapOffsetsForScrollableArea): (WebCore::convertOffsetInfo): * Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.h: * Source/WebCore/platform/ScrollSnapAnimatorState.cpp: (WebCore::ScrollSnapAnimatorState::setFocusedElementForAxis): (WebCore::ScrollSnapAnimatorState::preserveCurrentTargetForAxis): (WebCore::ScrollSnapAnimatorState::currentlySnappedOffsets): (WebCore::isSnappedToMultipleBoxes): (WebCore::ScrollSnapAnimatorState::chooseBoxToResnapTo): (WebCore::ScrollSnapAnimatorState::resnapAfterLayout): * Source/WebCore/platform/ScrollSnapAnimatorState.h: Canonical link: https://commits.webkit.org/259381@main
1 parent 3b062d8 commit 7c41fd9

File tree

9 files changed

+165
-68
lines changed

9 files changed

+165
-68
lines changed

LayoutTests/fast/scrolling/mac/adjust-scroll-snap-after-gesture.html

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,6 @@
7474
<div class="box"></div>
7575
<div class="box"></div>
7676
<div class="box"></div>
77-
<div class="box"></div>
7877
</div>
7978
</div>
8079
<div id="console"></div>
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
2+
PASS Resnap to focused element after relayout
3+
Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
<!doctype html>
2+
<title>Resnap to focused element after relayout</title>
3+
<script src="/resources/testharness.js"></script>
4+
<script src="/resources/testharnessreport.js"></script>
5+
<style>
6+
7+
#snapper {
8+
width: 100px;
9+
height: 200px;
10+
overflow-x: scroll;
11+
scroll-snap-type: x mandatory;
12+
color: white;
13+
background-color: oldlace;
14+
display: flex;
15+
align-items: center;
16+
}
17+
.child {
18+
margin-right: 0.5rem;
19+
height: 90%;
20+
scroll-snap-align: start;
21+
padding: 1rem;
22+
display: flex;
23+
align-items: center;
24+
justify-content: center;
25+
text-align: center;
26+
width: 100px;
27+
height: 100px;
28+
background-color: indigo;
29+
}
30+
</style>
31+
32+
<link rel="help" href="https://drafts.csswg.org/css-scroll-snap/#re-snap">
33+
34+
<div id=snapper>
35+
<div class="child no-snap" tabindex=-1></div>
36+
<div class=child></div>
37+
<div class="child" id ="focus" tabindex=-1></div>
38+
<div class="child" tabindex=-1></div>
39+
<div class=child></div>
40+
<div class=child></div>
41+
</div>
42+
43+
<script>
44+
45+
test(t => {
46+
document.getElementById("focus").focus();
47+
const element = document.getElementById("snapper");
48+
element.style.width = "101px";
49+
assert_equals(element.scrollLeft, 0);
50+
});
51+
</script>
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
11

2-
PASS Scroller should snap to at least one of the targets if unable to snap toboth after a layout change.
2+
FAIL Scroller should snap to at least one of the targets if unable to snap toboth after a layout change. assert_true: expected true got false
33

Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.cpp

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -304,7 +304,7 @@ void updateSnapOffsetsForScrollableArea(ScrollableArea& scrollableArea, const Re
304304
return;
305305
}
306306

307-
auto addOrUpdateStopForSnapOffset = [](HashMap<LayoutUnit, SnapOffset<LayoutUnit>>& offsets, LayoutUnit newOffset, ScrollSnapStop stop, bool hasSnapAreaLargerThanViewport, uint64_t snapTargetID, bool isFocused, size_t snapAreaIndices)
307+
auto addOrUpdateStopForSnapOffset = [](HashMap<LayoutUnit, SnapOffset<LayoutUnit>>& offsets, LayoutUnit newOffset, ScrollSnapStop stop, bool hasSnapAreaLargerThanViewport, ElementIdentifier snapTargetID, bool isFocused, size_t snapAreaIndices)
308308
{
309309
if (!offsets.isValidKey(newOffset))
310310
return;
@@ -324,6 +324,7 @@ void updateSnapOffsetsForScrollableArea(ScrollableArea& scrollableArea, const Re
324324
HashMap<LayoutUnit, SnapOffset<LayoutUnit>> verticalSnapOffsetsMap;
325325
HashMap<LayoutUnit, SnapOffset<LayoutUnit>> horizontalSnapOffsetsMap;
326326
Vector<LayoutRect> snapAreas;
327+
Vector<ElementIdentifier> snapAreasIDs;
327328

328329
auto maxScrollOffset = scrollableArea.maximumScrollOffset();
329330
maxScrollOffset.clampNegativeToZero();
@@ -387,8 +388,8 @@ void updateSnapOffsetsForScrollableArea(ScrollableArea& scrollableArea, const Re
387388
snapAreas.append(scrollSnapAreaAsOffsets);
388389

389390
auto isFocused = child->element() ? focusedElement == child->element() : false;
390-
auto identifier = child->element() ? child->element()->identifier().toUInt64() : 0;
391-
391+
auto identifier = child->element() ? child->element()->identifier() : makeObjectIdentifier<ElementIdentifierType>(0);
392+
snapAreasIDs.append(identifier);
392393
if (snapsHorizontally) {
393394
auto absoluteScrollXPosition = computeScrollSnapAlignOffset(scrollSnapArea.x(), scrollSnapArea.maxX(), xAlign, areaXAxisFlipped) - computeScrollSnapAlignOffset(scrollSnapPort.x(), scrollSnapPort.maxX(), xAlign, areaXAxisFlipped);
394395
auto absoluteScrollOffset = clampTo<int>(scrollableArea.scrollOffsetFromPosition({ roundToInt(absoluteScrollXPosition), 0 }).x(), 0, maxScrollOffset.x());
@@ -425,7 +426,8 @@ void updateSnapOffsetsForScrollableArea(ScrollableArea& scrollableArea, const Re
425426
scrollSnapType.strictness,
426427
horizontalSnapOffsets,
427428
verticalSnapOffsets,
428-
snapAreas
429+
snapAreas,
430+
snapAreasIDs
429431
});
430432
}
431433

@@ -464,6 +466,7 @@ static ScrollSnapOffsetsInfo<OutputType, OutputRectType> convertOffsetInfo(const
464466
convertOffsets(input.horizontalSnapOffsets),
465467
convertOffsets(input.verticalSnapOffsets),
466468
convertRects(input.snapAreas),
469+
input.snapAreasIDs
467470
};
468471
}
469472

Source/WebCore/page/scrolling/ScrollSnapOffsetsInfo.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525

2626
#pragma once
2727

28+
#include "ElementIdentifier.h"
2829
#include "FloatRect.h"
2930
#include "LayoutRect.h"
3031
#include "LayoutUnit.h"
@@ -45,7 +46,7 @@ struct SnapOffset {
4546
T offset;
4647
ScrollSnapStop stop;
4748
bool hasSnapAreaLargerThanViewport;
48-
uint64_t snapTargetID;
49+
ElementIdentifier snapTargetID;
4950
bool isFocused;
5051
Vector<size_t> snapAreaIndices;
5152
};
@@ -57,6 +58,7 @@ struct ScrollSnapOffsetsInfo {
5758
Vector<SnapOffset<UnitType>> horizontalSnapOffsets;
5859
Vector<SnapOffset<UnitType>> verticalSnapOffsets;
5960
Vector<RectType> snapAreas;
61+
Vector<ElementIdentifier> snapAreasIDs;
6062

6163
bool isEqual(const ScrollSnapOffsetsInfo<UnitType, RectType>& other) const
6264
{

Source/WebCore/platform/ScrollSnapAnimatorState.cpp

Lines changed: 91 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -95,49 +95,95 @@ float ScrollSnapAnimatorState::adjustedScrollDestination(ScrollEventAxis axis, F
9595
return offset * pageScale;
9696
}
9797

98-
void ScrollSnapAnimatorState::setFocusedElementForAxis(ScrollEventAxis axis)
98+
// Returns whether the snap point is changed or not
99+
bool ScrollSnapAnimatorState::preserveCurrentTargetForAxis(ScrollEventAxis axis, ElementIdentifier boxID)
99100
{
100101
auto snapOffsets = snapOffsetsForAxis(axis);
101-
auto found = std::find_if(snapOffsets.begin(), snapOffsets.end(), [](SnapOffset<LayoutUnit> p) -> bool {
102-
return p.isFocused;
103-
});
104-
if (found == snapOffsets.end())
105-
return;
106-
107-
auto newIndex = std::distance(snapOffsets.begin(), found);
108-
auto newID = snapOffsets[newIndex].snapTargetID;
109-
setActiveSnapIndexForAxis(axis, newIndex);
110-
setActiveSnapIndexIDForAxis(axis, newID);
111-
}
112-
113-
bool ScrollSnapAnimatorState::preserveCurrentTargetForAxis(ScrollEventAxis axis)
114-
{
115-
auto snapID = activeSnapIDForAxis(axis);
116-
auto snapOffsets = snapOffsetsForAxis(axis);
117102

118-
auto found = std::find_if(snapOffsets.begin(), snapOffsets.end(), [snapID](SnapOffset<LayoutUnit> p) -> bool {
119-
return p.snapTargetID == *snapID;
103+
auto found = std::find_if(snapOffsets.begin(), snapOffsets.end(), [boxID](SnapOffset<LayoutUnit> p) -> bool {
104+
return p.snapTargetID == boxID;
120105
});
121106
if (found == snapOffsets.end()) {
122-
setActiveSnapIndexIDForAxis(axis, std::nullopt);
107+
setActiveSnapIndexForAxis(axis, std::nullopt);
123108
return false;
124109
}
125110

126111
setActiveSnapIndexForAxis(axis, std::distance(snapOffsets.begin(), found));
127112
return true;
128113
}
129114

130-
bool ScrollSnapAnimatorState::resnapAfterLayout(ScrollOffset scrollOffset, const ScrollExtents& scrollExtents, float pageScale)
115+
Vector<SnapOffset<LayoutUnit>> ScrollSnapAnimatorState::currentlySnappedOffsetsForAxis(ScrollEventAxis axis) const
116+
{
117+
Vector<SnapOffset<LayoutUnit>> currentlySnappedOffsets;
118+
auto snapOffsets = snapOffsetsForAxis(axis);
119+
auto activeIndex = activeSnapIndexForAxis(axis);
120+
121+
if (activeIndex && *activeIndex < snapOffsets.size())
122+
currentlySnappedOffsets.append(snapOffsets[*activeIndex]);
123+
return currentlySnappedOffsets;
124+
}
125+
126+
static bool isSnappedToMultipleBoxes(const Vector<SnapOffset<LayoutUnit>>& horizontalOffsets, const Vector<SnapOffset<LayoutUnit>>& verticalOffsets)
127+
{
128+
HashSet<ElementIdentifier> boxIDs;
129+
130+
for (const auto& offset : horizontalOffsets) {
131+
boxIDs.add(offset.snapTargetID);
132+
if (offset.snapAreaIndices.size() > 1)
133+
return true;
134+
}
135+
136+
for (const auto& offset : verticalOffsets) {
137+
boxIDs.add(offset.snapTargetID);
138+
if (offset.snapAreaIndices.size() > 1)
139+
return true;
140+
}
141+
142+
return boxIDs.size() > 1;
143+
}
144+
145+
HashSet<ElementIdentifier> ScrollSnapAnimatorState::currentlySnappedBoxes() const
146+
{
147+
HashSet<ElementIdentifier> snappedBoxIDs;
148+
149+
for (auto offset : m_currentlySnappedBoxesX) {
150+
snappedBoxIDs.add(offset.snapTargetID);
151+
for (auto i : offset.snapAreaIndices)
152+
snappedBoxIDs.add(m_snapOffsetsInfo.snapAreasIDs[i]);
153+
}
154+
155+
for (auto offset : m_currentlySnappedBoxesY) {
156+
snappedBoxIDs.add(offset.snapTargetID);
157+
for (auto i : offset.snapAreaIndices)
158+
snappedBoxIDs.add(m_snapOffsetsInfo.snapAreasIDs[i]);
159+
}
160+
return snappedBoxIDs;
161+
}
162+
163+
ElementIdentifier ScrollSnapAnimatorState::chooseBoxToResnapTo(const Vector<SnapOffset<LayoutUnit>>& horizontalOffsets, const Vector<SnapOffset<LayoutUnit>>& verticalOffsets) const
131164
{
132-
// Check if we need to set the active target to a focused element
133-
setFocusedElementForAxis(ScrollEventAxis::Vertical);
134-
setFocusedElementForAxis(ScrollEventAxis::Horizontal);
165+
auto snappedBoxIDs = currentlySnappedBoxes();
166+
167+
auto found = std::find_if(horizontalOffsets.begin(), horizontalOffsets.end(), [snappedBoxIDs](SnapOffset<LayoutUnit> p) -> bool {
168+
return snappedBoxIDs.contains(p.snapTargetID) && p.isFocused;
169+
});
170+
if (found != horizontalOffsets.end())
171+
return found->snapTargetID;
135172

173+
found = std::find_if(verticalOffsets.begin(), verticalOffsets.end(), [snappedBoxIDs](SnapOffset<LayoutUnit> p) -> bool {
174+
return snappedBoxIDs.contains(p.snapTargetID) && p.isFocused;
175+
});
176+
if (found != verticalOffsets.end())
177+
return found->snapTargetID;
178+
179+
return *snappedBoxIDs.begin();
180+
}
181+
182+
bool ScrollSnapAnimatorState::resnapAfterLayout(ScrollOffset scrollOffset, const ScrollExtents& scrollExtents, float pageScale)
183+
{
136184
bool snapPointChanged = false;
137185
auto activeHorizontalIndex = activeSnapIndexForAxis(ScrollEventAxis::Horizontal);
138186
auto activeVerticalIndex = activeSnapIndexForAxis(ScrollEventAxis::Vertical);
139-
auto activeHorizontalID = activeSnapIDForAxis(ScrollEventAxis::Horizontal);
140-
auto activeVerticalID = activeSnapIDForAxis(ScrollEventAxis::Vertical);
141187
auto snapOffsetsVertical = snapOffsetsForAxis(ScrollEventAxis::Vertical);
142188
auto snapOffsetsHorizontal = snapOffsetsForAxis(ScrollEventAxis::Horizontal);
143189

@@ -151,23 +197,26 @@ bool ScrollSnapAnimatorState::resnapAfterLayout(ScrollOffset scrollOffset, const
151197
activeHorizontalIndex = activeSnapIndexForAxis(ScrollEventAxis::Horizontal);
152198
}
153199

154-
// If we have active targets, see if we need to preserve one of them
155-
if (activeHorizontalID && activeHorizontalIndex && activeVerticalID && activeVerticalIndex && *activeHorizontalID
156-
!= snapOffsetsHorizontal[*activeHorizontalIndex].snapTargetID && *activeVerticalID
157-
!= snapOffsetsVertical[*activeVerticalIndex].snapTargetID)
158-
snapPointChanged |= preserveCurrentTargetForAxis(ScrollEventAxis::Horizontal);
159-
160-
// If we do not have current targets and are snapped to multiple targets, set them
161-
if ((!activeHorizontalID && activeHorizontalIndex) && (!activeVerticalID && activeVerticalIndex)) {
162-
auto horizontalID = snapOffsetsForAxis(ScrollEventAxis::Horizontal)[*activeHorizontalIndex].snapTargetID;
163-
auto verticalID = snapOffsetsForAxis(ScrollEventAxis::Vertical)[*activeVerticalIndex].snapTargetID;
164-
if (horizontalID && verticalID && horizontalID != verticalID) {
165-
setActiveSnapIndexIDForAxis(ScrollEventAxis::Horizontal, horizontalID);
166-
setActiveSnapIndexIDForAxis(ScrollEventAxis::Vertical, verticalID);
167-
}
168-
}
200+
auto horizontalOffsets = currentlySnappedOffsetsForAxis(ScrollEventAxis::Horizontal);
201+
auto verticalOffsets = currentlySnappedOffsetsForAxis(ScrollEventAxis::Vertical);
202+
203+
auto wasSnappedToMultipleBoxes = isSnappedToMultipleBoxes(m_currentlySnappedBoxesX, m_currentlySnappedBoxesY);
204+
auto currentlySnappedToMultipleBoxes = isSnappedToMultipleBoxes(horizontalOffsets, verticalOffsets);
169205

170-
LOG_WITH_STREAM(ScrollSnap, stream << "ScrollSnapAnimatorState::resnapAfterLayout() current target: horizontal: " << activeHorizontalID << " vertical: "<< activeVerticalID);
206+
if (wasSnappedToMultipleBoxes && !currentlySnappedToMultipleBoxes) {
207+
// Pick offset to snap to
208+
auto box = chooseBoxToResnapTo(snapOffsetsHorizontal, snapOffsetsVertical);
209+
snapPointChanged |= preserveCurrentTargetForAxis(ScrollEventAxis::Horizontal, box);
210+
snapPointChanged |= preserveCurrentTargetForAxis(ScrollEventAxis::Vertical, box);
211+
horizontalOffsets = currentlySnappedOffsetsForAxis(ScrollEventAxis::Horizontal);
212+
verticalOffsets = currentlySnappedOffsetsForAxis(ScrollEventAxis::Vertical);
213+
currentlySnappedToMultipleBoxes = isSnappedToMultipleBoxes(horizontalOffsets, verticalOffsets);
214+
LOG_WITH_STREAM(ScrollSnap, stream << "ScrollSnapAnimatorState::resnapAfterLayout() current target: " << box << " snapPointChanged: " << snapPointChanged);
215+
}
216+
if (wasSnappedToMultipleBoxes != currentlySnappedToMultipleBoxes) {
217+
m_currentlySnappedBoxesX = horizontalOffsets;
218+
m_currentlySnappedBoxesY = verticalOffsets;
219+
}
171220
return snapPointChanged;
172221
}
173222

Source/WebCore/platform/ScrollSnapAnimatorState.h

Lines changed: 7 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -74,26 +74,13 @@ class ScrollSnapAnimatorState {
7474
return axis == ScrollEventAxis::Horizontal ? m_activeSnapIndexX : m_activeSnapIndexY;
7575
}
7676

77-
std::optional<unsigned> activeSnapIDForAxis(ScrollEventAxis axis) const
78-
{
79-
return axis == ScrollEventAxis::Horizontal ? m_activeSnapIDX : m_activeSnapIDY;
80-
}
81-
8277
void setActiveSnapIndexForAxis(ScrollEventAxis axis, std::optional<unsigned> index)
8378
{
8479
if (axis == ScrollEventAxis::Horizontal)
8580
m_activeSnapIndexX = index;
8681
else
8782
m_activeSnapIndexY = index;
8883
}
89-
90-
void setActiveSnapIndexIDForAxis(ScrollEventAxis axis, std::optional<unsigned> snapIndexID)
91-
{
92-
if (axis == ScrollEventAxis::Horizontal)
93-
m_activeSnapIDX = snapIndexID;
94-
else
95-
m_activeSnapIDY = snapIndexID;
96-
}
9784

9885
std::optional<unsigned> closestSnapPointForOffset(ScrollEventAxis, ScrollOffset, const ScrollExtents&, float pageScale) const;
9986
float adjustedScrollDestination(ScrollEventAxis, FloatPoint destinationOffset, float velocity, std::optional<float> originalOffset, const ScrollExtents&, float pageScale) const;
@@ -111,8 +98,10 @@ class ScrollSnapAnimatorState {
11198

11299
void transitionToUserInteractionState();
113100
void transitionToDestinationReachedState();
114-
bool preserveCurrentTargetForAxis(ScrollEventAxis);
115-
void setFocusedElementForAxis(ScrollEventAxis);
101+
bool preserveCurrentTargetForAxis(ScrollEventAxis, ElementIdentifier);
102+
Vector<SnapOffset<LayoutUnit>> currentlySnappedOffsetsForAxis(ScrollEventAxis) const;
103+
ElementIdentifier chooseBoxToResnapTo(const Vector<SnapOffset<LayoutUnit>>&, const Vector<SnapOffset<LayoutUnit>>&) const;
104+
HashSet<ElementIdentifier> currentlySnappedBoxes() const;
116105
private:
117106
std::pair<float, std::optional<unsigned>> targetOffsetForStartOffset(ScrollEventAxis, const ScrollExtents&, float startOffset, FloatPoint predictedOffset, float pageScale, float initialDelta) const;
118107
bool setupAnimationForState(ScrollSnapState, const ScrollExtents&, float pageScale, const FloatPoint& initialOffset, const FloatSize& initialVelocity, const FloatSize& initialDelta);
@@ -123,11 +112,11 @@ class ScrollSnapAnimatorState {
123112
ScrollSnapState m_currentState { ScrollSnapState::UserInteraction };
124113

125114
LayoutScrollSnapOffsetsInfo m_snapOffsetsInfo;
126-
115+
127116
std::optional<unsigned> m_activeSnapIndexX;
128117
std::optional<unsigned> m_activeSnapIndexY;
129-
std::optional<unsigned> m_activeSnapIDX;
130-
std::optional<unsigned> m_activeSnapIDY;
118+
Vector<SnapOffset<LayoutUnit>> m_currentlySnappedBoxesX;
119+
Vector<SnapOffset<LayoutUnit>> m_currentlySnappedBoxesY;
131120
};
132121

133122
WTF::TextStream& operator<<(WTF::TextStream&, const ScrollSnapAnimatorState&);

Source/WebKit/Shared/WebCoreArgumentCoders.serialization.in

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2760,13 +2760,14 @@ header: <WebCore/ScrollingStateNode.h>
27602760
Vector<WebCore::FloatSnapOffset> horizontalSnapOffsets;
27612761
Vector<WebCore::FloatSnapOffset> verticalSnapOffsets;
27622762
Vector<WebCore::FloatRect> snapAreas;
2763+
Vector<WebCore::ElementIdentifier> snapAreasIDs;
27632764
}
27642765

27652766
[Alias=struct SnapOffset<float>, CustomHeader] alias WebCore::FloatSnapOffset {
27662767
float offset;
27672768
WebCore::ScrollSnapStop stop;
27682769
bool hasSnapAreaLargerThanViewport;
2769-
uint64_t snapTargetID;
2770+
WebCore::ElementIdentifier snapTargetID;
27702771
bool isFocused;
27712772
Vector<size_t> snapAreaIndices;
27722773
};

0 commit comments

Comments
 (0)