Closed Bug 1364622 Opened 7 years ago Closed 7 years ago

Take into account async transformation of the thumb layer when APZ initiates scrollbar dragging

Categories

(Core :: Panning and Zooming, enhancement, P3)

55 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: botond, Assigned: botond)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(7 files)

As described in bug 1349750 comment 38, the implementation of APZ initiation of scrollbar dragging has a limitation:

>   1) When initiating an async drag, the APZCTM relies on the position 
>      of the thumb in two places
> 
>       - during hit testing (to determine whether we hit the thumb 
>         rather than another part of the scrollbar)
> 
>       - during computation of the drag start offset
> 
>      However, the position used for these computations does not take
>      into account any async transform that may be applied to the
>      thumb as a result of async-scrolling its target APZC (because
>      that async transform is only computed and applied in
>      AsyncCompositionManager).

I forgot to mention the implication of this: if you async-scroll the page (e.g. with mousewheel), and then grab the thumb before the main thread has had a chance to repaint, *that* drag will not be initiated asynchronously.

This bug tracks fixing that limitation.
Assignee: nobody → botond
Summary: Take into async transformation of the thumb layer when APZ initiates scrollbar dragging → Take into account async transformation of the thumb layer when APZ initiates scrollbar dragging
Depends on: 1365088
This is a WIP patch series; it compiles but needs testing.
Priority: -- → P3
Whiteboard: [gfx-noted]
Version: Trunk → 55 Branch
I realized during testing that I had forgotten the second part of this change, to take the async transform on the thumb into account during computation of the drag start offset. Added that in an additional patch.

With that in place, this seems to be working well. Posting for review.
Comment on attachment 8867950 [details]
Bug 1364622 - Introduce a helper function for computing the async transform for a scroll thumb layer.

https://reviewboard.mozilla.org/r/139482/#review143624

Hah, I basically wrote this same patch in order to reuse this code for updating scrollbar transforms with webrender! But yours is cleaner :)
Attachment #8867950 - Flags: review?(bugmail) → review+
Comment on attachment 8867951 [details]
Bug 1364622 - Factor out a helper function for computing the current async transform for a HitTestingTreeNode.

https://reviewboard.mozilla.org/r/139484/#review143626
Attachment #8867951 - Flags: review?(bugmail) → review+
Comment on attachment 8867952 [details]
Bug 1364622 - Add a utility function AsyncPanZoomController::CallWithLastContentPaintMetrics().

https://reviewboard.mozilla.org/r/139486/#review143628

In my version I just returned a copy of the metrics. But this is fine too :)
Attachment #8867952 - Flags: review?(bugmail) → review+
Comment on attachment 8867953 [details]
Bug 1364622 - Make APZCTreeManager::GetTargetNode() const.

https://reviewboard.mozilla.org/r/139488/#review143630
Attachment #8867953 - Flags: review?(bugmail) → review+
Comment on attachment 8867954 [details]
Bug 1364622 - Add a helper function HitTestingTreeNode::IsAncestorOf().

https://reviewboard.mozilla.org/r/139490/#review143632

::: gfx/layers/apz/src/HitTestingTreeNode.cpp:208
(Diff revision 2)
> +{
> +  while (aOther) {
> +    if (aOther == this) {
> +      return true;
> +    }
> +    aOther = aOther->GetParent();

In general try to avoid writing to passed-in arguments. I'm sure we do it in other places, and this function is small enough that it doesn't really matter but I would have written it as

for (HitTestingTreeNode* i = aOther; i; i = i->GetParent()) {
  if (i == this) {
    return true;
  }
}
return false;

I don't care much either way, your call.
Attachment #8867954 - Flags: review?(bugmail) → review+
Comment on attachment 8867955 [details]
Bug 1364622 - Consider async transforms on scroll thumb layers during hit testing.

https://reviewboard.mozilla.org/r/139492/#review143634
Attachment #8867955 - Flags: review?(bugmail) → review+
Comment on attachment 8868708 [details]
Bug 1364622 - Consider the async transforms on the scroll thumb when computing the drag start offset during APZ initiation of scrollbar dragging.

https://reviewboard.mozilla.org/r/140302/#review143636

Nice clean patchset, thanks!
Attachment #8868708 - Flags: review?(bugmail) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #22)
> ::: gfx/layers/apz/src/HitTestingTreeNode.cpp:208
> (Diff revision 2)
> > +{
> > +  while (aOther) {
> > +    if (aOther == this) {
> > +      return true;
> > +    }
> > +    aOther = aOther->GetParent();
> 
> In general try to avoid writing to passed-in arguments. I'm sure we do it in
> other places, and this function is small enough that it doesn't really
> matter but I would have written it as
> 
> for (HitTestingTreeNode* i = aOther; i; i = i->GetParent()) {
>   if (i == this) {
>     return true;
>   }
> }
> return false;

Revised as suggested. 

Thanks for the reviews!
(In reply to Botond Ballo [:botond] from comment #29)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=a7f52cfc6e2a5f5c6f84f605690199b9af71ca92

The last patch had a typo that was thankfully caught by clang's "statement has no effect" warning. Go static analysis!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0be6b0cda95e740c60a313c838d66ef9c7ad705c
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5a18a862fcae
Introduce a helper function for computing the async transform for a scroll thumb layer. r=kats
https://hg.mozilla.org/integration/autoland/rev/aee667fdb9fc
Factor out a helper function for computing the current async transform for a HitTestingTreeNode. r=kats
https://hg.mozilla.org/integration/autoland/rev/240bd145226d
Add a utility function AsyncPanZoomController::CallWithLastContentPaintMetrics(). r=kats
https://hg.mozilla.org/integration/autoland/rev/d3762999925c
Make APZCTreeManager::GetTargetNode() const. r=kats
https://hg.mozilla.org/integration/autoland/rev/5f9d2643a7cc
Add a helper function HitTestingTreeNode::IsAncestorOf(). r=kats
https://hg.mozilla.org/integration/autoland/rev/9cb79caf9f1a
Consider async transforms on scroll thumb layers during hit testing. r=kats
https://hg.mozilla.org/integration/autoland/rev/281bfdc318c6
Consider the async transforms on the scroll thumb when computing the drag start offset during APZ initiation of scrollbar dragging. r=kats
So what's next, async autoscrolling?
(In reply to avada from comment #34)
> So what's next, async autoscrolling?

Yes (bug 1105109), and async keyboard scrolling (bug 1351783).
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: