1204502 - Test nested-apzc overscroll handoff with opposing constrained axes

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Chris Lord [:cwiiis]]

Bug 1201098 fixed the issue that an apzc that has no scrolling on one particular axis would disable overscroll of that axis of all apzcs further along the overscroll handoff chain.

We should add gtests for this to make certain it remains fixed.

Comment 1

[Botond Ballo [:botond]]

Chris (hi! :)), any plans to write these tests?

Comment 2

[Botond Ballo [:botond]]

I'm going to make this a mentored bug. Chris, please feel free to re-assign to yourself if you're actually going to work on it.

Comment 3

[Tanuja]

Hi! I can take up this bug :D
Botond, any tips on how to get started?
Thanks so much

Comment 4

[Botond Ballo [:botond]]

(In reply to Tanuja from comment #3)
> Hi! I can take up this bug :D

Sure! I assigned the bug to you.

> Botond, any tips on how to get started?

The purpose of this bug is to write a test for the following scenario:

  - There is a parent scrollable element P, which is only scrollable horizontally
  - There is a child scrollable element C, which is only scrollable vertically
  - You try to overscroll (meaning, scroll past the end) C vertically

The correct behaviour is that a vertical overscroll effect is shown for C. Previously, due to a bug (bug 1201098), the presence of P would prevent that from happening.

We use a framework called GTest to write test cases for things like this, and have an existing suite of tests related to scroll handoff (the transfer of scrolling between a child scrollable element and a parent scrollable element) and overscrolling here [1]. You can see there are some existing test cases related to overscrolling there, such as the various StuckInOverscroll_* test cases.

You would need to write a new test case that sets up the above scenario, and tests that the expected behaviour occurs.

Hopefully that's enough to get you started. I expect you'll have more questions as you start working on this, so please feel free to ask here!

[1] http://searchfox.org/mozilla-central/source/gfx/layers/apz/test/gtest/TestScrollHandoff.cpp

Comment 5

[Botond Ballo [:botond]]

Tanuja, did you get a chance to try this? Let me know if there's anything I can help with.

Comment 6

[Tanuja]

Botond, I have started working on this bug but I'll be able to contribute only next week, because I have exams right now. Thanks for your concern.

Comment 7

[Botond Ballo [:botond]]

No problem, good luck with your exams!

Comment 8

[Tanuja]

Botond, is the bug fix been committed to main repository? And how do I run the test?
Thanks!

Comment 9

[Tanuja]

(In reply to Botond Ballo [:botond] from comment #7)
> No problem, good luck with your exams!

Thanks for your patience! =)

Comment 10

[Botond Ballo [:botond]]

(In reply to Tanuja from comment #8)
> Botond, is the bug fix been committed to main repository? 

Yes, in bug 1201098.

> And how do I run the test?

You can run a gtest using a command like:

  ./mach gtest APZScrollHandoffTester.StuckInOverscroll_Bug1073250

This would run the test at [1].

For the test that you write, you would substitute the name of your test for "StuckInOverscroll_Bug1073250".

[1] http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/gfx/layers/apz/test/gtest/TestScrollHandoff.cpp#210

Comment 11

[Botond Ballo [:botond]]

Hey Tanuja, how are things going here? Anything I can do to help?

Comment 12

[Tanuja]

Botond, what are parent and child scrollable elements? What do you mean by C shows an overscroll effect? How can one scroll past the end?

Comment 13

[Botond Ballo [:botond]]

(In reply to Tanuja from comment #12)
> Botond, what are parent and child scrollable elements? 

When two scrollable elements are nested inside one another, we refer to the outer one as the "parent" and the inner one as the "child".

For example:

  <div id="foo" style="overflow:scroll">
    <!-- Some content here -->
    <div id="bar" style="overflow:scroll">
      <!-- Some more content here -->
    </div>
  </div>

Here, there are two scrollable elements: the "foo" div, and the "bar" div. The "bar" div is inside the "foo" div, so they are nested; "foo" is the parent and "bar" is the child.

> What do you mean by C shows an overscroll effect? How can one scroll past the end?

When a scrollable element has been scrolled as far as it can in a given direction, and the user performs a gesture that tries to scroll it even further ("past the end"), a visual effect is shown.

(The effect is specific to the platform. On macOS, the content continues to move as if scrolling, revealing blank space in the exposed area. On Android, a blue glow is shown (as in other apps). On our discontinued Firefox OS platform, there was a bounce effect. On Windows and Linux there is currently no visual effect.)

Comment 14

[Botond Ballo [:botond]]

Unassigning as Tanuja has indicated to me that she will not be able to finish working on this.

The bug is available for someone else to take.

Comment 15

[Gregory Moore [:gmoore]]

I am interested in working on this if no one else currently is. It looks like there's probably enough information to get started, but let me know if there's anything else I should know.

Also, I don't have access to a touchscreen device, so if that's a problem, please let me know. Thanks.

Comment 16

[Botond Ballo [:botond]]

Hi Gregory, thanks for your interest! The bug is available for you to work on.

The first step is to get a working Firefox build [1]. See also [2] for running GTests once you have a build, since this bug will involve that.

Other than that, I think comment 4 should be enough to get you started. My suggestion is to read some of the existing testcases in TestScrollHandoff.cpp, before trying to write the new one.

Access to a touchscreen is not necessary - you can run the GTests using a regular desktop build.

Let me know if you have any questions / run into any issues!

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/GTest

Comment 17

[Gregory Moore [:gmoore]]

Okay thanks, sounds good. I've used GTest a little before for some other bugs. 

I looked into this a little more, and I have a bunch of questions. There's a lot of different functions that the GTests use, and I don't really understand what a lot of them do. 

From looking at the other tests, it seems the first thing they do is to call Create*LayerTree*(), which seems like it sets up the HTML/CSS for us to test. I'm not sure which of these functions we should use though, or if we should write a new function for this case. We need a parent and a child, so it seems like maybe CreateScrollHandoffLayerTree1() would work? It has a layer tree syntax of "c(t)" (where c is the parent, and t is the child? Not exactly sure what c and t mean... ).

I also don't know how we can set it up so the parent can only scroll horizontally, and the child only vertically either. Is that something we should set within Create*LayerTree*()? Or is it just determined by the size of the CSSRect()? I also see this getting called a lot at the beginning of tests:
> // Enable overscrolling.
> SCOPED_GFX_PREF(APZOverscrollEnabled, bool, true);
Is there a different preference that we can change for horizontal/vertical scrolling for the parent and child. 

The next step would be to overscroll C vertically. I'm not exactly how we should do this though. Should we use the Pan() function? If so, what should the parameters be to ensure that we overscroll? Or should we use the TouchUp()/TouchDown() functions?

And lastly, we need to verify that the visual, vertical overscroll effect is shown for C. From the other test cases I'm not exactly sure how to do this either though. It seems that when using the TestAsyncPanZoomController class we can test for things like IsOverscrolled() or AssertStateIsFling(). Is there a function or series of functions we could use to test that the overscroll effect is shown? One of the possible values for TestAsyncPanZoomController::mState is OVERSCROLL_ANIMATION, but I'm not sure what a clean way to test if were in that state would be. Thanks.

Comment 18

[Botond Ballo [:botond]]

Hey Gregory, these are great questions!

> From looking at the other tests, it seems the first thing they do is to call
> Create*LayerTree*(), which seems like it sets up the HTML/CSS for us to
> test. 

The tests do not create HTML/CSS, because they are not testing the entire browser rendering pipeline.

The rendering pipeline is something like this:

   HTML
-> DOM
-> Frame tree
-> Display lists
-> Layer tree
-> Final composited image

Async panning/zooming (APZ) operates on the Layer tree, translating and scaling layers in response to user input.

Since these GTests are unit-testing APZ, they create a "mock" Layer tree directly as input to the tests.

> I'm not sure which of these functions we should use though, or if we
> should write a new function for this case. We need a parent and a child, so
> it seems like maybe CreateScrollHandoffLayerTree1() would work? 

The general idea is to use an existing one if an existing one suits your needs for the new test, otherwise write a new one.

> It has a
> layer tree syntax of "c(t)" (where c is the parent, and t is the child? Not
> exactly sure what c and t mean... ).

Yeah, this is a bit obscure and not well documented. 'c' stands for "container layer", which is a layer that contains other layers. 't' stands for "thebes layer", which is an old name for what we now call "painted layers" - these are the layers that actually contain rendered pixels, and they are leaves in the layer tree.

At some point only container layers were allowed to be scrollable, but currently all layers are allowed to be scrollable, so a "c(t)" syntax is sufficient for this test.

> I also don't know how we can set it up so the parent can only scroll
> horizontally, and the child only vertically either. Is that something we
> should set within Create*LayerTree*()? Or is it just determined by the size
> of the CSSRect()? 

Whether a layer is scrollable in a given direction is determined by the size of the layer's content, and the size of the layer's bounds (the "window" through which you view the content): if the content is larger than the bounds in a given direction, the layer is scrollable in that direction.

Both of these sizes are determined by Create*LayerTree*(): the content size by the third argument to the SetScrollableFrameMetrics() call, and the bounds by the visible region passed to CreateLayerTree().

So, for example, for CreateScrollHandoffLayerTree1(), the root layer (the "c") has a content size of 200x200, and bounds of 100x100, so it's scrollable in both directions (and as a result, CreateScrollHandoffLayerTree1() is not suitable for our test).

> I also see this getting called a lot at the beginning of
> tests:
> > // Enable overscrolling.
> > SCOPED_GFX_PREF(APZOverscrollEnabled, bool, true);
> Is there a different preference that we can change for horizontal/vertical
> scrolling for the parent and child.

Since our test concerns overscrolling, we will want to enable overscrolling using a statement of this form.

For horizontal/vertical scrolling, see above.

> The next step would be to overscroll C vertically. I'm not exactly how we
> should do this though. Should we use the Pan() function? If so, what should
> the parameters be to ensure that we overscroll? Or should we use the
> TouchUp()/TouchDown() functions?

The initial scroll offset will be (0,0), so if you want to overscroll at the top, any amount of scrolling should do it. If you want to overscroll at the bottom, you'd need to scroll by more than the difference between the content height and the layer bounds' height (200 - 100 = 100 in the above example).

The Pan() function is the easiest way to scroll. (TouchDown()/TouchUp() are lower-level functions that Pan() calls. A few tests use them directly, but this one shouldn't need to.)

> And lastly, we need to verify that the visual, vertical overscroll effect is
> shown for C. From the other test cases I'm not exactly sure how to do this
> either though. It seems that when using the TestAsyncPanZoomController class
> we can test for things like IsOverscrolled() or AssertStateIsFling(). Is
> there a function or series of functions we could use to test that the
> overscroll effect is shown? One of the possible values for
> TestAsyncPanZoomController::mState is OVERSCROLL_ANIMATION, but I'm not sure
> what a clean way to test if were in that state would be. Thanks.

Testing IsOverscrolled() is sufficient, since the bug that this test is for involved not being overscrolled in the relevant scenario.

Comment 19

[Gregory Moore [:gmoore]]

Attachment: Initial version of patch.

Thanks for the quick and very detailed response! It was very helpful. I think I understand most of what I need to know now. I've created a patch with the new test in it. It compiles and passes, but I'm not sure about a few things. 

(In reply to Botond Ballo [:botond] from comment #18)
> The initial scroll offset will be (0,0), so if you want to overscroll at the
> top, any amount of scrolling should do it. If you want to overscroll at the
> bottom, you'd need to scroll by more than the difference between the content
> height and the layer bounds' height (200 - 100 = 100 in the above example).
I'm a little confused by this. If we are doing a pan starting at y = 50, ending at y = 60, wouldn't that be a pan down?
> Pan(childApzc, 50, 60);
Why does this overscroll at the top? And why does:
> Pan(childApzc, 120, 10);
overscroll at the bottom? It seems like that's backwards. I initially had it the opposite way but it would fail the test. Maybe I just don't understand what the arguments to Pan() represent though.

I wrote tests for overscrolling at the bottom and the top, but if only one case is necessary I can remove one of them. I'm also not sure if we need to assert that the parent hasn't overscrolled. Let me know if there are other problems or things to change. Thanks.

Comment 20

[Botond Ballo [:botond]]

(In reply to Gregory Moore [:gmoore] from comment #19)
> I've created a patch with the new test in it. It compiles and passes,

I applied your patch, and the second |childApzc->IsOverscrolled()| assertion is failing for me.

Were you testing with something like |./mach gtest APZScrollHandoffTester.NestedAPZCOverscrollHandoff_Bug1201098|?

> (In reply to Botond Ballo [:botond] from comment #18)
> > The initial scroll offset will be (0,0), so if you want to overscroll at the
> > top, any amount of scrolling should do it. If you want to overscroll at the
> > bottom, you'd need to scroll by more than the difference between the content
> > height and the layer bounds' height (200 - 100 = 100 in the above example).
> I'm a little confused by this. If we are doing a pan starting at y = 50,
> ending at y = 60, wouldn't that be a pan down?
> > Pan(childApzc, 50, 60);
> Why does this overscroll at the top? And why does:
> > Pan(childApzc, 120, 10);
> overscroll at the bottom? It seems like that's backwards. I initially had it
> the opposite way but it would fail the test. Maybe I just don't understand
> what the arguments to Pan() represent though.

The arguments to this overload of Pan() represent the y-coordinates of the start and end of a pan gesture.

Pan gestures are used with touchscreens, which is the type of input that APZ was initially designed for (this is why most of the tests use them). The way they work is that content follows your finger. So, if you move your finger from y=50 to y=60, you are dragging the content down, which results in scrolling upwards (and thus overscrolling at the top if you hit the top edge). Likewise, if you move your finger from y=120 to y=10, you are dragging the content up, which results in scrolling downward (and thus overscrolling at the bottom if you hit the bottom edge).

Let me know if that makes sense.

Comment 21

[Gregory Moore [:gmoore]]

(In reply to Botond Ballo [:botond] from comment #20)
> I applied your patch, and the second |childApzc->IsOverscrolled()| assertion
> is failing for me.
> 
> Were you testing with something like |./mach gtest
> APZScrollHandoffTester.NestedAPZCOverscrollHandoff_Bug1201098|?

Hmmm, that's odd. Yeah, that's exactly how I was testing it. I rebased the patch (I hadn't done that in a while), but it's still passing on my machine.

Because the visible region is set up to only be 100x100, but the pan starts at y = 120, could that potentially be a problem? Because we don't start within the visible region? When I originally wrote that I thought that might not work, but it passed the test case so I thought it was fine. 

> The arguments to this overload of Pan() represent the y-coordinates of the
> start and end of a pan gesture.
> 
> Pan gestures are used with touchscreens, which is the type of input that APZ
> was initially designed for (this is why most of the tests use them). The way
> they work is that content follows your finger. So, if you move your finger
> from y=50 to y=60, you are dragging the content down, which results in
> scrolling upwards (and thus overscrolling at the top if you hit the top
> edge). Likewise, if you move your finger from y=120 to y=10, you are
> dragging the content up, which results in scrolling downward (and thus
> overscrolling at the bottom if you hit the bottom edge).
> 
> Let me know if that makes sense.

Oh okay, yeah that makes sense. Thanks for explaining that.

Comment 22

[Botond Ballo [:botond]]

(In reply to Gregory Moore [:gmoore] from comment #21)
> (In reply to Botond Ballo [:botond] from comment #20)
> > I applied your patch, and the second |childApzc->IsOverscrolled()| assertion
> > is failing for me.
> > 
> > Were you testing with something like |./mach gtest
> > APZScrollHandoffTester.NestedAPZCOverscrollHandoff_Bug1201098|?
> 
> Hmmm, that's odd. Yeah, that's exactly how I was testing it. I rebased the
> patch (I hadn't done that in a while), but it's still passing on my machine.

I looked into why it's failing for me, and discovered that there's actually a bug in the APZ code being tested, that was caught by the failure. I filed bug 1343775 for it. (It's a simple bug to fix; if you're interested, it would make a nice bug for you to work on after you're finished with this one.)

I'm not really sure why it's not failing for you. The only thing I can think of, is that the failure requires the "APZFlingMinVelocityThreshold" pref to have its default value of 0.5 (or something higher); perhaps for you for some reason it doesn't?

Can you try adding the following line:

  SCOPED_GFX_PREF(APZFlingMinVelocityThreshold, float, 0.5f);

at the top of the test, and see if it still passes for you?

> Because the visible region is set up to only be 100x100, but the pan starts
> at y = 120, could that potentially be a problem? Because we don't start
> within the visible region? When I originally wrote that I thought that might
> not work, but it passed the test case so I thought it was fine. 

The reason this isn't a problem is a bit subtle.

The Pan() function accepts as its first argument either the APZCTreeManager (which is stored in the 'manager' field of the test harness base object), in which case the events go through hit-testing to see which APZC they should target; or an APZC, in which case the events are sent directly to that APZC.

Since you're using the second version, no hit-testing occurs, and thus the values of coordinates relative to the visible rect don't really matter.

Comment 23

[Gregory Moore [:gmoore]]

(In reply to Botond Ballo [:botond] [standards meeting Feb 27 - Mar 4] from comment #22)
> I looked into why it's failing for me, and discovered that there's actually
> a bug in the APZ code being tested, that was caught by the failure. I filed
> bug 1343775 for it. (It's a simple bug to fix; if you're interested, it
> would make a nice bug for you to work on after you're finished with this
> one.)
Oh okay, I see. I am definitely interested in working on that bug after this one. :)

> I'm not really sure why it's not failing for you. The only thing I can think
> of, is that the failure requires the "APZFlingMinVelocityThreshold" pref to
> have its default value of 0.5 (or something higher); perhaps for you for
> some reason it doesn't?
> 
> Can you try adding the following line:
> 
>   SCOPED_GFX_PREF(APZFlingMinVelocityThreshold, float, 0.5f);
> 
> at the top of the test, and see if it still passes for you?
I tried adding that to the top of the test, and it still passed. Even if I set it really high (1000.0f) it would still pass. So I'm not really sure why it's passing for me either.

So I guess for now, how should we make sure the test will pass? Can we set APZFlingMinVelocityThreshold temporarily so it will pass on any machine/build? Or should we just fix bug 1343775 first? Alternatively, we could get rid of the second overscroll and assertions (and just test overscrolling past the top). Let me know, thanks.

Comment 24

[Botond Ballo [:botond]]

(In reply to Gregory Moore [:gmoore] from comment #23)
> So I guess for now, how should we make sure the test will pass? Can we set
> APZFlingMinVelocityThreshold temporarily so it will pass on any
> machine/build? Or should we just fix bug 1343775 first? Alternatively, we
> could get rid of the second overscroll and assertions (and just test
> overscrolling past the top). Let me know, thanks.

Yeah, I think it's sufficient to just test overscrolling past the top. Since the failure occurs during the second part (overscrolling psat the bottom), we can just remove that part.

> > I'm not really sure why it's not failing for you. The only thing I can think
> > of, is that the failure requires the "APZFlingMinVelocityThreshold" pref to
> > have its default value of 0.5 (or something higher); perhaps for you for
> > some reason it doesn't?
> > 
> > Can you try adding the following line:
> > 
> >   SCOPED_GFX_PREF(APZFlingMinVelocityThreshold, float, 0.5f);
> > 
> > at the top of the test, and see if it still passes for you?
> I tried adding that to the top of the test, and it still passed. Even if I
> set it really high (1000.0f) it would still pass. So I'm not really sure why
> it's passing for me either.

If you're interested in investigating further (I for one am still curious), you could check what the values of flingVelocity.x and flingVelocity.y are when you get to this point [1] (with the second part of the test still in place), either with a debugger, or by inserting a "printf" statement (we usually use "printf_stderr", which has the same interface as printf but prints to standard error, although for GTests I believe plain "printf" works as well.

[1] http://searchfox.org/mozilla-central/rev/1bc7720a434af3c13b68b8d69f92078cae8890c4/gfx/layers/apz/src/AsyncPanZoomController.cpp#1275

Comment 25

[Gregory Moore [:gmoore]]

Attachment: Removed second part of test for overscrolling past bottom.

(In reply to Botond Ballo [:botond] from comment #24)
> If you're interested in investigating further (I for one am still curious),
> you could check what the values of flingVelocity.x and flingVelocity.y are
> when you get to this point [1] (with the second part of the test still in
> place), either with a debugger, or by inserting a "printf" statement (we
> usually use "printf_stderr", which has the same interface as printf but
> prints to standard error, although for GTests I believe plain "printf" works
> as well.
Sure. I added printf() statements and the output looked like this:
> flingVelocity.x = 0.000000
> flingVelocity.y = -0.090000
> flingVelocity.x = 0.000000
> flingVelocity.y = 1.110000
The first two are when we overscroll past the top, and the second two are from when we overscroll past the bottom.

I updated the patch so it just tests for overscrolling past the top. Let me know if there are more changes we should make. Thanks.

Comment 26

[Botond Ballo [:botond]]

Comment on attachment 8844191 [details] [diff] [review]
Removed second part of test for overscrolling past bottom.

Review of attachment 8844191 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks good! 

Just one small request:

::: gfx/layers/apz/test/gtest/TestScrollHandoff.cpp
@@ +71,5 @@
>      registration = MakeUnique<ScopedLayerTreeRegistration>(manager, 0, root, mcc);
>      manager->UpdateHitTestingTree(0, root, false, 0, 0);
>    }
>  
> +  void CreateScrollHandoffLayerTree4() {

Please add a comment similar to:

  // Creates a layer tree with a parent layer that is scrollable
  // only horizontally, and a child layer that is scrollable
  // only vertically.

This way, people writing future tests can easily tell if this layer tree is suitable for them.

Comment 27

[Botond Ballo [:botond]]

(In reply to Gregory Moore [:gmoore] from comment #25)
> Sure. I added printf() statements and the output looked like this:
> > flingVelocity.x = 0.000000
> > flingVelocity.y = -0.090000

Interesting. This is what I see as well, and with these values, the magnitude of the fling velocity vector is less than the default minimum of 0.5, so the code at this point early-exits. 

For me, that results in the overscroll never being cleared during the call to AdvanceAnimationsUntilEnd(), because the mechanism for clearing it is a check at the end of the fling animation, which is never reached since we don't run the fling animation at all (this is what I filed bug 1343775 about).

I guess for you, the overscroll is getting cleared by some other means, though I'm not sure how exactly. Anyways, we can continue this investigation in bug 1343775, which will involve writing a test case that catches this scenario.

Comment 28

[Gregory Moore [:gmoore]]

Attachment: Added comment describing what CreateScrollHandoffLayerTree4() does.

Sorry for taking so long to get to this. I updated the patch with a comment.

(In reply to Botond Ballo [:botond] from comment #27)
> I guess for you, the overscroll is getting cleared by some other means,
> though I'm not sure how exactly. Anyways, we can continue this investigation
> in bug 1343775, which will involve writing a test case that catches this
> scenario.

Okay sure, sounds good. Thanks for all of the help with this bug.

Comment 29

[Botond Ballo [:botond]]

Pushed to the Try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=25e1835be995d88f7e8e8bdb198a461642d5f3db

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d70f85b48971
Added a GTest to ensure that a nested-APZC overscroll handoff with opposing constrained axes will work properly (fixed by bug 1201098). r=botond

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d70f85b48971