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 712c3cf.

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:     chromium#1 0x5620c924e56c in Style ./../../third_party/blink/renderer/core/layout/layout_object.h:1615:0
STDERR:     chromium#2 0x5620c924e56c in GetPhysicalSnapType ./../../third_party/blink/renderer/core/page/scrolling/snap_coordinator.cc:88:0
STDERR:     chromium#3 0x5620c924e56c in blink::SnapCoordinator::UpdateSnapContainerData(blink::LayoutBox&) ./../../third_party/blink/renderer/core/page/scrolling/snap_coordinator.cc:107:0
STDERR:     chromium#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 <majidvp@chromium.org>
> Reviewed-by: David Bokan <bokan@chromium.org>
> Commit-Queue: Majid Valipour <majidvp@chromium.org>
> 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 <tasak@google.com>
Commit-Queue: Takashi Sakamoto <tasak@google.com>
Cr-Commit-Position: refs/heads/master@{#657571}
  • Loading branch information
@tasak
tasak authored and Commit Bot committed May 8, 2019
1 parent 426d8d1 commit 31a914f
Show file tree
Hide file tree
Showing 7 changed files with 68 additions and 167 deletions.
3 changes: 0 additions & 3 deletions cc/input/scroll_snap_data.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,9 +112,6 @@ void SnapContainerData::AddSnapAreaData(SnapAreaData snap_area_data) {
bool SnapContainerData::FindSnapPosition(
const SnapSelectionStrategy& strategy,
gfx::ScrollOffset* snap_position) const {
if (scroll_snap_type_.is_none)
return false;

gfx::ScrollOffset base_position = strategy.base_position();
SnapAxis axis = scroll_snap_type_.axis;
bool should_snap_on_x = strategy.ShouldSnapOnX() &&
Expand Down
10 changes: 3 additions & 7 deletions cc/input/scroll_snap_data.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,15 +154,11 @@ struct SnapAreaData {

typedef std::vector<SnapAreaData> SnapAreaList;

// Snap container is a scroll container that at least one snap area assigned to
// it. If the snap-type is not 'none', then it can be snapped to one of its
// snap areas when a scroll happens.
// Snap container is a scroll container that has non-'none' value for
// scroll-snap-type. It can be snapped to one of its snap areas when a scroll
// happens.
// This data structure describes the data needed for SnapCoordinator to perform
// snapping in the snap container.
//
// Note that the snap area data should only be used when snap-type is not 'none'
// There is not guarantee that this information is up-to-date otherwise. In
// fact, we skip updating these info as an optiomization.
class CC_EXPORT SnapContainerData {
public:
SnapContainerData();
Expand Down
6 changes: 2 additions & 4 deletions third_party/blink/renderer/core/layout/layout_box.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -473,10 +473,8 @@ void LayoutBox::UpdateScrollSnapMappingAfterStyleChange(
cc::ScrollSnapType new_snap_type = new_style && allows_snap_container
? new_style->GetScrollSnapType()
: cc::ScrollSnapType();
if (old_snap_type != new_snap_type) {
snap_coordinator->SnapContainerDidChange(*this,
!new_style /* is_removed */);
}
if (old_snap_type != new_snap_type)
snap_coordinator->SnapContainerDidChange(*this, new_snap_type);

cc::ScrollSnapAlign old_snap_align =
old_style ? old_style->GetScrollSnapAlign() : cc::ScrollSnapAlign();
Expand Down
139 changes: 61 additions & 78 deletions third_party/blink/renderer/core/page/scrolling/snap_coordinator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,70 +103,66 @@ static cc::ScrollSnapType GetPhysicalSnapType(const LayoutBox& snap_container) {
}

void SnapCoordinator::UpdateSnapContainerData(LayoutBox& snap_container) {
if (snap_container.Style()->GetScrollSnapType().is_none)
return;

cc::SnapContainerData snap_container_data(
GetPhysicalSnapType(snap_container));

// When snap type is 'none' we don't perform any snapping so there is no need
// to keep the area data up to date. So just update the type and skip updating
// areas as an optimization.
if (!snap_container_data.scroll_snap_type().is_none) {
ScrollableArea* scrollable_area = ScrollableAreaForSnapping(snap_container);
if (!scrollable_area)
return;
FloatPoint max_position = scrollable_area->ScrollOffsetToPosition(
scrollable_area->MaximumScrollOffset());
snap_container_data.set_max_position(
gfx::ScrollOffset(max_position.X(), max_position.Y()));

// Scroll-padding represents inward offsets from the corresponding edge of
// the scrollport.
// https://drafts.csswg.org/css-scroll-snap-1/#scroll-padding Scrollport is
// the visual viewport of the scroll container (through which the scrollable
// overflow region can be viewed) coincides with its padding box.
// https://drafts.csswg.org/css-overflow-3/#scrollport. So we use the
// LayoutRect of the padding box here. The coordinate is relative to the
// container's border box.
LayoutRect container_rect(snap_container.PhysicalPaddingBoxRect());

const ComputedStyle* container_style = snap_container.Style();
LayoutRectOutsets container_padding(
// The percentage of scroll-padding is different from that of normal
// padding, as scroll-padding resolves the percentage against
// corresponding dimension of the scrollport[1], while the normal
// padding resolves that against "width".[2,3] We use
// MinimumValueForLength here to ensure kAuto is resolved to
// LayoutUnit() which is the correct behavior for padding.
// [1] https://drafts.csswg.org/css-scroll-snap-1/#scroll-padding
// "relative to the corresponding dimension of the scroll
// container’s
// scrollport"
// [2] https://drafts.csswg.org/css-box/#padding-props
// [3] See for example LayoutBoxModelObject::ComputedCSSPadding where it
// uses |MinimumValueForLength| but against the "width".
MinimumValueForLength(container_style->ScrollPaddingTop(),
container_rect.Height()),
MinimumValueForLength(container_style->ScrollPaddingRight(),
container_rect.Width()),
MinimumValueForLength(container_style->ScrollPaddingBottom(),
container_rect.Height()),
MinimumValueForLength(container_style->ScrollPaddingLeft(),
container_rect.Width()));
container_rect.Contract(container_padding);
snap_container_data.set_rect(FloatRect(container_rect));

if (snap_container_data.scroll_snap_type().strictness ==
cc::SnapStrictness::kProximity) {
LayoutSize size = container_rect.Size();
size.Scale(kProximityRatio);
gfx::ScrollOffset range(size.Width().ToFloat(), size.Height().ToFloat());
snap_container_data.set_proximity_range(range);
}
ScrollableArea* scrollable_area = ScrollableAreaForSnapping(snap_container);
if (!scrollable_area)
return;
FloatPoint max_position = scrollable_area->ScrollOffsetToPosition(
scrollable_area->MaximumScrollOffset());
snap_container_data.set_max_position(
gfx::ScrollOffset(max_position.X(), max_position.Y()));

// Scroll-padding represents inward offsets from the corresponding edge of the
// scrollport. https://drafts.csswg.org/css-scroll-snap-1/#scroll-padding
// Scrollport is the visual viewport of the scroll container (through which
// the scrollable overflow region can be viewed) coincides with its padding
// box. https://drafts.csswg.org/css-overflow-3/#scrollport. So we use the
// LayoutRect of the padding box here. The coordinate is relative to the
// container's border box.
LayoutRect container_rect(snap_container.PhysicalPaddingBoxRect());

if (SnapAreaSet* snap_areas = snap_container.SnapAreas()) {
for (const LayoutBox* snap_area : *snap_areas) {
snap_container_data.AddSnapAreaData(
CalculateSnapAreaData(*snap_area, snap_container));
}
const ComputedStyle* container_style = snap_container.Style();
LayoutRectOutsets container_padding(
// The percentage of scroll-padding is different from that of normal
// padding, as scroll-padding resolves the percentage against
// corresponding dimension of the scrollport[1], while the normal padding
// resolves that against "width".[2,3]
// We use MinimumValueForLength here to ensure kAuto is resolved to
// LayoutUnit() which is the correct behavior for padding.
// [1] https://drafts.csswg.org/css-scroll-snap-1/#scroll-padding
// "relative to the corresponding dimension of the scroll container’s
// scrollport"
// [2] https://drafts.csswg.org/css-box/#padding-props
// [3] See for example LayoutBoxModelObject::ComputedCSSPadding where it
// uses |MinimumValueForLength| but against the "width".
MinimumValueForLength(container_style->ScrollPaddingTop(),
container_rect.Height()),
MinimumValueForLength(container_style->ScrollPaddingRight(),
container_rect.Width()),
MinimumValueForLength(container_style->ScrollPaddingBottom(),
container_rect.Height()),
MinimumValueForLength(container_style->ScrollPaddingLeft(),
container_rect.Width()));
container_rect.Contract(container_padding);
snap_container_data.set_rect(FloatRect(container_rect));

if (snap_container_data.scroll_snap_type().strictness ==
cc::SnapStrictness::kProximity) {
LayoutSize size = container_rect.Size();
size.Scale(kProximityRatio);
gfx::ScrollOffset range(size.Width().ToFloat(), size.Height().ToFloat());
snap_container_data.set_proximity_range(range);
}

if (SnapAreaSet* snap_areas = snap_container.SnapAreas()) {
for (const LayoutBox* snap_area : *snap_areas) {
snap_container_data.AddSnapAreaData(
CalculateSnapAreaData(*snap_area, snap_container));
}
}

Expand Down Expand Up @@ -333,29 +329,16 @@ bool SnapCoordinator::PerformSnapping(
return true;
}

void SnapCoordinator::SnapContainerDidChange(LayoutBox& snap_container,
bool is_removed) {
if (is_removed) {
snap_container_map_.erase(&snap_container);
return;
}

bool is_scroll_container =
snap_container.IsLayoutView() || snap_container.HasOverflowClip();
if (!is_scroll_container) {
void SnapCoordinator::SnapContainerDidChange(
LayoutBox& snap_container,
cc::ScrollSnapType scroll_snap_type) {
snap_container.SetNeedsPaintPropertyUpdate();
if (scroll_snap_type.is_none) {
snap_container_map_.erase(&snap_container);
snap_container.ClearSnapAreas();
snap_container.SetNeedsPaintPropertyUpdate();
return;
}

// Note that even if scroll snap type is 'none' we continue to maintain its
// snap container entry as long as the element is a scroller. This is because
// while the scroller does not snap, it still captures the snap areas in its
// subtree for whom it is the nearest ancestor scroll container per spec [1].
//
// [1] https://drafts.csswg.org/css-scroll-snap/#overview

// TODO(sunyunjia): Only update when the localframe doesn't need layout.
UpdateSnapContainerData(snap_container);

Expand Down
2 changes: 1 addition & 1 deletion third_party/blink/renderer/core/page/scrolling/snap_coordinator.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class CORE_EXPORT SnapCoordinator final
~SnapCoordinator();
void Trace(blink::Visitor* visitor) {}

void SnapContainerDidChange(LayoutBox&, bool is_removed);
void SnapContainerDidChange(LayoutBox&, cc::ScrollSnapType);
void SnapAreaDidChange(LayoutBox&, cc::ScrollSnapAlign);

// Returns the SnapContainerData if the snap container has one.
Expand Down
66 changes: 0 additions & 66 deletions third_party/blink/web_tests/external/wpt/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 31a914f

Please sign in to comment.