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

Status: RESOLVED FIXED

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

Description

[Botond Ballo [:botond]]

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.

Comment 1

[Botond Ballo [:botond]]

Attachment: Bug 1364622 - Introduce a helper function for computing the async transform for a scroll thumb layer.

Review commit: https://reviewboard.mozilla.org/r/139482/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139482/

Comment 2

[Botond Ballo [:botond]]

Attachment: Bug 1364622 - Factor out a helper function for computing the current async transform for a HitTestingTreeNode.

Review commit: https://reviewboard.mozilla.org/r/139484/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139484/

Comment 3

[Botond Ballo [:botond]]

Attachment: Bug 1364622 - Add a utility function AsyncPanZoomController::CallWithLastContentPaintMetrics().

Review commit: https://reviewboard.mozilla.org/r/139486/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139486/

Comment 4

[Botond Ballo [:botond]]

Attachment: Bug 1364622 - Make APZCTreeManager::GetTargetNode() const.

Review commit: https://reviewboard.mozilla.org/r/139488/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139488/

Comment 5

[Botond Ballo [:botond]]

Attachment: Bug 1364622 - Add a helper function HitTestingTreeNode::IsAncestorOf().

Review commit: https://reviewboard.mozilla.org/r/139490/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139490/

Comment 6

[Botond Ballo [:botond]]

Attachment: Bug 1364622 - Consider async transforms on scroll thumb layers during hit testing.

Review commit: https://reviewboard.mozilla.org/r/139492/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139492/

Comment 7

[Botond Ballo [:botond]]

This is a WIP patch series; it compiles but needs testing.

Comment 8

[Botond Ballo [:botond]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f8084023e15bd9723f35905fcc36dea6f119229

Comment 9

[Botond Ballo [:botond]]

Attachment: Bug 1364622 - Consider the async transforms on the scroll thumb when computing the drag start offset during APZ initiation of scrollbar dragging.

Review commit: https://reviewboard.mozilla.org/r/140302/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140302/

Comment 10

[Botond Ballo [:botond]]

Comment on attachment 8867950 [details]
Bug 1364622 - Introduce a helper function for computing the async transform for a scroll thumb layer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139482/diff/1-2/

Comment 11

[Botond Ballo [:botond]]

Comment on attachment 8867951 [details]
Bug 1364622 - Factor out a helper function for computing the current async transform for a HitTestingTreeNode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139484/diff/1-2/

Comment 12

[Botond Ballo [:botond]]

Comment on attachment 8867952 [details]
Bug 1364622 - Add a utility function AsyncPanZoomController::CallWithLastContentPaintMetrics().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139486/diff/1-2/

Comment 13

[Botond Ballo [:botond]]

Comment on attachment 8867953 [details]
Bug 1364622 - Make APZCTreeManager::GetTargetNode() const.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139488/diff/1-2/

Comment 14

[Botond Ballo [:botond]]

Comment on attachment 8867954 [details]
Bug 1364622 - Add a helper function HitTestingTreeNode::IsAncestorOf().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139490/diff/1-2/

Comment 15

[Botond Ballo [:botond]]

Comment on attachment 8867955 [details]
Bug 1364622 - Consider async transforms on scroll thumb layers during hit testing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139492/diff/1-2/

Comment 16

[Botond Ballo [:botond]]

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 17

[Botond Ballo [:botond]]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140302/diff/1-2/

Comment 18

[Kartikaya Gupta (email:[e-mail])]

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 :)

Comment 19

[Kartikaya Gupta (email:[e-mail])]

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

Comment 20

[Kartikaya Gupta (email:[e-mail])]

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 :)

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8867953 [details]
Bug 1364622 - Make APZCTreeManager::GetTargetNode() const.

https://reviewboard.mozilla.org/r/139488/#review143630

Comment 22

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 23

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8867955 [details]
Bug 1364622 - Consider async transforms on scroll thumb layers during hit testing.

https://reviewboard.mozilla.org/r/139492/#review143634

Comment 24

[Kartikaya Gupta (email:[e-mail])]

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!

Comment 25

[Botond Ballo [:botond]]

Comment on attachment 8867954 [details]
Bug 1364622 - Add a helper function HitTestingTreeNode::IsAncestorOf().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139490/diff/2-3/

Comment 26

[Botond Ballo [:botond]]

Comment on attachment 8867955 [details]
Bug 1364622 - Consider async transforms on scroll thumb layers during hit testing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139492/diff/2-3/

Comment 27

[Botond Ballo [:botond]]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140302/diff/2-3/

Comment 28

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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!

Comment 29

[Botond Ballo [:botond]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7f52cfc6e2a5f5c6f84f605690199b9af71ca92

Comment 30

[Botond Ballo [:botond]]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140302/diff/3-4/

Comment 31

[Botond Ballo [:botond]]

(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

Comment 32

[Pulsebot]

Pushed by [e-mail]:
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

Comment 33

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/5a18a862fcae
https://hg.mozilla.org/mozilla-central/rev/aee667fdb9fc
https://hg.mozilla.org/mozilla-central/rev/240bd145226d
https://hg.mozilla.org/mozilla-central/rev/d3762999925c
https://hg.mozilla.org/mozilla-central/rev/5f9d2643a7cc
https://hg.mozilla.org/mozilla-central/rev/9cb79caf9f1a
https://hg.mozilla.org/mozilla-central/rev/281bfdc318c6

Comment 34

[avada]

So what's next, async autoscrolling?

Comment 35

[Botond Ballo [:botond]]

(In reply to avada from comment #34)
> So what's next, async autoscrolling?

Yes (bug 1105109), and async keyboard scrolling (bug 1351783).