Closed
Bug 1418387
Opened 6 years ago
Closed 6 years ago
Make APZ hit-test scrollbars properly when using WR hit-testing
Categories
(Core :: Graphics: WebRender, enhancement, P1)
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox57 | --- | unaffected |
firefox59 | --- | fixed |
People
(Reporter: kats, Assigned: kats)
References
Details
(Whiteboard: [wr-mvp] [gfx-noted])
Attachments
(6 files)
59 bytes,
text/x-review-board-request
|
botond
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
botond
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
botond
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mstange
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
botond
:
review+
|
Details |
Bug 1389149 and bug 1417519 set up some infrastructure to allow APZ to do hit-testing via WR. However it is incomplete; one of the missing pieces is scrollbar and scrollthumb hit-testing. That will be done in this bug.
Updated•6 years ago
|
Updated•6 years ago
|
Status: NEW → ASSIGNED
Priority: P2 → P1
Whiteboard: [gfx-noted][wr-mvp][triage] → [wr-mvp] [gfx-noted]
Assignee | ||
Comment 1•6 years ago
|
||
I've been having a lot of trouble making all the scrollbar hit-testing cases work exactly the same as with non-webrender. There's a lot of cases - at the very least {root scrollbars, active subframe scrollbars, inactive subframe scrollbars} x {overlay scrollbars, non-overlay scrollbars}. For each these we may or may not generate a nsDisplayOwnLayer for the scrollbar "container" and we may or may not generate a nsDisplayOwnLayer for the scrollthumb. Some of these cases are already handled incorrectly with webrender, so I can't use that as a baseline for comparison. Instead I have to use the non-webrender codepath as my gold standard, but even there I think some of these cases fall into the "don't care" bucket as to whether or not we return a HitTestingTreeNode for the scrollbar container hit-test case. After a few different implementation attempts I have something now that I *think* should produce correct user-visible behaviour in all the cases but I could very well be wrong. I'm inclined to land this for now and then if we run into scenarios where the user-observed behaviour is wrong we can debug that and adjust on a case-by-case basis. The code I have now is thankfully relatively straightforward (some of my earlier attempts were quite hideous).
Assignee | ||
Comment 2•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e3256551b747feb9a4c7062ef165dff28763a4d
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 6•6 years ago
|
||
mozreview-review |
Comment on attachment 8930534 [details] Bug 1418387 - Make the nsDisplayOwnLayer flags more strongly typed. https://reviewboard.mozilla.org/r/201658/#review207018
Attachment #8930534 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 7•6 years ago
|
||
I'm going to make a slight change to the semantics of the eScrollbar bit. Specifically I'm going to set eScrollbar for any frame *including* the thumb that's a descendant of the scrollbar frame in the frame tree. Any frame which gets eScrollbarThumb will now also have eScrollbar, instead of the two being mutually exclusive. This removes some conditions and simplifies later work. I'll document the semantics of the two bits in CompositorHitTestInfo.h as well.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 11•6 years ago
|
||
mozreview-review |
Comment on attachment 8930533 [details] Bug 1418387 - Fix bugs that C++ should really catch for us. https://reviewboard.mozilla.org/r/201656/#review207460
Attachment #8930533 -
Flags: review?(botond) → review+
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8930535 [details] Bug 1418387 - Propagate scrollbar direction for scrollbar containers to APZ. https://reviewboard.mozilla.org/r/201660/#review207462 GetAPZCAtPoint computes the target scroll frame differently depending on whether a scrollbar node was hit. If a non-scrollbar node was hit, it just walks up the hit-testing tree to find the nearest enclosing APZC. If a scrollbar node was hit, it uses the target scroll frame (the nearest enclosing APZC would be the parent scroll frame, which is not what we want). Does WebRenderApi::HitTest() have similar logic, to ensure that the correct scroll frame is chosen if a scrollbar node is hit? ::: gfx/layers/apz/src/APZCTreeManager.cpp:2267 (Diff revision 2) > result = FindRootApzcForLayersId(layersId); > MOZ_ASSERT(result); > } > > + bool scrollbar = bool(hitInfo & gfx::CompositorHitTestInfo::eScrollbar); > + bool scrollthumb = bool(hitInfo & gfx::CompositorHitTestInfo::eScrollbarThumb); Would prefer calling these |isScrollbar|, |isScrollbarThumb|. ::: gfx/layers/apz/src/APZCTreeManager.cpp:2274 (Diff revision 2) > + *aOutScrollbarNode = BreadthFirstSearch<ReverseIterator>(mRootNode.get(), > + [&](HitTestingTreeNode* aNode) { > + return (aNode->GetLayersId() == layersId) && > + (aNode->IsScrollbarNode() == scrollbar) && > + (aNode->IsScrollThumbNode() == scrollthumb) && > + (aNode->GetScrollTargetId() == scrollId); This information does not uniquely identify the scrollbar node, since for a scroll frame that's scrollable in both directions, it's the same for the horizontal and vertical thumbs (and horizontal and vertical non-thumb areas).
Attachment #8930535 -
Flags: review?(botond) → review-
Assignee | ||
Comment 13•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8930535 [details] Bug 1418387 - Propagate scrollbar direction for scrollbar containers to APZ. https://reviewboard.mozilla.org/r/201660/#review207462 The logic for this is in part 2 - in case of the hitting scrollbars, the scrollId returned is that of the target scrollframe rather than the parent scrollframe. (This happens in the nsDisplayCompositorHitTestInfo constructor). > Would prefer calling these |isScrollbar|, |isScrollbarThumb|. Sure, I can update this. > This information does not uniquely identify the scrollbar node, since for a scroll frame that's scrollable in both directions, it's the same for the horizontal and vertical thumbs (and horizontal and vertical non-thumb areas). Indeed, good point! I'll have to send the scroll direction over as well somehow.
Assignee | ||
Comment 14•6 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #13) > Comment on attachment 8930535 [details] > Bug 1418387 - Find the appropriate scrollbar node in GetAPZCAtPointWR. > > https://reviewboard.mozilla.org/r/201660/#review207462 > > The logic for this is in part 2 - in case of the hitting scrollbars, MozReview didn't put the context, but this is in reply to the first part of your review comments. Also this should say "in the case of hitting scrollbars"
Assignee | ||
Updated•6 years ago
|
Depends on: 1419440
See Also: → https://github.com/servo/webrender/pull/2082
Assignee | ||
Comment 15•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec894f70b5e26ef57d0eefea731fc920f555a874
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 22•6 years ago
|
||
Comment on attachment 8930533 [details] Bug 1418387 - Fix bugs that C++ should really catch for us. MozReview propagated the flags incorrectly (probably my fault) so changing the r+ to an r? here in Bugzilla
Attachment #8930533 -
Flags: review+ → review?(botond)
Assignee | ||
Updated•6 years ago
|
Attachment #8930534 -
Flags: review+ → review?(mstange)
Comment 23•6 years ago
|
||
mozreview-review |
Comment on attachment 8931774 [details] Bug 1418387 - Add CompositorHitTestInfo bits for scrollbars. https://reviewboard.mozilla.org/r/202900/#review208214
Attachment #8931774 -
Flags: review?(mstange) → review+
Comment 24•6 years ago
|
||
mozreview-review |
Comment on attachment 8930533 [details] Bug 1418387 - Fix bugs that C++ should really catch for us. https://reviewboard.mozilla.org/r/201656/#review208216
Attachment #8930533 -
Flags: review?(botond) → review+
Comment 25•6 years ago
|
||
mozreview-review |
Comment on attachment 8930534 [details] Bug 1418387 - Make the nsDisplayOwnLayer flags more strongly typed. https://reviewboard.mozilla.org/r/201658/#review208218
Attachment #8930534 -
Flags: review?(mstange) → review+
Comment 26•6 years ago
|
||
mozreview-review |
Comment on attachment 8931773 [details] Bug 1418387 - Add missing tree lock. https://reviewboard.mozilla.org/r/202898/#review208220
Attachment #8931773 -
Flags: review?(botond) → review+
Comment 27•6 years ago
|
||
mozreview-review |
Comment on attachment 8930535 [details] Bug 1418387 - Propagate scrollbar direction for scrollbar containers to APZ. https://reviewboard.mozilla.org/r/201660/#review208222 ::: gfx/layers/LayerAttributes.h:327 (Diff revision 3) > uint32_t mContentFlags; > float mOpacity; > bool mIsFixedPosition; > uint64_t mScrollbarTargetContainerId; > ScrollThumbData mThumbData; > - bool mIsScrollbarContainer; > + Maybe<ScrollDirection> mScrollbarContainerDirection; It's a bit unfortunate to store the direction both here, and in mThumbData. This could be cleaned up by generalizing ScrollThumbData to ScrollbarData, and adding an isScrollbarContainer flag to it. Could you file a follow-up for that? It's not high priority, we can make it mentored (we're running low on mentored APZ bugs anyways). ::: layout/painting/FrameLayerBuilder.cpp:5201 (Diff revision 3) > > SetupScrollingMetadata(e); > > if (hideAll) { > e->mVisibleRegion.SetEmpty(); > - } else if (!e->mLayer->IsScrollbarContainer()) { > + } else if (e->mLayer->GetScrollbarContainerDirection().isNothing()) { This might read more nicely if we kept the IsScrollbarContainer() getter and just changed its implementation.
Attachment #8930535 -
Flags: review?(botond) → review+
Comment 28•6 years ago
|
||
mozreview-review |
Comment on attachment 8931775 [details] Bug 1418387 - Find the appropriate scrollbar node in GetAPZCAtPointWR. https://reviewboard.mozilla.org/r/202902/#review208228
Attachment #8931775 -
Flags: review?(botond) → review+
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 35•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8930535 [details] Bug 1418387 - Propagate scrollbar direction for scrollbar containers to APZ. https://reviewboard.mozilla.org/r/201660/#review208222 > This might read more nicely if we kept the IsScrollbarContainer() getter and just changed its implementation. Done
Comment 36•6 years ago
|
||
Pushed by kgupta@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4aaddb8a8c96 Fix bugs that C++ should really catch for us. r=botond https://hg.mozilla.org/integration/autoland/rev/93489c5150a0 Make the nsDisplayOwnLayer flags more strongly typed. r=mstange https://hg.mozilla.org/integration/autoland/rev/4ddd3744f6ae Propagate scrollbar direction for scrollbar containers to APZ. r=botond https://hg.mozilla.org/integration/autoland/rev/6243c27e1e99 Add missing tree lock. r=botond https://hg.mozilla.org/integration/autoland/rev/f4e8cd924f81 Add CompositorHitTestInfo bits for scrollbars. r=mstange https://hg.mozilla.org/integration/autoland/rev/152f561ce602 Find the appropriate scrollbar node in GetAPZCAtPointWR. r=botond
Comment 37•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4aaddb8a8c96 https://hg.mozilla.org/mozilla-central/rev/93489c5150a0 https://hg.mozilla.org/mozilla-central/rev/4ddd3744f6ae https://hg.mozilla.org/mozilla-central/rev/6243c27e1e99 https://hg.mozilla.org/mozilla-central/rev/f4e8cd924f81 https://hg.mozilla.org/mozilla-central/rev/152f561ce602
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in
before you can comment on or make changes to this bug.
Description
•