C++ APZ should only allow handoff to ancestor APZC

RESOLVED FIXED in Firefox 48

Status

()

Core
Panning and Zooming
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: rbarker, Assigned: rbarker)

Tracking

({polish})

Trunk
mozilla48
polish
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
When apz.allow_immediate_handoff=true, handoff should only be allowed to ancestor APZCs. Currently handoff can go to ancestors and descendants. This means currently when scrolling in an iframe and the end is reached, the scroll is handed off to the ancestor, however if in same gesture the pan direction is changed, the handoff will go back to the scrollable descendant instead of staying with the ancestor currently being scrolled.
(Assignee)

Updated

2 years ago
Assignee: nobody → rbarker
(Assignee)

Comment 1

2 years ago
Created attachment 8736068 [details] [diff] [review]
0001-Bug-1260588-C-APZ-should-only-allow-handoff-to-ancestor-APZC-r-16032916-64d9e69.patch
(Assignee)

Updated

2 years ago
Attachment #8736068 - Flags: review?(botond)
(Assignee)

Updated

2 years ago
Blocks: 1257269
(Assignee)

Updated

2 years ago
Depends on: 1257264
Comment on attachment 8736068 [details] [diff] [review]
0001-Bug-1260588-C-APZ-should-only-allow-handoff-to-ancestor-APZC-r-16032916-64d9e69.patch

Review of attachment 8736068 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! r+ with comments addressed.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +2364,5 @@
> +    if (!block->GetScrolledApzc() || (this == block->GetScrolledApzc())) {
> +      scrollThisApzc = true;
> +    } else if (gfxPrefs::APZAllowImmediateHandoff()) {
> +      scrollThisApzc = block->IsAncestorOfScrolledApzc(this);
> +    }

I think this can be made simpler:

if (InputBlockState* block = CurrentInputBlock()) {
  scrollThisApzc = !block->GetScrolledApzc() ||
                   block->IsAncestorOfScrolledApzc(this);
}

Note that the purpose of this code was never to restrict handoff from an APZC earlier in the chain to an APZC later in the chain in no-immediate-handoff mode (it was only doing that incidentally). The intention is to handle that elsewhere (AllowScrollHandoffInCurrentBlock() for pans, DispatchFling() for flings).

::: gfx/layers/apz/src/InputBlockState.cpp
@@ +123,5 @@
>    return mScrolledApzc;
>  }
>  
> +bool
> +InputBlockState::IsAncestorOfScrolledApzc(AsyncPanZoomController* aApzc) const

We're conflating two senses of the word "ancestor" here.

The sense in which InputBlockState::IsAncestorOf() uses it is "ancestor in the handoff chain", as in "earlier in the handoff chain".

The sense in which this name seems to use it is "ancestor in the APZC tree", which the assumption that ancestors in the APZC tree are *later* in the handoff chain.

To avoid this confusion, I suggest we call this IsDownchainFromScrolledApzc(). "Downchain" is a word I invented, but its meaning should be clear enough ("further down the chain"); compare "downstream".

@@ +126,5 @@
> +bool
> +InputBlockState::IsAncestorOfScrolledApzc(AsyncPanZoomController* aApzc) const
> +{
> +  if (!aApzc || !mScrolledApzc) {
> +    return false;

Please assert this instead. We want the caller to determine the semantics if one of these is null (it should never be the case for the current call site).
Attachment #8736068 - Flags: review?(botond) → review+
(Assignee)

Comment 3

2 years ago
Created attachment 8736512 [details] [diff] [review]
0001-Bug-1260588-C-APZ-should-only-allow-handoff-to-ancestor-APZC-r-botond-16033016-55455ba.patch

Address review comments, carry forward r+ from :botond.
Attachment #8736068 - Attachment is obsolete: true
(Assignee)

Comment 4

2 years ago
Created attachment 8736513 [details] [diff] [review]
0001-Bug-1260588-C-APZ-should-only-allow-handoff-to-ancestor-APZC-r-botond-16033016-4b6325f.patch

Uploaded wrong patch.
Attachment #8736512 - Attachment is obsolete: true
Keywords: polish
Whiteboard: [gfx-noted]

Comment 5

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5620c5785d8e
https://hg.mozilla.org/mozilla-central/rev/5620c5785d8e
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.