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)

Product:
Core
Component:
Graphics: WebRender
Version:
Other Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox57 --- unaffected
firefox59 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [wr-mvp] [gfx-noted])

Attachments

(6 files)

Bug 1418387 - Fix bugs that C++ should really catch for us.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1418387 - Make the nsDisplayOwnLayer flags more strongly typed.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1418387 - Propagate scrollbar direction for scrollbar containers to APZ.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1418387 - Add missing tree lock.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1418387 - Add CompositorHitTestInfo bits for scrollbars.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1418387 - Find the appropriate scrollbar node in GetAPZCAtPointWR.
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.
Status: NEW → ASSIGNED
Priority: P2 → P1
Whiteboard: [gfx-noted][wr-mvp][triage] → [wr-mvp] [gfx-noted]
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).
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+
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 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 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-
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.
(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"
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)
Attachment #8930534 - Flags: review+ → review?(mstange)
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 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 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 on attachment 8931773 [details]
Bug 1418387 - Add missing tree lock.

https://reviewboard.mozilla.org/r/202898/#review208220
Attachment #8931773 - Flags: review?(botond) → 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 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+
Depends on: 1420516
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
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
You need to log in before you can comment on or make changes to this bug.