Correctly handle scroll-snap-type changes to 'none' #16714
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wpt-pr-bot
approved these changes
May 7, 2019
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-1589899
branch
2 times, most recently
from
eabde6e
to
8a73ece
Compare
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 <majidvp@chromium.org> Reviewed-by: David Bokan <bokan@chromium.org> Commit-Queue: Majid Valipour <majidvp@chromium.org> Cr-Commit-Position: refs/heads/master@{#657460}
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-1589899
branch
from
8a73ece
to
e85a511
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
Change-Id: Ie493ad68ecba818ed41c0ee103ccf44725ff6e3f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1589899
Reviewed-by: Majid Valipour <majidvp@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Commit-Queue: Majid Valipour <majidvp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#657460}