Skip to content

Commit

Permalink
Partially revert "Fix touch-action update timing for touch pointers"
Browse files Browse the repository at this point in the history
This partially reverts commit bd37a1d
keeping the new WPT intact.

Reason for revert: the exposed behavior have made PointerEvent's
touch-action behavior very similar to canceling TouchEvent's
touchstart, and it seems to have been exposed for years!  We need a
PSA and impact assessment.

Fixed: 1328973
Bug: 1314739

Original change's description:
> Fix touch-action update timing for touch pointers.
>
> Historically the update was done right before touchstart event firing.
> When we shipped pointer events, we missed to move this update to
> pointerdown event firing.  As a result, changing the touch-action
> after pointerdown but before touchstart unexpectedly exposed the
> change.
>
> This CL fixes this historical mistake.
>
> Fixed: 1314739
> Change-Id: I59f4d0bd6d89bf577603644714e975fe56d05239
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3627854
> Commit-Queue: Mustaq Ahmed <mustaq@chromium.org>
> Reviewed-by: Robert Flack <flackr@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#999634}

(cherry picked from commit cac214d)

Change-Id: I4f04a7bbdb0749de6da491d5e84f7cd5e748bd26
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3668933
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Mustaq Ahmed <mustaq@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1007969}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3688820
Cr-Commit-Position: refs/branch-heads/5060@{#616}
Cr-Branched-From: b83393d-refs/heads/main@{#1002911}
  • Loading branch information
mustaqahmed authored and Chromium LUCI CQ committed Jun 6, 2022
1 parent 8dfc224 commit 2331829
Show file tree
Hide file tree
Showing 4 changed files with 16 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -658,11 +658,6 @@ WebInputEventResult PointerEventManager::HandlePointerEvent(
mojom::blink::UserActivationNotificationType::kInteraction);
}

if (event.GetType() == WebInputEvent::Type::kPointerDown) {
touch_event_manager_->UpdateTouchAttributeMapsForPointerDown(
event, pointer_event_target);
}

WebInputEventResult result = DispatchTouchPointerEvent(
event, coalesced_events, predicted_events, pointer_event_target);

Expand Down
6 changes: 6 additions & 0 deletions third_party/blink/renderer/core/input/touch_event_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -624,6 +624,12 @@ void TouchEventManager::HandleTouchPoint(
return;
}

// In touch event model only touch starts can set the target and after that
// the touch event always goes to that target.
if (event.GetType() == WebInputEvent::Type::kPointerDown) {
UpdateTouchAttributeMapsForPointerDown(event, pointer_event_target);
}

// We might not receive the down action for a touch point. In that case we
// would have never added them to |touch_attribute_map_| or hit-tested
// them. For those just keep them in the map with a null target. Later they
Expand Down
19 changes: 8 additions & 11 deletions third_party/blink/renderer/core/input/touch_event_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,6 @@ class CORE_EXPORT TouchEventManager final
// Returns whether there is any touch on the screen.
bool IsAnyTouchActive() const;

// Keeps track of attributes of the touch point in the
// |touch_points_attributes_| map and computes the effective touch-action
// value, after possibly performing a hit-test if the original hit test result
// was not inside capturing frame |touch_sequence_document_| for touch events.
void UpdateTouchAttributeMapsForPointerDown(
const WebPointerEvent&,
const event_handling_util::PointerEventTarget&);

private:
// Class represending one touch point event with its coalesced events and
// related attributes.
Expand Down Expand Up @@ -83,6 +75,14 @@ class CORE_EXPORT TouchEventManager final
Touch* CreateDomTouch(const TouchPointAttributes*, bool* known_target);
void AllTouchesReleasedCleanup();

// Keeps track of attributes of the touch point in the
// |touch_points_attributes_| map and does the hit-testing if the original hit
// test result was not inside capturing frame |touch_sequence_document_| for
// touch events.
void UpdateTouchAttributeMapsForPointerDown(
const WebPointerEvent&,
const event_handling_util::PointerEventTarget&);

// This is triggered either by VSync signal to send one touch event per frame
// accumulating all move events or by discrete events pointerdown/up/cancel.
WebInputEventResult DispatchTouchEventFromAccumulatdTouchPoints();
Expand Down Expand Up @@ -135,9 +135,6 @@ class CORE_EXPORT TouchEventManager final
// action which is sent to the browser after handling each dispatched
// 'touchstart' is the intersection of all the previously calculated effective
// touch action values during the sequence.
//
// TODO(https://crbug.com/844493): This seems incomplete code, should be
// removed.
absl::optional<TouchAction> delayed_effective_touch_action_;
};

Expand Down
2 changes: 2 additions & 0 deletions third_party/blink/web_tests/TestExpectations
Original file line number Diff line number Diff line change
Expand Up @@ -5910,6 +5910,8 @@ crbug.com/1176162 http/tests/devtools/screen-orientation-override.js [ Failure P
crbug.com/1215949 external/wpt/pointerevents/pointerevent_iframe-touch-action-none_touch.html [ Pass Timeout ]
crbug.com/1216139 http/tests/devtools/bfcache/bfcache-elements-update.js [ Failure Pass ]

crbug.com/1314739 external/wpt/pointerevents/pointerevent_touch-action-modified_touch.html [ Timeout ]

# Sheriff 2021-06-10
crbug.com/1177996 [ Mac10.15 ] storage/websql/database-lock-after-reload.html [ Failure Pass ]

Expand Down

0 comments on commit 2331829

Please sign in to comment.