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)
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: botond, Assigned: botond)
References
Details
(Whiteboard: [gfx-noted])
Attachments
(7 files)
59 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
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•7 years ago
|
Assignee: nobody → botond
Assignee | ||
Updated•7 years 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 | ||
Comment 7•7 years ago
|
||
This is a WIP patch series; it compiles but needs testing.
Assignee | ||
Comment 8•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f8084023e15bd9723f35905fcc36dea6f119229
Updated•7 years ago
|
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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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!
Assignee | ||
Comment 29•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7f52cfc6e2a5f5c6f84f605690199b9af71ca92
Comment hidden (mozreview-request) |
Assignee | ||
Comment 31•7 years 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•7 years 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 33•7 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 34•7 years ago
|
||
So what's next, async autoscrolling?
Assignee | ||
Comment 35•7 years 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.
Description
•