Correctly invalidate snap container data when relevant css props change
Previously we would only recompute snap data when only the first time
the snap area or container was processed and again when a handful of css
properties [1] changed.
This means style changes that contained other relevant properties [2]
did not take affect. This problem was mostly masked because our existing
tests never updates these properties beyond the first instance.
Changes in this patch:
- Update invalidation logic to take all relevant props into account.
- Simplify invalidation logic by separating add/remove case from style
changed case.
- Minor improvement in invalidation documentation in viewport case.
[1] snap-scroll-type, snap-scroll-align [2] scroll-padding,
scroll-margin, scroll-snap-stop
TODO: This CL adds a new test specifically for scroll-snap-stop
property changing. We need additional test to check invalidation logic
for other properties.
Test: external/wpt/css/css-scroll-snap/scroll-snap-stop-change.html => ensuring changing scroll-snap-stop takes effect
Bug: 957995
Change-Id: I494bb259bb213dec44b19449d9b79b1446a67be2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1597536
Commit-Queue: Majid Valipour <majidvp@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#658583}
diff --git a/css/css-scroll-snap/scroll-snap-stop-change.html b/css/css-scroll-snap/scroll-snap-stop-change.html
new file mode 100644
index 0000000..0a73ff1
--- /dev/null
+++ b/css/css-scroll-snap/scroll-snap-stop-change.html
@@ -0,0 +1,77 @@
+<!DOCTYPE html>
+<link rel="help" href="https://drafts.csswg.org/css-scroll-snap/#scroll-snap-stop" />
+
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<style>
+div, html, body {
+ margin: 0;
+ padding: 0;
+}
+html {
+ scroll-snap-type: x mandatory;
+ overflow: scroll;
+}
+#scroller {
+ scroll-snap-type: x mandatory;
+ overflow: scroll;
+ height: 300px;
+ width: 300px;
+}
+.large_space {
+ width: 2000px;
+ height: 2000px;
+}
+.snap_area {
+ scroll-snap-align: none start;
+ width: 100px;
+ height: 100px;
+
+ background-color: blue;
+}
+.snap_area:nth-child(1) {
+ margin-left: 100px;
+}
+.snap_area:nth-child(2) {
+ margin-left: 300px;
+}
+.snap_area:nth-child(3) {
+ margin-left: 500px;
+}
+</style>
+
+<!-- Add snap area to the root scroller -->
+<div class="snap_area"></div>
+<div class="snap_area"></div>
+<div class="snap_area"></div>
+
+<div id="scroller">
+ <div class="snap_area"></div>
+ <div class="snap_area"></div>
+ <div class="snap_area"></div>
+ <div class="large_space"></div>
+</div>
+
+<div class="large_space"></div>
+
+<script>
+const scrollers = [document.scrollingElement, document.getElementById("scroller")];
+for (const scroller of scrollers) {
+ test(_ => {
+ assert_equals(scroller.scrollLeft, 0); // sanity check
+
+ scroller.querySelectorAll('.snap_area').forEach(area => area.style.scrollSnapStop = 'always');
+ scroller.scrollBy(500, 0);
+ assert_equals(scroller.scrollLeft, 100, "scrollBy should not skip area with stop always");
+
+ scroller.querySelectorAll('.snap_area').forEach(area => area.style.scrollSnapStop = 'normal');
+ scroller.scrollBy(500, 0);
+ assert_equals(scroller.scrollLeft, 500, "scrollBy should skip secon area with stop normal");
+
+ // When snap type is changed back to mandatory, scrolling should snap.
+ scroller.querySelectorAll('.snap_area').forEach(area => area.style.scrollSnapStop = 'always');
+ scroller.scrollBy(-500, 0);
+ assert_equals(scroller.scrollLeft, 300, "scrollBy should not skip area with stop always");
+ }, `scroll-snap-stop for areas on ${scroller.nodeName} should control snapping behavior and changing it takes effect`);
+}
+</script>
diff --git a/css/css-scroll-snap/scroll-snap-stop-always.html b/css/css-scroll-snap/scroll-snap-stop.html
similarity index 100%
rename from css/css-scroll-snap/scroll-snap-stop-always.html
rename to css/css-scroll-snap/scroll-snap-stop.html
