Botond Ballo [:botond] Assignee
	Description • 11 years ago

Bug 912666 introduced the notion of scroll-grabbing, where an outer scrollable element can be marked as 'scrollgrab' to have priority for scrolling over inner scrollable elements.

With the current implementation, the outer element also grabs tap gestures, which is not desirable.

	Botond Ballo [:botond] Assignee
	Comment 1 • 11 years ago

The attached patch fixes the problem by slightly reworking the way scroll grabbing is implemented.

Rather than rerouting all touch events in a one-finger touch block to the scroll grabbing APZC, we now send them to the innermost APZC still, but we still build the overscroll handoff chain, and when scrolling is about to happen we reroute the scroll to the first APZC in the chain. In the absence of scroll-grabbing, this re-routing becomes a no-op (I made sure we don't do an unnecessary transformation round-trip in this case).

	Chris Lord [:cwiiis]
	Comment 2 • 11 years ago

Comment on attachment 8340165 [details] [diff] [review]
bug944521.patch

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

Great work - I'm happy for this to go in as is (minus the spurious logging, perhaps), but feel free to address any of the comments as you see fit.

::: gfx/layers/composite/APZCTreeManager.cpp
@@ +753,5 @@
>    // and users of 'scrollgrab' should not rely on this.)
>    std::stable_sort(mOverscrollHandoffChain.begin(), mOverscrollHandoffChain.end(),
>                     CompareByScrollPriority());
>  
> +  apzclog2 << "Overscroll handoff chain after accounting for scroll grab: " << mOverscrollHandoffChain << endline;

Is this intended to be in the final patch? (I'm guessing not, but if so, perhaps this ought to be in an #ifdef?)

::: gfx/layers/composite/APZCTreeManager.h
@@ +245,5 @@
> +   *   - TM.DispatchScroll() calls C.AttemptScroll() (since C is at index 1 in the chain)
> +   *   - C.AttemptScroll() scrolls C. If there is overscroll, it calls TM.DispatchScroll() with index = 2.
> +   *   - TM.DispatchScroll() calls A.AttemptScroll() (since A is at index 2 in the chain)
> +   *   - A.AttemptScroll() scrolls A. If there is overscroll, it calls TM.DispatchScroll() with index = 3.
> +   *   - TM.DispatchScroll() discards the rest of the scroll as there are no more elements in the chain.

Love this example, good work :)

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +923,5 @@
>      }
>    }
>  
>    if (fabs(overscroll.x) > EPSILON || fabs(overscroll.y) > EPSILON) {
> +    // "+ overscroll" rather than "- overscroll" for the same reason as above.

As kats said to me in a recent review, code tends to move around a lot and 'as above' may not stay true for long. Better to re-state the reason (as concisely as possible), unless you're confident that this code will always be together.

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +280,5 @@
>                       uint32_t aOverscrollHandoffChainIndex = 0);
>  
>    /**
> +   * A helper function for calling APZCTreeManager::DispatchScroll() that
> +   * handles some concurrency issues.

This comment is a little vague - it'd be nice to hear what those issues are. Also, if this is always the function that should be used from APZC, perhaps the APZCTreeManager version of it should have a more conspicuous name, to avoid being mistakenly called?

	Botond Ballo [:botond] Assignee
	Comment 3 • 11 years ago

(In reply to Chris Lord [:cwiiis] from comment #2)
> Comment on attachment 8340165 [details] [diff] [review]
> bug944521.patch
> 
> Review of attachment 8340165 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great work - I'm happy for this to go in as is (minus the spurious logging,
> perhaps), but feel free to address any of the comments as you see fit.
> 
> ::: gfx/layers/composite/APZCTreeManager.cpp
> @@ +753,5 @@
> >    // and users of 'scrollgrab' should not rely on this.)
> >    std::stable_sort(mOverscrollHandoffChain.begin(), mOverscrollHandoffChain.end(),
> >                     CompareByScrollPriority());
> >  
> > +  apzclog2 << "Overscroll handoff chain after accounting for scroll grab: " << mOverscrollHandoffChain << endline;
> 
> Is this intended to be in the final patch? (I'm guessing not, but if so,
> perhaps this ought to be in an #ifdef?)

Whoops, that snuck in there during a rebase. Thanks for catching it!

I will address the other comments as well.

	Botond Ballo [:botond] Assignee
	Comment 4 • 11 years ago

Updated patch to address Chris' review comments. Carrying r+.

	Botond Ballo [:botond] Assignee
	Comment 5 • 11 years ago

Try push: https://tbpl.mozilla.org/?tree=Try&rev=eb27138927fb

	Botond Ballo [:botond] Assignee
	Comment 6 • 11 years ago

(In reply to Botond Ballo [:botond] from comment #5)
> Try push: https://tbpl.mozilla.org/?tree=Try&rev=eb27138927fb

Looks like some APZC tests are failing. Will investigate.

	Botond Ballo [:botond] Assignee
	Comment 7 • 11 years ago

(In reply to Botond Ballo [:botond] from comment #6)
> (In reply to Botond Ballo [:botond] from comment #5)
> > Try push: https://tbpl.mozilla.org/?tree=Try&rev=eb27138927fb
> 
> Looks like some APZC tests are failing. Will investigate.

There was nothing wrong with the code; the problem was that with this change, panning an APZC requires its mTreeManager to be non-null, which is always the case in real code, but wasn't in the test code.

Updated patch to fix the tests. Re-requesting review for test changes.

Also, new try push: https://tbpl.mozilla.org/?tree=Try&rev=391907d29c90

	Chris Lord [:cwiiis]
	Comment 8 • 11 years ago

Comment on attachment 8340555 [details] [diff] [review]
bug944521.patch

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

I think this is good enough, but I wonder if it would've been cleaner to make BuildOverscrollHandoffChain a static method that takes a tree manager and can handle the aTreeManager == nullptr case? At this point, I think that's down to personal preference, and I'd like to see this code in the tree already :)

	Botond Ballo [:botond] Assignee
	Comment 9 • 11 years ago

(In reply to Chris Lord [:cwiiis] from comment #8)
> I think this is good enough, but I wonder if it would've been cleaner to
> make BuildOverscrollHandoffChain a static method that takes a tree manager
> and can handle the aTreeManager == nullptr case? At this point, I think
> that's down to personal preference, and I'd like to see this code in the
> tree already :)

In order for panning to work without a tree manager, AsyncPanZoomController::TrackTouch would need to handle the case where mTreeManager == nullptr. I'm reluctant to add that as it should never arise in production code, only in test code.

	Botond Ballo [:botond] Assignee
	Comment 10 • 11 years ago

(In reply to Botond Ballo [:botond] from comment #7)
> Also, new try push: https://tbpl.mozilla.org/?tree=Try&rev=391907d29c90

Looks clean.

	Botond Ballo [:botond] Assignee
	Comment 11 • 11 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/edc83d2520ec

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/edc83d2520ec