1429373 - Scrollbar does not drag properly when inside a modal element and APZ is enabled

Status: RESOLVED FIXED

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

Description

[elubarsky]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20180103231032

Steps to reproduce:

See https://qk31v7vw0q.codesandbox.io/ (source: https://codesandbox.io/s/qk31v7vw0q)


Actual results:

Attempting to scroll by dragging the scrollbar does not work in FF57 & FF58.
Clicking the arrows or using the mouse wheel does work.
Setting apz.drag.enabled to false immediately resolves the issue.

Comment 1

[Alice0775 White]

regression window:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ddd566eb39b7a39d0ff630343f179fedc8fcafd5&tochange=7e18014be68d9e13760b2814862745911caf49a5

regressed by: Bug 1168263

and seems to be fixed by Bug 1424591

Comment 2

[elubarsky]

Ah ok, Bug 1424591 didn't come up as a similar issue when searching Bugzilla..

On Firefox 59 the second test case is still broken for me (apologies - it's unclear there are two). Here's a direct link: https://6j926wx92n.codesandbox.io/

Comment 3

[Alice0775 White]

[Tracking Requested - why for this release]: Scrollbar is broken

(In reply to elubarsky from comment #2)
> Ah ok, Bug 1424591 didn't come up as a similar issue when searching
> Bugzilla..
> 
> On Firefox 59 the second test case is still broken for me (apologies - it's
> unclear there are two). Here's a direct link:
> https://6j926wx92n.codesandbox.io/

I can reproduce the issue on Nightly59.0a1.
In my case it needed zoom level > 133% and reload.

Comment 4

[Alice0775 White]

@mattwoodrow
This seemed to be regressed by your parch.
Could you look into this?

Comment 5

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

/cc botond also as he might have a better idea of what's going on since he worked on this more recently.

Comment 6

[Botond Ballo [:botond]]

(In reply to Alice0775 White from comment #3)
> (In reply to elubarsky from comment #2)
> > On Firefox 59 the second test case is still broken for me (apologies - it's
> > unclear there are two). Here's a direct link:
> > https://6j926wx92n.codesandbox.io/
> 
> I can reproduce the issue on Nightly59.0a1.
> In my case it needed zoom level > 133% and reload.

Yeah, there's definitely something going on with that testcase.

  - At 100% zoom, dragging the scrollbar works in that it
    scrolls the dropdown contents, but sometimes the
    scrollbar thumb itself disappears while dragged.

  - At 140% zoom, when the page becomes scrollable and
    you have to scroll it down to bring the dropdown into
    view, dragging the scrollbar doesn't work at all.

Comment 7

[Botond Ballo [:botond]]

(In reply to elubarsky from comment #2)
> On Firefox 59 the second test case is still broken for me (apologies - it's
> unclear there are two). Here's a direct link:
> https://6j926wx92n.codesandbox.io/

Is the source code of this package available? I went to https://codesandbox.io/s/6j926wx92n and clicked Download, but the public/index.html in the downloaded zip file is blank.

Comment 8

[Botond Ballo [:botond]]

Attachment: Testcase

Never mind, I figured it out: you had to install and run a node server for the testcase to work.

Here is the testcase in a form that's more suitable for testing (no server required).

Comment 9

[Botond Ballo [:botond]]

Attachment: Reduced testcase

And here is a reduced testcase.

Comment 10

[Botond Ballo [:botond]]

I think what's going on here is that the "propagate the scroll-clip on a child of a perspective layer up to the perspective layer itself" business that we did in bug 1168263 [1] needs to be done for hit-testing purposes as well (whereas we only did it for rendering purposes).

[1] https://reviewboard.mozilla.org/r/25559/diff/4#index_header

Comment 11

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #10)
> I think what's going on here is that the "propagate the scroll-clip on a
> child of a perspective layer up to the perspective layer itself" business
> that we did in bug 1168263 [1] needs to be done for hit-testing purposes as
> well (whereas we only did it for rendering purposes).

With that in place, I'm getting correct hit-testing results, but the calculations in AsyncPanZoomController::HandleDragEvent() still go wrong.

Comment 12

[Botond Ballo [:botond]]

The remainder of the problem is the same underlying issue as in bug 1424591.

In bug 1424591 comment 7, I said:

(In reply to Botond Ballo [:botond] from comment #7)
> The motivation for excluding perspective transforms is to address scenarios
> where we have multiple layers that scroll together, and some of them are
> covered by a perspective transform and some are not. In such cases,
> including the perspective transform leads to the different layers having
> different ancestor transforms, which violates APZ constraint (the APZC needs
> to have a single ancestor transform).
> 
> The page in this bug is not like that: here, the APZC in question only has
> one layer, and the perspective transform is on an ancestor layer. In such
> cases, excluding the perspective transform is not necessary.
> 
> So, the problem can be resolved by limiting the exclusion to perspective
> transforms which are on the path from layers that scroll together to their
> common ancestor.

The page in that bug did not have multiple layers that scroll together, but the page in this bug does, so the mitigation implemented in that bug does not apply to this page.

Comment 13

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #12)
> The remainder of the problem is the same underlying issue as in bug 1424591.

Slight correction: it's not quite the same issue. The issue there concerned comparisons between quantities in the ParentLayer space of the APZC that scrolls the perspective-transformed content.

Here, the issue is calculations for a descendant APZC of that APZC. Since we are excluding the perspective transform from the latter's ancestor transform, the descendant's apzc-to-screen transform will exclude the perspective transform altogether, leading to inaccurate coordinate conversions. We can fix this "deferring" the perspective transform so it applies to the child instead, ensuring that it's still part of the child's apzc-to-screen transform.

With this in place, the scrollbar can now be dragged in both the reduced testcase and the original testcase. However, the "sometimes the scrollbar thumb itself disappears while dragged" issue mentioned in comment 6 (first bullet) persists.

Comment 14

[Botond Ballo [:botond]]

Attachment: Bug 1429373 - Add a clarifying comment to the declaration of APZCTreeManager::SetTargetAPZC().

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

Comment 15

[Botond Ballo [:botond]]

Attachment: Bug 1429373 - During hit-testing, ignore clips on layers whose parent has a perspective transform.

The "scroll clip" in this context refers to ScrollMetadata::mScrollClip,
and the clip based on composition bounds that ComputeClipRegion() uses
in its place if there isn't one.

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

Comment 16

[Botond Ballo [:botond]]

Attachment: Bug 1429373 - If a perspective transform is excluded from an APZC's ancestor transform, include it in the ancestor transforms of its child APZCs.

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

Comment 17

[Botond Ballo [:botond]]

I posted my WIP patch series.

In addition to needing some cleanup, the second patch (the one that addresses the hit testing issue) is incomplete because it implements a new function IsChildOfPerspectiveLayer() for LayerMetricsWrapper but not for WebRenderScrollDataWrapper.

Implementing IsChildOfPerspectiveLayer() for WebRenderScrollDataWrapper is not as trivial as for LayerMetricsWrapper because WebRenderScrollDataWrapper does implement a GetParent() function the way LayerMetricsWrapper does.

I looked into what it would take to implement WebRenderScrollDataWrapper::GetParent(), and I think it would require storing an extra piece of data in WebRenderScrollDataWrapper, such as "mParentDescendantCount" (then the index of the parent could be computed as "mContainingSubtreeLastIndex - mParentDescendantCount" or similar).

Alternatively, I could change tack and have the tree traversal that consumes the IsChildOfPerspectiveLayer() information just use IsPerspectiveLayer() instead, and propagate the info down to the child itself.

Kats, do you have a preference between these approaches? (In my mind, it really boils down to "do you foresee needing to eventually add WebRenderScrollDataWrapper::GetParent() anyways for others reasons, so we may as well add it now?").

Comment 18

[Botond Ballo [:botond]]

I should mention that having to make these changes in the first place is fairly unfortunate.

:mattwoodrow suggested a cleaner and more principled way for handling perspective transforms as a longer-term solution, requiring changes to Layout code, for which I filed bug 1431249.

Comment 19

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

(In reply to Botond Ballo [:botond] from comment #17)
> Kats, do you have a preference between these approaches? (In my mind, it
> really boils down to "do you foresee needing to eventually add
> WebRenderScrollDataWrapper::GetParent() anyways for others reasons, so we
> may as well add it now?").

I don't foresee needing to add that for other reasons, but to be honest I didn't have too clear of a mental picture of what the scroll data wrapper stuff would look like with the WR hit testing fully in-place, so I don't have a lot of confidence in this. I'd say whichever approach is easier for you is fine by me.

One question though - should the perspective-ness affect just the child, or the whole descendant subtree?

Comment 20

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #19)
> One question though - should the perspective-ness affect just the child, or
> the whole descendant subtree?

In this context, just the child. There's a particular clip on the child layer of a perspective layer that needs to be ignored, since the clip on the parent layer is what's relevant instead. We've hacked AsyncCompositionManager to do this back in bug 1168263, but we didn't do the corresponding change to hit-testing.

Comment 21

[Botond Ballo [:botond]]

Comment on attachment 8943432 [details]
Bug 1429373 - During hit-testing, ignore clips on layers whose parent has a perspective transform.

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

Comment 22

[Botond Ballo [:botond]]

Comment on attachment 8943433 [details]
Bug 1429373 - If a perspective transform is excluded from an APZC's ancestor transform, include it in the ancestor transforms of its child APZCs.

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

Comment 23

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #19)
> I'd say whichever approach is easier for you is fine by me.

I went with the second approach (passing down the information during tree traversal) as it ended up being cleaner.

I posted my updated patches. Since this kind of hackery has some regression risk, I'd like to see the results of a Try push before requesting review (but Try is closed at the moment).

Comment 24

[Botond Ballo [:botond]]

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

Comment 25

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #24)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=63ff4c34c5514c212f015a82ebea82aadf7a5b63

Looking good. Posting patches for review.

Comment 26

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

Comment on attachment 8943432 [details]
Bug 1429373 - During hit-testing, ignore clips on layers whose parent has a perspective transform.

https://reviewboard.mozilla.org/r/213758/#review220244

::: gfx/layers/apz/src/APZCTreeManager.cpp:763
(Diff revision 2)
> -        aLayer.GetClipRect() ? Some(ParentLayerIntRegion(*aLayer.GetClipRect())) : Nothing(),
> +        (!aParentHasPerspective && aLayer.GetClipRect()) 
> +          ? Some(ParentLayerIntRegion(*aLayer.GetClipRect())) 
> +          : Nothing(),

nit: trailing ws

::: gfx/layers/apz/src/APZCTreeManager.cpp:872
(Diff revision 2)
> -    ParentLayerIntRegion clipRegion = ComputeClipRegion(state->mController, aLayer);
> +    Maybe<ParentLayerIntRegion> clipRegion = 
> +        aParentHasPerspective 
> +      ? Nothing() 

more trailing ws. Also I'd just leave aParentHasPerspective on the previous line

::: gfx/layers/apz/src/APZCTreeManager.cpp:965
(Diff revision 2)
> -    ParentLayerIntRegion clipRegion = ComputeClipRegion(state->mController, aLayer);
> +    Maybe<ParentLayerIntRegion> clipRegion = 
> +        aParentHasPerspective 
> +      ? Nothing() 
> +      : Some(ComputeClipRegion(state->mController, aLayer));

ditto

Comment 27

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

Comment on attachment 8943431 [details]
Bug 1429373 - Add a clarifying comment to the declaration of APZCTreeManager::SetTargetAPZC().

https://reviewboard.mozilla.org/r/213756/#review220246

Comment 28

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

Comment on attachment 8943433 [details]
Bug 1429373 - If a perspective transform is excluded from an APZC's ancestor transform, include it in the ancestor transforms of its child APZCs.

https://reviewboard.mozilla.org/r/213760/#review220248

r+ with the correctness issue below addressed

::: gfx/layers/apz/src/APZCTreeManager.h:611
(Diff revision 2)
>    HitTestingTreeNode* PrepareNodeForLayer(const ScrollNode& aLayer,
>                                            const FrameMetrics& aMetrics,
>                                            uint64_t aLayersId,
>                                            const AncestorTransform& aAncestorTransform,
>                                            HitTestingTreeNode* aParent,
>                                            HitTestingTreeNode* aNextSibling,
>                                            TreeBuildingState& aState,
> -                                          bool aParentHasPerspective);
> +                                          bool aParentHasPerspective,
> +                                          DeferredTransformMap& aPerspectiveTransformsDeferredToChildren);

Can we move some of these arguments into the TreeBuildingState structure? Maybe as a follow-up

::: gfx/layers/apz/src/APZCTreeManager.cpp:409
(Diff revision 2)
> +    // time we realize we need to defer a perspective transform for an APZC,
> +    // we may already have processed a previous layer (including children
> +    // found in its subtree) that shares that APZC.
> +    if (!perspectiveTransformsDeferredToChildren.empty()) {
> +      ForEachNode<ReverseIterator>(mRootNode.get(),
> +          [&perspectiveTransformsDeferredToChildren](HitTestingTreeNode* aNode){

nit: space before {

::: gfx/layers/apz/src/APZCTreeManager.cpp:410
(Diff revision 2)
> +    // we may already have processed a previous layer (including children
> +    // found in its subtree) that shares that APZC.
> +    if (!perspectiveTransformsDeferredToChildren.empty()) {
> +      ForEachNode<ReverseIterator>(mRootNode.get(),
> +          [&perspectiveTransformsDeferredToChildren](HitTestingTreeNode* aNode){
> +            AsyncPanZoomController* apzc = aNode->GetApzc();

There might be multiple HitTestingTreeNodes in the tree with the same APZC, and the way this code is written I think you're going to apply the deferred transform for each of those HitTestingTreeNodes.

I think you want to do a:
if (!aNode->IsPrimaryHolder()) {
  return;
}

or equivalent here.

Comment 29

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

Comment on attachment 8943432 [details]
Bug 1429373 - During hit-testing, ignore clips on layers whose parent has a perspective transform.

https://reviewboard.mozilla.org/r/213758/#review220250

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

Once this work lands on m-c, please let me know if you think we should uplift it to 59 beta. Thanks!

Comment 31

[Botond Ballo [:botond]]

Comment on attachment 8943432 [details]
Bug 1429373 - During hit-testing, ignore clips on layers whose parent has a perspective transform.

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

Comment 32

[Botond Ballo [:botond]]

Comment on attachment 8943433 [details]
Bug 1429373 - If a perspective transform is excluded from an APZC's ancestor transform, include it in the ancestor transforms of its child APZCs.

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

Comment 33

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #28)
> ::: gfx/layers/apz/src/APZCTreeManager.h:611
> (Diff revision 2)
> >    HitTestingTreeNode* PrepareNodeForLayer(const ScrollNode& aLayer,
> >                                            const FrameMetrics& aMetrics,
> >                                            uint64_t aLayersId,
> >                                            const AncestorTransform& aAncestorTransform,
> >                                            HitTestingTreeNode* aParent,
> >                                            HitTestingTreeNode* aNextSibling,
> >                                            TreeBuildingState& aState,
> > -                                          bool aParentHasPerspective);
> > +                                          bool aParentHasPerspective,
> > +                                          DeferredTransformMap& aPerspectiveTransformsDeferredToChildren);
> 
> Can we move some of these arguments into the TreeBuildingState structure?
> Maybe as a follow-up

Good idea. I did this in the updated patches. It also has the side benefit of not having to modify APZCTreeManager.h.

> ::: gfx/layers/apz/src/APZCTreeManager.cpp:410
> (Diff revision 2)
> > +    // we may already have processed a previous layer (including children
> > +    // found in its subtree) that shares that APZC.
> > +    if (!perspectiveTransformsDeferredToChildren.empty()) {
> > +      ForEachNode<ReverseIterator>(mRootNode.get(),
> > +          [&perspectiveTransformsDeferredToChildren](HitTestingTreeNode* aNode){
> > +            AsyncPanZoomController* apzc = aNode->GetApzc();
> 
> There might be multiple HitTestingTreeNodes in the tree with the same APZC,
> and the way this code is written I think you're going to apply the deferred
> transform for each of those HitTestingTreeNodes.
> 
> I think you want to do a:
> if (!aNode->IsPrimaryHolder()) {
>   return;
> }
> 
> or equivalent here.

Good catch! Fixed in the updated patch.

All other review comments are addressed as well.

Comment 34

[Botond Ballo [:botond]]

Comment on attachment 8943433 [details]
Bug 1429373 - If a perspective transform is excluded from an APZC's ancestor transform, include it in the ancestor transforms of its child APZCs.

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

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7acde19831d8
Add a clarifying comment to the declaration of APZCTreeManager::SetTargetAPZC(). r=kats
https://hg.mozilla.org/integration/autoland/rev/5b0b60dd22e8
During hit-testing, ignore clips on layers whose parent has a perspective transform. r=kats
https://hg.mozilla.org/integration/autoland/rev/a76e57103a80
If a perspective transform is excluded from an APZC's ancestor transform, include it in the ancestor transforms of its child APZCs. r=kats

Comment 36

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/7acde19831d8
https://hg.mozilla.org/mozilla-central/rev/5b0b60dd22e8
https://hg.mozilla.org/mozilla-central/rev/a76e57103a80

Comment 37

[Botond Ballo [:botond]]

Comment on attachment 8943432 [details]
Bug 1429373 - During hit-testing, ignore clips on layers whose parent has a perspective transform.

Approval Request Comment
[Feature/Bug causing the regression]:
  Combination of bug 1168263 (APZ support for perspective transforms)
  and bug 1211610 (APZ scrollbar dragging).

[User impact if declined]:
  On some pages involving perspective transforms, some subframe
  scrollbars cannot be dragged.

[Is this code covered by automated tests?]:
  Not really; this is a tricky type of functionality to write
  automated tests for.

[Has the fix been verified in Nightly?]:
  Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 
  No.

[List of other uplifts needed for the feature/fix]:
  None.

[Is the change risky?]:
  Moderately.

[Why is the change risky/not risky?]:
  This is a tricky area of the code where we've had regressions
  in the past. However, we're very early in the beta cycle and 
  any regressions would be limited to scenarios involving
  perspective transforms, so I'm comfortable with the risk.

[String changes made/needed]:
  None.

Comment 38

[Botond Ballo [:botond]]

Comment on attachment 8943431 [details]
Bug 1429373 - Add a clarifying comment to the declaration of APZCTreeManager::SetTargetAPZC().

(See previous comment)

Comment 39

[Botond Ballo [:botond]]

Comment on attachment 8943433 [details]
Bug 1429373 - If a perspective transform is excluded from an APZC's ancestor transform, include it in the ancestor transforms of its child APZCs.

(See previous comment)

Comment 40

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8943432 [details]
Bug 1429373 - During hit-testing, ignore clips on layers whose parent has a perspective transform.

Let's try this for the beta 5 build, since the scope of any potential regressions would be limited, and the fix to scrollbars sounds pretty useful.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/eab542b07371
https://hg.mozilla.org/releases/mozilla-beta/rev/bbfc4a42c362
https://hg.mozilla.org/releases/mozilla-beta/rev/a6ee1ad603bc