Skip to content

Commit

Permalink
Revert "Change autoscroll latching to top-most delta-consumable scrol…
Browse files Browse the repository at this point in the history 
…ler"

This reverts commit 07b882d.

Reason for revert: Suspected culprit for use after free on WebKit Linux ASAN builder

first failing build: https://ci.chromium.org/p/chromium/builders/ci/WebKit%20Linux%20ASAN/20457

Original change's description:
> Change autoscroll latching to top-most delta-consumable scroller
>
> Users will now be able to use middle click autoscroll to scroll a
> parent div if the inner-most scroller is unable to scroll in that
> direction.
>
> If there is no delta-consumable scroller, the top-most autoscrollable
> scroller will be latched.
>
> Bug: 1107648
> Change-Id: Iccd4efec3b1ce5d09c701d3d46052176275dbc32
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2488042
> Reviewed-by: Robert Flack <flackr@chromium.org>
> Reviewed-by: Rahul Arakeri <arakeri@microsoft.com>
> Commit-Queue: Sahir Vellani <sahir.vellani@microsoft.com>
> Cr-Commit-Position: refs/heads/master@{#835318}

TBR=flackr@chromium.org,gerchiko@microsoft.com,arakeri@microsoft.com,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com,sahir.vellani@microsoft.com

Change-Id: Ifea4e760767f30e24e573ea36287108409cfd6e0
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1107648
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2581712
Reviewed-by: Meredith Lane <meredithl@chromium.org>
Commit-Queue: Meredith Lane <meredithl@chromium.org>
Cr-Commit-Position: refs/heads/master@{#835371}
  • Loading branch information
Meredith Lane authored and Chromium LUCI CQ committed Dec 9, 2020
1 parent 3f70552 commit 1f86b5e
Show file tree
Hide file tree
Showing 10 changed files with 74 additions and 446 deletions.
17 changes: 4 additions & 13 deletions cc/input/threaded_input_handler.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1842,7 +1842,6 @@ ScrollNode* ThreadedInputHandler::FindNodeToLatch(ScrollState* scroll_state,
ui::ScrollInputType type) {
ScrollTree& scroll_tree = GetScrollTree();
ScrollNode* scroll_node = nullptr;
ScrollNode* first_scrollable_node = nullptr;
for (ScrollNode* cur_node = starting_node; cur_node;
cur_node = scroll_tree.parent(cur_node)) {
if (GetViewport().ShouldScroll(*cur_node)) {
Expand All @@ -1856,11 +1855,10 @@ ScrollNode* ThreadedInputHandler::FindNodeToLatch(ScrollState* scroll_state,
if (!cur_node->scrollable)
continue;

if (!first_scrollable_node) {
first_scrollable_node = cur_node;
}

if (CanConsumeDelta(*scroll_state, *cur_node)) {
// For UX reasons, autoscrolling should always latch to the top-most
// scroller, even if it can't scroll in the initial direction.
if (type == ui::ScrollInputType::kAutoscroll ||
CanConsumeDelta(*scroll_state, *cur_node)) {
scroll_node = cur_node;
break;
}
Expand All @@ -1882,13 +1880,6 @@ ScrollNode* ThreadedInputHandler::FindNodeToLatch(ScrollState* scroll_state,
}
}

// If the root scroller can not consume delta in an autoscroll, latch on
// to the top most autoscrollable scroller. See https://crbug.com/969150
if ((type == ui::ScrollInputType::kAutoscroll) && first_scrollable_node &&
!CanConsumeDelta(*scroll_state, *scroll_node)) {
scroll_node = first_scrollable_node;
}

return scroll_node;
}

Expand Down
2 changes: 0 additions & 2 deletions third_party/blink/renderer/core/input/event_handler.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -299,10 +299,8 @@ void EventHandler::StartMiddleClickAutoscroll(LayoutObject* layout_object) {
AutoscrollController* controller = scroll_manager_->GetAutoscrollController();
if (!controller)
return;

LayoutBox* scrollable = LayoutBox::FindAutoscrollable(
layout_object, /*is_middle_click_autoscroll*/ true);

controller->StartMiddleClickAutoscroll(
layout_object->GetFrame(), scrollable,
LastKnownMousePositionInRootFrame(),
Expand Down
159 changes: 51 additions & 108 deletions third_party/blink/renderer/core/input/scroll_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,127 +133,63 @@ AutoscrollController* ScrollManager::GetAutoscrollController() const {
return nullptr;
}

ScrollPropagationDirection ScrollManager::ComputePropagationDirection(
const ScrollState& scroll_state) {
if (scroll_state.deltaXHint() == 0 && scroll_state.deltaYHint() != 0)
return ScrollPropagationDirection::kVertical;
if (scroll_state.deltaXHint() != 0 && scroll_state.deltaYHint() == 0)
return ScrollPropagationDirection::kHorizontal;
if (scroll_state.deltaXHint() != 0 && scroll_state.deltaYHint() != 0)
return ScrollPropagationDirection::kBoth;
return ScrollPropagationDirection::kNone;
}

bool ScrollManager::CanPropagate(const LayoutBox* layout_box,
ScrollPropagationDirection direction) {
ScrollableArea* scrollable_area = layout_box->GetScrollableArea();
static bool CanPropagate(const ScrollState& scroll_state, const Node& node) {
ScrollableArea* scrollable_area = node.GetLayoutBox()->GetScrollableArea();
if (!scrollable_area)
return true;

if (!scrollable_area->UserInputScrollable(kHorizontalScrollbar) &&
!scrollable_area->UserInputScrollable(kVerticalScrollbar))
return true;

switch (direction) {
case ScrollPropagationDirection::kBoth:
return ((layout_box->StyleRef().OverscrollBehaviorX() ==
EOverscrollBehavior::kAuto) &&
(layout_box->StyleRef().OverscrollBehaviorY() ==
EOverscrollBehavior::kAuto));
case ScrollPropagationDirection::kVertical:
return layout_box->StyleRef().OverscrollBehaviorY() ==
EOverscrollBehavior::kAuto;
case ScrollPropagationDirection::kHorizontal:
return layout_box->StyleRef().OverscrollBehaviorX() ==
EOverscrollBehavior::kAuto;
case ScrollPropagationDirection::kNone:
return true;
default:
NOTREACHED();
}
return (scroll_state.deltaXHint() == 0 ||
node.GetComputedStyle()->OverscrollBehaviorX() ==
EOverscrollBehavior::kAuto) &&
(scroll_state.deltaYHint() == 0 ||
node.GetComputedStyle()->OverscrollBehaviorY() ==
EOverscrollBehavior::kAuto);
}

void ScrollManager::RecomputeScrollChain(const Node& start_node,
const ScrollState& scroll_state,
Deque<DOMNodeId>& scroll_chain,
bool is_autoscroll) {
Deque<DOMNodeId>& scroll_chain) {
DCHECK(scroll_chain.IsEmpty());
scroll_chain.clear();

DCHECK(start_node.GetLayoutObject());
LayoutBox* cur_box = start_node.GetLayoutObject()->EnclosingBox();

if (is_autoscroll) {
// Propagate the autoscroll along the layout object chain, and
// append only the first node which is able to consume the scroll delta.
// The scroll node is computed differently to regular scrolls in order to
// maintain consistency with the autoscroll controller.
LayoutBox* autoscrollable = LayoutBox::FindAutoscrollable(
start_node.GetLayoutObject(), is_autoscroll);
if (autoscrollable) {
Node* cur_node = autoscrollable->GetNode();
LayoutObject* layout_object = cur_node->GetLayoutObject();
while (layout_object &&
!CanScroll(scroll_state, *cur_node, is_autoscroll)) {
ScrollPropagationDirection direction =
ComputePropagationDirection(scroll_state);

if (!CanPropagate(cur_node->GetLayoutBox(), direction))
break;

if (!layout_object->Parent() &&
layout_object->GetNode() == layout_object->GetDocument() &&
layout_object->GetDocument().LocalOwner()) {
layout_object =
layout_object->GetDocument().LocalOwner()->GetLayoutObject();
} else {
layout_object = layout_object->Parent();
}
LayoutBox* new_autoscrollable =
LayoutBox::FindAutoscrollable(layout_object, is_autoscroll);
if (new_autoscrollable)
cur_node = new_autoscrollable->GetNode();
}
scroll_chain.push_front(DOMNodeIds::IdForNode(cur_node));
}
} else {
LayoutBox* cur_box = start_node.GetLayoutObject()->EnclosingBox();
// Scrolling propagates along the containing block chain and ends at the
// RootScroller node. The RootScroller node will have a custom applyScroll
// callback that performs scrolling as well as associated "root" actions like
// browser control movement and overscroll glow.
while (cur_box) {
Node* cur_node = cur_box->GetNode();

// Scrolling propagates along the containing block chain and ends at the
// RootScroller node. The RootScroller node will have a custom applyScroll
// callback that performs scrolling as well as associated "root" actions
// like browser control movement and overscroll glow.
while (cur_box) {
Node* cur_node = cur_box->GetNode();
if (cur_node) {
if (CanScroll(scroll_state, *cur_node))
scroll_chain.push_front(DOMNodeIds::IdForNode(cur_node));

if (cur_node) {
if (CanScroll(scroll_state, *cur_node, /* for_autoscroll */ false))
scroll_chain.push_front(DOMNodeIds::IdForNode(cur_node));
if (cur_node->IsEffectiveRootScroller())
break;

if (cur_node->IsEffectiveRootScroller())
break;

ScrollPropagationDirection direction =
ComputePropagationDirection(scroll_state);
if (!CanPropagate(cur_node->GetLayoutBox(), direction)) {
// We should add the first node with non-auto overscroll-behavior to
// the scroll chain regardlessly, as it's the only node we can latch
// to.
if (scroll_chain.empty() ||
scroll_chain.front() != DOMNodeIds::IdForNode(cur_node)) {
scroll_chain.push_front(DOMNodeIds::IdForNode(cur_node));
}
break;
if (!CanPropagate(scroll_state, *cur_node)) {
// We should add the first node with non-auto overscroll-behavior to
// the scroll chain regardlessly, as it's the only node we can latch to.
if (scroll_chain.empty() ||
scroll_chain.front() != DOMNodeIds::IdForNode(cur_node)) {
scroll_chain.push_front(DOMNodeIds::IdForNode(cur_node));
}
break;
}

cur_box = cur_box->ContainingBlock();
}

cur_box = cur_box->ContainingBlock();
}
}

bool ScrollManager::CanScroll(const ScrollState& scroll_state,
const Node& current_node,
bool for_autoscroll) {
const Node& current_node) {
LayoutBox* scrolling_box = current_node.GetLayoutBox();
if (auto* element = DynamicTo<Element>(current_node))
scrolling_box = element->GetLayoutBoxForScrolling();
Expand All @@ -262,20 +198,18 @@ bool ScrollManager::CanScroll(const ScrollState& scroll_state,

// We need to always add the global root scroller even if it isn't scrollable
// since we can always pinch-zoom and scroll as well as for overscroll
// effects. If autoscrolling, ignore this condition because we latch on
// to the deepest autoscrollable node.
if (scrolling_box->IsGlobalRootScroller() && !for_autoscroll)
// effects.
if (scrolling_box->IsGlobalRootScroller())
return true;

// If this is the main LayoutView, and it's not the root scroller, that means
// we have a non-default root scroller on the page. In this case, attempts to
// scroll the LayoutView should cause panning of the visual viewport as well
// so ensure it gets added to the scroll chain. See LTHI::ApplyScroll for the
// equivalent behavior in CC. Node::NativeApplyScroll contains a special
// handler for this case. If autoscrolling, ignore this condition because we
// latch on to the deepest autoscrollable node.
// handler for this case.
if (IsA<LayoutView>(scrolling_box) &&
current_node.GetDocument().GetFrame()->IsMainFrame() && !for_autoscroll) {
current_node.GetDocument().GetFrame()->IsMainFrame()) {
return true;
}

Expand Down Expand Up @@ -339,8 +273,7 @@ bool ScrollManager::LogicalScroll(mojom::blink::ScrollDirection direction,
std::make_unique<ScrollStateData>();
auto* scroll_state =
MakeGarbageCollected<ScrollState>(std::move(scroll_state_data));
RecomputeScrollChain(*node, *scroll_state, scroll_chain,
/* is_autoscroll */ false);
RecomputeScrollChain(*node, *scroll_state, scroll_chain);

while (!scroll_chain.IsEmpty()) {
Node* scroll_chain_node = DOMNodeIds::NodeForId(scroll_chain.TakeLast());
Expand Down Expand Up @@ -564,11 +497,21 @@ WebInputEventResult ScrollManager::HandleGestureScrollBegin(
delta_consumed_for_scroll_sequence_;
auto* scroll_state =
MakeGarbageCollected<ScrollState>(std::move(scroll_state_data));

RecomputeScrollChain(
*scroll_gesture_handling_node_.Get(), *scroll_state,
current_scroll_chain_,
gesture_event.SourceDevice() == WebGestureDevice::kSyntheticAutoscroll);
// For middle click autoscroll, only scrollable area for
// |scroll_gesture_handling_node_| should receive and handle all scroll
// events. It should not bubble up to the ancestor.
if (gesture_event.SourceDevice() == WebGestureDevice::kSyntheticAutoscroll) {
LayoutBox* scrollable = LayoutBox::FindAutoscrollable(
scroll_gesture_handling_node_->GetLayoutObject(),
/*is_middle_click_autoscroll*/ true);
if (scrollable) {
Node* scrollable_node = scrollable->GetNode();
current_scroll_chain_.push_back(DOMNodeIds::IdForNode(scrollable_node));
}
} else {
RecomputeScrollChain(*scroll_gesture_handling_node_.Get(), *scroll_state,
current_scroll_chain_);
}

TRACE_EVENT_INSTANT1("input", "Computed Scroll Chain",
TRACE_EVENT_SCOPE_THREAD, "length",
Expand Down
18 changes: 2 additions & 16 deletions third_party/blink/renderer/core/input/scroll_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,6 @@ class Scrollbar;
class ScrollState;
class WebGestureEvent;

// Scroll directions used to check whether propagation is possible in a given
// direction. Used in CanPropagate.
enum class ScrollPropagationDirection { kHorizontal, kVertical, kBoth, kNone };

// This class takes care of scrolling and resizing and the related states. The
// user action that causes scrolling or resizing is determined in other *Manager
// classes and they call into this class for doing the work.
Expand Down Expand Up @@ -112,13 +108,6 @@ class CORE_EXPORT ScrollManager : public GarbageCollected<ScrollManager>,

void AnimateSnapFling(base::TimeTicks monotonic_time);

// Determines whether the scroll-chain should be propagated upwards given a
// scroll direction.
static bool CanPropagate(const LayoutBox* layout_box,
ScrollPropagationDirection direction);
static ScrollPropagationDirection ComputePropagationDirection(
const ScrollState&);

private:
Node* NodeTargetForScrollableAreaElementId(
CompositorElementId scrollable_area_element_id) const;
Expand All @@ -144,11 +133,8 @@ class CORE_EXPORT ScrollManager : public GarbageCollected<ScrollManager>,

void RecomputeScrollChain(const Node& start_node,
const ScrollState&,
Deque<DOMNodeId>& scroll_chain,
bool is_autoscroll);
bool CanScroll(const ScrollState&,
const Node& current_node,
bool for_autoscroll);
Deque<DOMNodeId>& scroll_chain);
bool CanScroll(const ScrollState&, const Node& current_node);

// scroller_size is set only when scrolling non root scroller.
void ComputeScrollRelatedMetrics(
Expand Down
Loading

0 comments on commit 1f86b5e

Please sign in to comment.