898478 - Provide some seamless scrolling mechanism in multi-APZC for B2G dynamic toolbar work

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

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

With bug 886232 in a relatively stable state and close to landing (hopefully) we should start looking at what the dynamic toolbar on B2G needs in order to get unblocked. The primary requirement as I understand it is to get "seamless scrolling" (i.e. when you reach the end of a scroll frame, it continues to the parent scrollframe).

However for the dynamic toolbar it's a little more complicated than just that because you want to be able to scroll the dynamic toolbar into view when scrolling upwards anywhere on the page, so it's not a strict "use the innermost scrollframe first" thing. We'll need some additional policy and architecture to do this in a generic enough way.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #0)
> With bug 886232 in a relatively stable state and close to landing
> (hopefully) we should start looking at what the dynamic toolbar on B2G needs
> in order to get unblocked. The primary requirement as I understand it is to
> get "seamless scrolling" (i.e. when you reach the end of a scroll frame, it
> continues to the parent scrollframe).
> 
> However for the dynamic toolbar it's a little more complicated than just
> that because you want to be able to scroll the dynamic toolbar into view
> when scrolling upwards anywhere on the page, so it's not a strict "use the
> innermost scrollframe first" thing. We'll need some additional policy and
> architecture to do this in a generic enough way.

For a first cut, we can do something "manually" in js to handle bringing the url bar into view with the content not scrolled all the way to the top.

Comment 2

[Chris Lord [:cwiiis]]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #1)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #0)
> > With bug 886232 in a relatively stable state and close to landing
> > (hopefully) we should start looking at what the dynamic toolbar on B2G needs
> > in order to get unblocked. The primary requirement as I understand it is to
> > get "seamless scrolling" (i.e. when you reach the end of a scroll frame, it
> > continues to the parent scrollframe).
> > 
> > However for the dynamic toolbar it's a little more complicated than just
> > that because you want to be able to scroll the dynamic toolbar into view
> > when scrolling upwards anywhere on the page, so it's not a strict "use the
> > innermost scrollframe first" thing. We'll need some additional policy and
> > architecture to do this in a generic enough way.
> 
> For a first cut, we can do something "manually" in js to handle bringing the
> url bar into view with the content not scrolled all the way to the top.

This isn't going to cut it for going in the reverse direction - what you describe is basically what happens at the moment (the header is programmatically scrolled in/out based on page scroll position). i.e. this complication applies to this situation regardless of page scroll position.

Comment 3

[Botond Ballo [:botond]]

Kats and I discussed this today, and came up with the following plan:

  1. Introduce a mechanism where, if an APZC that receives a touch event
     causing a scroll does not "consume" the entire scroll (because it's 
     already scrolled as far as possible in the relevant direction), 
     the APZCTreeManager hands off the remainder of the scroll to 
     another APZC. By default, this other APZC would be the parent of 
     the first one.

  2. Create an APZC for the <div> that contains the toolbar and the browser
     iframe. This <div> currently exists in the parent process (this will
     change when dzbarsky's patches for making the browser app have its own
     process land, but according to jlebar in IRC, that won't happen in 26,
     and we would like this bug to be fixed in 26, so we'll have to deal with
     this). Currently we don't have APZCs for things in the parent process,
     and introducing them for everything in the parent process right away 
     may be too big of a disruption, so we are proposing to create an APZC
     *only* for that <div>, for now.

  3. Introduce some special handling of browser iframe elements by APZC(TM).
     
       a. When deciding on the initial target of a scroll, we currently use 
          the APZC for innermost scrollable layer that passes the hit test.
          We'll need to change this so that, if there is a browser iframe
          element on the path from the root scrollable layer to the
          innermost one, the initial target is the APZC for the innermost 
          scrollable layer *outside* of the browser iframe. This will ensure 
          that the toolbar will be scrolled on- or off-screen before 
          anything else.

       b. When deciding which APZC to hand off scrolling to (see point (1)),
          if the initial APZC was the one just outside the browser iframe
          (as per (3a)), the next one should be the one for the innermost
          scrollable layer inside the browser iframe.

Any feedback on this plan is welcome.

I suggest we use this bug for (1), and file sibling bugs for (2) and (3).

I will start looking at (1).

Comment 4

[Botond Ballo [:botond]]

Attachment: Part 1 - Have APZC keep a pointer to the APZCTM

Comment 5

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

Comment on attachment 793146 [details] [diff] [review]
Part 1 - Have APZC keep a pointer to the APZCTM

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

Looks ok but I'll hold off on giving the r+ until I see the rest of the patches.

Comment 6

[Botond Ballo [:botond]]

Attachment: [WIP] Part 2 - Have APZCs pass overscroll caused by panning on to their parents

This is a work-in-progress patch for having an APZC hand off scrolling to its parent if it can't scroll itself any further.

The patch is full of debugging statements and not suitable for review yet. I wanted to post it because I will put my work on this bug on hold for a bit while I work on displayport issues. The patch does compile, and it partially works.

The patch implements the hand-off of any overscroll (portion of requested scroll amount that cannot be consumed by a given layer) from one APZC to its parent. It has a problem, which is that it breaks an assumption currently made my APZC, which is that while an APZC L is processing an input event block, the processing of the events does not change the transform T that goes from real screen coordinates to L's screen coordinates.

This assumption allows an APZC to store the previous touch point in its screen coordinates (i.e. transformed by T) and receive a new touch point also in its screen coordinates (i.e. transformed by T), and get a meaningful result when it subtracts the two (it then uses this difference to determine the amount by which to scroll).

If T changes in between being used to transform the previous touch point and being used to transform the current touch point, however, then this subtraction becomes meaningless.

As long as touch events sent to L scroll only L, T does not change in response to the events, because scrolling L changes L's async transform, but T only depends on the async transforms of L's ancestors.

With hand-off, touch events sent to L can end up scrolling ancestors of L, and thus changing T, so the assumption stated above no longer holds and Bad Things (tm) happen (scrolling that involves hand-off becomes inaccurate and jumpy).

A possible solution is to cache the transform T that is used to transform touch points in an input event block, when handling the first event in the block, and use the cached transform for subsequent events in the block. This would make the assumption stated above hold again. If we do this, we have to be careful to make sure that the transform used when passing touch points to Gecko (http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/APZCTreeManager.cpp#313) remains correct. One approach is to use the cached transformToApzc transform to pass to the APZC, but the up-to-date transformToApzc and transformToScreen to pass to Gecko.

Comment 7

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #3)
> Kats and I discussed this today, and came up with the following plan:
> 
> [...]
> 
>   2. Create an APZC for the <div> that contains the toolbar and the browser
>      iframe. This <div> currently exists in the parent process (this will
>      change when dzbarsky's patches for making the browser app have its own
>      process land, but according to jlebar in IRC, that won't happen in 26,
>      and we would like this bug to be fixed in 26, so we'll have to deal with
>      this). Currently we don't have APZCs for things in the parent process,
>      and introducing them for everything in the parent process right away 
>      may be too big of a disruption, so we are proposing to create an APZC
>      *only* for that <div>, for now.
> 
> [...]
> 
> I suggest we use this bug for (1), and file sibling bugs for (2) and (3).

I filed bug 912657 for (2).

Comment 8

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #3)
> Kats and I discussed this today, and came up with the following plan:
> 
> [...]
> 
>   3. Introduce some special handling of browser iframe elements by APZC(TM).
>      
>        a. When deciding on the initial target of a scroll, we currently use 
>           the APZC for innermost scrollable layer that passes the hit test.
>           We'll need to change this so that, if there is a browser iframe
>           element on the path from the root scrollable layer to the
>           innermost one, the initial target is the APZC for the innermost 
>           scrollable layer *outside* of the browser iframe. This will ensure 
>           that the toolbar will be scrolled on- or off-screen before 
>           anything else.
> 
>        b. When deciding which APZC to hand off scrolling to (see point (1)),
>           if the initial APZC was the one just outside the browser iframe
>           (as per (3a)), the next one should be the one for the innermost
>           scrollable layer inside the browser iframe.
> 
> [...]
> 
> I suggest we use this bug for (1), and file sibling bugs for (2) and (3).

I filed bug 912666 for (3).

Comment 9

[Botond Ballo [:botond]]

Attachment: Part 2 - Have APZCs pass overscroll caused by panning on to their parents

Updated patch to clean it up (remove debugging statements) and implement the workaround for the remaining problem that was described in comment #6.

I can't actually test this right now, so I'm posting it for f+ for now.

Comment 10

[Botond Ballo [:botond]]

Tested updated patch on B2G - looks good. Marking patches for review.

Comment 11

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

Comment on attachment 801869 [details] [diff] [review]
Part 2 - Have APZCs pass overscroll caused by panning on to their parents

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

This looks ok to me. The only thing I'm concerned about is what happened to the timeDelta that used to be passed into Axis. I understand the code reorg but it feels like there's still a timeDelta factor missing in APZC::TrackTouch.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +731,5 @@
> +  {
> +    ReentrantMonitorAutoEnter lock(mMonitor);
> +
> +    // TODO(botond): used to be CalculateResolution() - make sure this is OK
> +    CSSToScreenScale resolution = mFrameMetrics.mZoom;

Remove the TODO here, I believe the code is correct. Although I would rename the local variable to zoom, since resolution means something else in FrameMetrics.

@@ +763,5 @@
>  void AsyncPanZoomController::TrackTouch(const MultiTouchInput& aEvent) {
> +  SingleTouchData& touch = GetFirstSingleTouch(aEvent);
> +  ScreenIntPoint prevTouchPoint(mX.GetPos(), mY.GetPos());
> +  ScreenIntPoint touchPoint = touch.mScreenPoint;
> +  ScreenRect screenViewport = mFrameMetrics.mViewport * mFrameMetrics.mZoom;

screenViewport appears unused

@@ +805,4 @@
>  
> +  // Inversely scale the offset by the resolution (when you're zoomed further in,
> +  // a larger swipe should move you a shorter distance).
> +  // TODO(botond): used to be CalculateResolution() - make sure this is OK

ditto on the TODO

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +15,5 @@
>  #include "InputData.h"
>  #include "Axis.h"
>  #include "TaskThrottler.h"
>  #include "gfx3DMatrix.h"
> +#include "nsEvent.h"

Why this include?

Comment 12

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> Comment on attachment 801869 [details] [diff] [review]
> Part 2 - Have APZCs pass overscroll caused by panning on to their parents
> 
> The only thing I'm concerned about is what happened to
> the timeDelta that used to be passed into Axis. I understand the code reorg
> but it feels like there's still a timeDelta factor missing in
> APZC::TrackTouch.

I actually find the revised code easier to understand. 

Here's what we did before:
  1. In Axis::UpdateWithTouchAtDevicePoint(), set Axis::mVelocity to be the
     displacement divided by timeDelta.
  2. In Axis::GetDisplacementForDuration(), multiply Axis::mVelocity by
     timeDelta to get back the displacement.

That seemed a bit ciruitous to me. Since we already have the displacement,
we might as well use it directly in (2) - that's why I replaced
Axis::GetDisplacementForDuration() with Axis::AdjustDisplacement(),
which is basically the same thing but without recomputing the displacement.

I am open to suggestions for improvement.


> ::: gfx/layers/ipc/AsyncPanZoomController.h
> @@ +15,5 @@
> >  #include "InputData.h"
> >  #include "Axis.h"
> >  #include "TaskThrottler.h"
> >  #include "gfx3DMatrix.h"
> > +#include "nsEvent.h"
> 
> Why this include?

nsEvent.h defines 'enum nsEventStatus', which is referenced in this file (return value of ReceiveInputEvent()).

Before, nsEvent.h was included indirectly through APZCTreeManager.h, but the Part 1 patch removed that include.

Comment 13

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

(In reply to Botond Ballo [:botond] from comment #12)
> I actually find the revised code easier to understand. 
> 
> Here's what we did before:
>   1. In Axis::UpdateWithTouchAtDevicePoint(), set Axis::mVelocity to be the
>      displacement divided by timeDelta.
>   2. In Axis::GetDisplacementForDuration(), multiply Axis::mVelocity by
>      timeDelta to get back the displacement.
> 
> That seemed a bit ciruitous to me. Since we already have the displacement,
> we might as well use it directly in (2) - that's why I replaced
> Axis::GetDisplacementForDuration() with Axis::AdjustDisplacement(),
> which is basically the same thing but without recomputing the displacement.
> 
> I am open to suggestions for improvement.
> 

Ah ok, this makes sense. I didn't read through the parts of the code not included in the diff when I made my earlier comment so I didn't realize we were doing the same calculation multiple times.

> nsEvent.h defines 'enum nsEventStatus', which is referenced in this file
> (return value of ReceiveInputEvent()).
> 
> Before, nsEvent.h was included indirectly through APZCTreeManager.h, but the
> Part 1 patch removed that include.

Ok. Looks good to me.

Comment 14

[Botond Ballo [:botond]]

Attachment: Part 2 - Have APZCs pass overscroll caused by panning on to their parents

Updated part 2 patch to address kats' review comments.

Comment 15

[Botond Ballo [:botond]]

Try job: https://tbpl.mozilla.org/?tree=Try&rev=40435594a8b3

Comment 16

[Botond Ballo [:botond]]

Attachment: bug898478-part2.patch

Updated part 2 patch to fix an Android compiler error revealed by the try push. The fix is just adding a missing include. Carrying r+.

New try job: https://tbpl.mozilla.org/?tree=Try&rev=1c3069f3eec2

Comment 17

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #15)
> Try job: https://tbpl.mozilla.org/?tree=Try&rev=40435594a8b3

The try run is showing a number of test failures on B2G and one on windows 7. The B2G ones are the same as the ones for bug 914825's try push, but the windows 7 one seems to be particular to this bug. Will investigate before going ahead with the patch.

Comment 18

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #17)
> (In reply to Botond Ballo [:botond] from comment #15)
> > Try job: https://tbpl.mozilla.org/?tree=Try&rev=40435594a8b3
> 
> The try run is showing a number of test failures on B2G and one on windows
> 7. The B2G ones are the same as the ones for bug 914825's try push, but the
> windows 7 one seems to be particular to this bug. Will investigate before
> going ahead with the patch.

The bug 914825 failures were fixed by simply pulling the latest code and rebasing the patch. I'm hoping the same will fix the failure for this.

Trying again: https://tbpl.mozilla.org/?tree=Try&rev=ee9de3ee0f9a

Comment 19

[Botond Ballo [:botond]]

https://hg.mozilla.org/integration/b2g-inbound/rev/da6ef0250d8e
https://hg.mozilla.org/integration/b2g-inbound/rev/e669f4a0235a

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/da6ef0250d8e
https://hg.mozilla.org/mozilla-central/rev/e669f4a0235a

Comment 21

[Botond Ballo [:botond]]

Comment on attachment 793146 [details] [diff] [review]
Part 1 - Have APZC keep a pointer to the APZCTM

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: Bug 867787 (which is koi?) remains unfixed.
Testing completed (on m-c, etc.): locally, on m-c since Tuesday
Risk to taking this patch (and alternatives if risky): Medium. May introduce unexpected side effects with scrolling behaviour with nested scrollable items (something we don't handle very well to begin with); based on experience, this code tends to be a little brittle.
String or IDL/UUID changes made by this patch: none

Comment 22

[Botond Ballo [:botond]]

Comment on attachment 803842 [details] [diff] [review]
bug898478-part2.patch

[Approval Request Comment]
See previous patch - the two only make sense together.

Comment 24

[Julien Wajsberg [:julienw]]

dupe bug 867787 is koi+

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 793146 [details] [diff] [review]
Part 1 - Have APZC keep a pointer to the APZCTM

koi+ have automatic approval

Comment 26

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

Note that there is a medium level of risk associated with uplifting these patches. A blanket uplift policy simplifies some things but it would be nice to have the release drivers manually approve this uplift.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8977ee4184dc
https://hg.mozilla.org/releases/mozilla-aurora/rev/5e11a411141e

Comment 28

[Ryan VanderMeulen [:RyanVM]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #26)
> Note that there is a medium level of risk associated with uplifting these
> patches. A blanket uplift policy simplifies some things but it would be nice
> to have the release drivers manually approve this uplift.

Bit late for that :) FWIW, why is this marked as a blocker if you aren't sure if this should land on 1.2 or not?

Comment 29

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

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #28)
> Bit late for that :) FWIW, why is this marked as a blocker if you aren't
> sure if this should land on 1.2 or not?

We discussed this a bit on IRC [1]. We'll leave this patch in and if there are regressions that turn up we can consider backing it out and maybe finding a lower-risk patch for it.

[1] http://logbot.glob.com.au/?c=mozilla%23b2g&s=23%20Sep%202013&e=23%20Sep%202013#c92407

Comment 30

[Milan Sreckovic [:milan] (needinfo for best results)]

Thanks.  If it is not a blocker of bug 919625, let's make sure we change that.  This was a primary reason I made this a blocker - it was blocking one.

Comment 31

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

To be clear: this fixes bug 867787 which is koi+. It does block bug 919625, which is also koi+. However I don't think 919625 needs to be koi+; it inherited this from bug 867787 which it was cloned from and is more severe. I'm going to request dropping koi+ from bug 919625 but *this* bug still should inherit koi+ from 867787, I think.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Funny story, I just remembered that for b2g18, we used the NO_UPLIFT whiteboard status to denote blockers that weren't ready for uplifting. And my bug queries are already setup to ignore such bugs. So rather than reinventing the wheel, let's just use that :)