From 59ffeb608fd8491995002d5c1dce17d4e49c5291 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Tue, 7 May 2019 19:27:39 -0700 Subject: [PATCH] Revert "Correctly handle scroll-snap-type changes to 'none'" This reverts commit 712c3cf3ed8201420acf23f760eaa34be20781cd. Reason for revert: This patch causes webkit-layout-tests failure on WebKit_Linux_Trusty_ASAN bot: https://ci.chromium.org/p/chromium/builders/ci/WebKit%20Linux%20Trusty%20ASAN/25720 Unexpected Failures: * external/wpt/css/css-scroll-snap/scroll-snap-type.html * virtual/threaded/external/wpt/css/css-scroll-snap/scroll-snap-type.html STDERR: ==1==ERROR: AddressSanitizer: heap-use-after-free on address 0x61200023f8d8 at pc 0x5620c924e56d bp 0x7ffde3c56830 sp 0x7ffde3c56828 STDERR: READ of size 8 at 0x61200023f8d8 thread T0 (content_shell) STDERR: #0 0x5620c924e56c in get ./../../base/memory/scoped_refptr.h:212:27 STDERR: #1 0x5620c924e56c in Style ./../../third_party/blink/renderer/core/layout/layout_object.h:1615:0 STDERR: #2 0x5620c924e56c in GetPhysicalSnapType ./../../third_party/blink/renderer/core/page/scrolling/snap_coordinator.cc:88:0 STDERR: #3 0x5620c924e56c in blink::SnapCoordinator::UpdateSnapContainerData(blink::LayoutBox&) ./../../third_party/blink/renderer/core/page/scrolling/snap_coordinator.cc:107:0 STDERR: #4 0x5620c924e74b in blink::SnapCoordinator::UpdateAllSnapContainerData() ./../../third_party/blink/renderer/core/page/scrolling/snap_coordinator.cc:76:5 Original change's description: > Correctly handle scroll-snap-type changes to 'none' > > > Previously when a scroll container's snap type is changed to 'none' its > data was discarded including all of its snap areas. However this is > incorrect. Because while the snap type is 'none', the element is still > a scroll container which per spec [1] means that is should continue to > captures the snap areas in its subtree for whom it is the nearest > ancestor scroll container . The only difference is that it no longer > snaps. > > The fix is that we no longer remove the snap container data just > because is has a 'none' snap type and instead keep it and its snap > areas. But we check the snap type before performing any snap. > > To ensure this does not introduce any performance regression, this CL > also includes an optimization where we avoid re-calculating > snap_container_data when the snap type is 'none'. So keeping these snap > data should not be cheap. > > Note that there is another problem where if the current snap container > is no longer a scroll container (e.g., overflow: scroll => overflow: > visible) we release its snap areas and they become "orphan". But if we > are to do this correctly, we should re-assign these areas to the next > stroller in the chain. Similarly when an element becomes a scroll > container, it can potentially take over snap areas from its parent snap > container. > > > This patch does not address that situation yet but fixes the easier > problem. > > [1] https://drafts.csswg.org/css-scroll-snap/#overview > > Bug: 953575 > Test: > - wpt/css/css-scroll-snap/scroll-snap-type-change.html => Changing snap-type should work correctly > - wpt/css/css-scroll-snap/scroll-snap-type.html => Add a specific test for type 'none' to ensure it does not snap > > Change-Id: Ie493ad68ecba818ed41c0ee103ccf44725ff6e3f > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1589899 > Reviewed-by: Majid Valipour > Reviewed-by: David Bokan > Commit-Queue: Majid Valipour > Cr-Commit-Position: refs/heads/master@{#657460} TBR=bokan@chromium.org,majidvp@chromium.org Change-Id: I3a327f6e342e95d045194d24ceaf49de52b2b921 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 953575 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1600437 Reviewed-by: Takashi Sakamoto Commit-Queue: Takashi Sakamoto Cr-Commit-Position: refs/heads/master@{#657571} --- .../scroll-snap-type-change.html | 66 ------------------- ...e.html => scroll-snap-type-proximity.html} | 9 +-- 2 files changed, 1 insertion(+), 74 deletions(-) delete mode 100644 css/css-scroll-snap/scroll-snap-type-change.html rename css/css-scroll-snap/{scroll-snap-type.html => scroll-snap-type-proximity.html} (84%) diff --git a/css/css-scroll-snap/scroll-snap-type-change.html b/css/css-scroll-snap/scroll-snap-type-change.html deleted file mode 100644 index 89b4edaf135be4..00000000000000 --- a/css/css-scroll-snap/scroll-snap-type-change.html +++ /dev/null @@ -1,66 +0,0 @@ - - - - - - - - -
- -
-
-
-
- -
- - diff --git a/css/css-scroll-snap/scroll-snap-type.html b/css/css-scroll-snap/scroll-snap-type-proximity.html similarity index 84% rename from css/css-scroll-snap/scroll-snap-type.html rename to css/css-scroll-snap/scroll-snap-type-proximity.html index 1577aa7afc6657..cfe990c4fcab85 100644 --- a/css/css-scroll-snap/scroll-snap-type.html +++ b/css/css-scroll-snap/scroll-snap-type-proximity.html @@ -1,5 +1,5 @@ - + @@ -77,11 +77,4 @@ assert_equals(scroller.scrollLeft, 1000); assert_equals(scroller.scrollTop, 1000); }, "proximity scroll-snap-type should snap if the snap position is close."); - -test(_ => { - scroller.style.scrollSnapType = "none"; - scroller.scrollTo(100, 100); - assert_equals(scroller.scrollLeft, 100, "scrolling should not snap"); - assert_equals(scroller.scrollTop, 100, "scrolling should not snap"); -}, "none scroll-snap-type shouldn't snap.");