All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED in Firefox 55

Status

()

P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: botond, Assigned: botond)

Tracking

55 Branch
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(7 attachments)

(Assignee)

Description

a year ago
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)

Updated

a year ago
Assignee: nobody → botond
(Assignee)

Updated

a year ago
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Depends on: 1365088
(Assignee)

Comment 7

a year ago
This is a WIP patch series; it compiles but needs testing.
status-firefox55: --- → affected
Priority: -- → P3
Whiteboard: [gfx-noted]
Version: Trunk → 55 Branch
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 16

a year ago
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 hidden (mozreview-request)

Comment 18

a year ago
mozreview-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 19

a year ago
mozreview-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 20

a year ago
mozreview-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 21

a year ago
mozreview-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 22

a year ago
mozreview-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 23

a year ago
mozreview-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 24

a year ago
mozreview-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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 28

a year ago
(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!
Comment hidden (mozreview-request)
(Assignee)

Comment 31

a year ago
(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

a year ago
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

Comment 34

a year ago
So what's next, async autoscrolling?
(Assignee)

Comment 35

a year ago
(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.