Closed Bug 1260588 Opened 5 years ago Closed 5 years ago

C++ APZ should only allow handoff to ancestor APZC

Categories

(Core :: Panning and Zooming, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: rbarker, Assigned: rbarker)

References

Details

(Keywords: polish, Whiteboard: [gfx-noted])

Attachments

(1 file, 2 obsolete files)

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: nobody → rbarker
Attachment #8736068 - Flags: review?(botond)
Blocks: 1257269
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+
Address review comments, carry forward r+ from :botond.
Attachment #8736068 - Attachment is obsolete: true
Keywords: polish
Whiteboard: [gfx-noted]
https://hg.mozilla.org/mozilla-central/rev/5620c5785d8e
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.