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

RESOLVED FIXED in Firefox 59

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: elubarsky, Assigned: botond)

Tracking

({regression})

58 Branch
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox57 wontfix, firefox58 wontfix, firefox59+ fixed, firefox60 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(5 attachments)

Reporter

Description

2 years ago
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.

Updated

2 years ago
Status: UNCONFIRMED → NEW
Component: Untriaged → Panning and Zooming
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Reporter

Comment 2

2 years ago
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

2 years ago
[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.
No longer depends on: 1424591

Comment 4

2 years ago
@mattwoodrow
This seemed to be regressed by your parch.
Could you look into this?
Flags: needinfo?(matt.woodrow)
/cc botond also as he might have a better idea of what's going on since he worked on this more recently.
Assignee

Comment 6

2 years ago
(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.
Assignee

Comment 7

2 years ago
(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.
Flags: needinfo?(elubarsky)
Assignee

Comment 8

2 years ago
Posted file 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).
Flags: needinfo?(elubarsky)
Assignee

Comment 9

2 years ago
Posted file Reduced testcase
And here is a reduced testcase.
Assignee

Comment 10

2 years ago
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
Assignee

Comment 11

2 years ago
(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.
Assignee

Comment 12

2 years ago
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.
Assignee

Updated

2 years ago
See Also: → 1424591
Flags: needinfo?(matt.woodrow)
Assignee

Comment 13

Last year
(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 hidden (mozreview-request)
Assignee

Updated

Last year
Assignee: nobody → botond
Assignee

Comment 17

Last year
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?").
Flags: needinfo?(bugmail)
Assignee

Comment 18

Last year
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.
(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?
Flags: needinfo?(bugmail)
Assignee

Comment 20

Last year
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Priority: -- → P3
Whiteboard: [gfx-noted]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 23

Last year
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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).
Assignee

Comment 25

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

Looking good. Posting patches for review.
Assignee

Updated

Last year
Attachment #8943431 - Flags: review?(bugmail)
Attachment #8943432 - Flags: review?(bugmail)
Attachment #8943433 - Flags: review?(bugmail)
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 on attachment 8943431 [details]
Bug 1429373 - Add a clarifying comment to the declaration of APZCTreeManager::SetTargetAPZC().

https://reviewboard.mozilla.org/r/213756/#review220246
Attachment #8943431 - Flags: review?(bugmail) → review+
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.
Attachment #8943433 - Flags: review?(bugmail) → review+
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
Attachment #8943432 - Flags: review?(bugmail) → review+
Once this work lands on m-c, please let me know if you think we should uplift it to 59 beta. Thanks!
Flags: needinfo?(botond)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 33

Last year
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 hidden (mozreview-request)

Comment 35

Last year
Pushed by bballo@mozilla.com:
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
Assignee

Comment 37

Last year
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.
Flags: needinfo?(botond)
Attachment #8943432 - Flags: approval-mozilla-beta?
Assignee

Comment 38

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

(See previous comment)
Attachment #8943431 - Flags: approval-mozilla-beta?
Assignee

Comment 39

Last year
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)
Attachment #8943433 - Flags: approval-mozilla-beta?
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.
Attachment #8943432 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8943431 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8943433 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee

Updated

Last year
See Also: → 1437694
You need to log in before you can comment on or make changes to this bug.