Closed
Bug 1260588
Opened 8 years ago
Closed 8 years ago
C++ APZ should only allow handoff to ancestor APZC
Categories
(Core :: Panning and Zooming, defect)
Core
Panning and Zooming
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 | ||
Updated•8 years ago
|
Assignee: nobody → rbarker
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8736068 -
Flags: review?(botond)
Comment 2•8 years ago
|
||
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•8 years ago
|
||
Address review comments, carry forward r+ from :botond.
Attachment #8736068 -
Attachment is obsolete: true
Assignee | ||
Comment 4•8 years ago
|
||
Uploaded wrong patch.
Attachment #8736512 -
Attachment is obsolete: true
Comment 6•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5620c5785d8e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•