Skip to content

Commit

Permalink
Revert "Correctly handle scroll-snap-type changes to 'none'"
Browse files Browse the repository at this point in the history 
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 <[email protected]>
> Reviewed-by: David Bokan <[email protected]>
> Commit-Queue: Majid Valipour <[email protected]>
> Cr-Commit-Position: refs/heads/master@{#657460}

[email protected],[email protected]

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 <[email protected]>
Commit-Queue: Takashi Sakamoto <[email protected]>
Cr-Commit-Position: refs/heads/master@{#657571}
  • Loading branch information
@tasak @chromium-wpt-export-bot
tasak authored and chromium-wpt-export-bot committed May 8, 2019
1 parent 3a8dae2 commit 59ffeb6
Show file tree
Hide file tree
Showing 2 changed files with 1 addition and 74 deletions.
66 changes: 0 additions & 66 deletions css/css-scroll-snap/scroll-snap-type-change.html
View file

This file was deleted.

9 changes: 1 addition & 8 deletions css/css-scroll-snap/scroll-snap-type.html → ...roll-snap/scroll-snap-type-proximity.html
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<!DOCTYPE html>
<link rel="help" href="https://drafts.csswg.org/css-scroll-snap/#scroll-snap-type" />
<link rel="help" href="https://drafts.csswg.org/css-scroll-snap-1" />
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>

Expand Down Expand Up @@ -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.");
</script>

0 comments on commit 59ffeb6

Please sign in to comment.