1230552 - [apz] Flinging subframe flings parent when subframe ends

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

If you fling a subframe, and it hits the end of that document, any remaining velocity is applied to the parent document and the fling continues (starts?) there. I hate it.

Comment 1

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

Probably gonna WONTFIX this but I guess we should UX decide.

Comment 2

[Botond Ballo [:botond]]

This was done on purpose back in bug 965871.

Comment 3

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #2)
> This was done on purpose back in bug 965871.

Though, to be fair, the primary motivation at the time was B2G, where the structure of the dynamic toolbar meant we needed this to be able to fling the page at all.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Anthony, need some UX guidance here

My problem with the current behavior is that when you're flinging, you can easily hit the end of the frame and then it's quickly not even on the screen. I think it should be prefable if you need it for the b2g browser.

Comment 5

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

snorp, can you also provide some sample URLs where you ran into this issue? Might make it easier for Anthony to try it out and decide.

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Try this: https://bluemarvin.github.io/html/divscroll.html

You'll need to zoom in, though.

Comment 7

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Thanks!

Yeah this is weird - I don't think I've really seen other apps do this before. 

If B2G needs this, we could make it prefable as snorp said, but I don't think this should be the behaviour of Fennec right now.

Comment 8

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

What about panning with the finger down (as opposed to flinging, where the finger is lifted and scrolling continues)? Fennec with the old JPZC code wouldn't hand that off either (you can see the behaviour in Aurora/Beta/Release). Do we want to restore that behaviour as well?

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> What about panning with the finger down (as opposed to flinging, where the
> finger is lifted and scrolling continues)? Fennec with the old JPZC code
> wouldn't hand that off either (you can see the behaviour in
> Aurora/Beta/Release). Do we want to restore that behaviour as well?

I personally don't care so much about that case. Anthony?

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Add a pref for APZ fling handoff, and only enable for B2G

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

That patch is probably wrong, and misses some other cases, but does 'fix' it. I'm working on a better version.

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Add preference for APZ overscroll handoff and disable everywhere except B2G

I like this patch better, but not sure if it will end up breaking a bunch of other stuff.

Comment 13

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

Comment on attachment 8696083 [details] [diff] [review]
Add preference for APZ overscroll handoff and disable everywhere except B2G

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

Botond should review the actual change.

::: modules/libpref/init/all.js
@@ +606,5 @@
>  pref("apz.y_skate_highmem_adjust", "2.5");
>  #else
>  // Mobile prefs
> +#if defined(MOZ_WIDGET_GONK)
> +pref("apz.fling_allow_handoff", true);

You should default this to true and make it false for fennec in mobile/android/app/mobile.js

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

Chrome's behavior is to disallow panning/fling handoff unless you are at the edge of the document. That seems sensible to me, so let's try to do that.

Comment 15

[Botond Ballo [:botond]]

Comment on attachment 8696083 [details] [diff] [review]
Add preference for APZ overscroll handoff and disable everywhere except B2G

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

As discussed on IRC, this would disable handoff completely, even in cases where the child scroll frame is already scrolled to its end, but we do want handoff to work in that case.

I had suggested trying to condition handoff in AttemptScroll() on whether the current scroll frame has been scrolled by a nonzero amount, but I since realized that wouldn't do what we want, because that would make the decision per-{touch move}, so we could still get handoff in the middle of a single pan gesture. We need the decision to be per-{input block} instead.

I also realized that we need fling handoff to work in the above scenario, too, because if you start a pan from a child scroll frame scrolled to its end and scroll its parent, when you let go the fling still starts from the child's APZC, so we need that to be handed off to the parent.

Comment 16

[Botond Ballo [:botond]]

Here's an outline of how this could be done:

  - Have each input block store a "scrolled APZC" property which represents 
    the first APZC to actually scroll for that input block.

  - The first time an APZC is actually scrolled during an input block,
    set the "scrolled APZC" property on the input block.

  - Add a pref "apz.allow_immediate_handoff" which would be true on B2G and
    false on Fennec.

  - If the pref is not set, prevent handoff _beyond_ the the input block's 
    "scrolled APZC".

This would ensure that on setups where the pref is not set, a single input block only scrolls a single APZC.

Comment 17

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #9)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> > What about panning with the finger down (as opposed to flinging, where the
> > finger is lifted and scrolling continues)? Fennec with the old JPZC code
> > wouldn't hand that off either (you can see the behaviour in
> > Aurora/Beta/Release). Do we want to restore that behaviour as well?
> 
> I personally don't care so much about that case. Anthony?

Not quite following, but did we cover this in our session this week Snorp? If not, whats the best way to test this?

Comment 18

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Anthony Lam (:antlam) from comment #17)
> 
> Not quite following, but did we cover this in our session this week Snorp?
> If not, whats the best way to test this?

I think we showed you and talked about it, yeah. The idea here is that a single fling will only scroll *one* frame. If you get to the end of a subframe and fling again, it will allow a handoff at the beginning.

Comment 19

[Botond Ballo [:botond]]

Attachment: Disallow immediate scroll handoff on Fennec

I had some spare time today so I put together the beginnings of a patch that implements the strategy outlined in comment 16. It's not tested and doesn't handle flings, but it can be used as a starting point for further work.

Comment 20

[Botond Ballo [:botond]]

Ended up finishing this on the plane. Still haven't tested it on a device, but I will before landing.

Comment 21

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1230552 - Update some out-of-date comments and remove an old #undef. r=kats

Bug 1230552 - Update some out-of-date comments and remove an old #undef. r=kats

Comment 22

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1230552 - Const-correctness improvements. r=kats

Bug 1230552 - Const-correctness improvements. r=kats

Comment 23

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1230552 - Introduce a helper AsyncPanZoomController::CurrentInputBlock(). r=kats

Bug 1230552 - Introduce a helper AsyncPanZoomController::CurrentInputBlock(). r=kats

Comment 24

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1230552 - Make immediate scroll handoff for panning prefable. r=kats

Bug 1230552 - Make immediate scroll handoff for panning prefable. r=kats

Immediate handoff is the current behaviour. The alternative is to
only allow a single input block to scroll a single APZC.

Comment 25

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1230552 - Introduce a structure to group fling handoff state. r=kats

Bug 1230552 - Introduce a structure to group fling handoff state. r=kats

This patch also cleans up APZCTreeManager::DispatchFling() a bit.

Comment 26

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1230552 - Extend the immediate scroll handoff pref to apply to flings. r=kats

Bug 1230552 - Extend the immediate scroll handoff pref to apply to flings. r=kats

Comment 27

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1230552 - Disallow immediate scroll handoff on Fennec. r=kats

Bug 1230552 - Disallow immediate scroll handoff on Fennec. r=kats

Comment 28

[James Willcox (:snorp) ([e-mail]) (he/him)]

Nice, thanks for handling this Botond.

Comment 29

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

Comment on attachment 8697798 [details]
MozReview Request: Bug 1230552 - Update some out-of-date comments and remove an old #undef. r=kats

https://reviewboard.mozilla.org/r/27713/#review24955

Comment 30

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

Comment on attachment 8697799 [details]
MozReview Request: Bug 1230552 - Const-correctness improvements. r=kats

https://reviewboard.mozilla.org/r/27715/#review24957

Comment 31

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

Comment on attachment 8697800 [details]
MozReview Request: Bug 1230552 - Introduce a helper AsyncPanZoomController::CurrentInputBlock(). r=kats

https://reviewboard.mozilla.org/r/27717/#review24959

Comment 32

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

Comment on attachment 8697801 [details]
MozReview Request: Bug 1230552 - Make immediate scroll handoff for panning prefable. r=kats

https://reviewboard.mozilla.org/r/27719/#review24961

::: gfx/layers/apz/src/InputBlockState.h:52
(Diff revision 1)
> +    mScrolledApzc = aApzc;

I'd prefer having these function bodies in the .cpp file even though they're trivial. Also would it make sense to MOZ_ASSERT(!mScrolledApzc) at the top of the setter function?

Comment 33

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

Comment on attachment 8697802 [details]
MozReview Request: Bug 1230552 - Introduce a structure to group fling handoff state. r=kats

https://reviewboard.mozilla.org/r/27721/#review24965

Nice!

Comment 34

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

Comment on attachment 8697803 [details]
MozReview Request: Bug 1230552 - Extend the immediate scroll handoff pref to apply to flings. r=kats

https://reviewboard.mozilla.org/r/27723/#review24967

Comment 35

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

Comment on attachment 8697804 [details]
MozReview Request: Bug 1230552 - Disallow immediate scroll handoff on Fennec. r=kats

https://reviewboard.mozilla.org/r/27725/#review24969

Nice set of patches, thanks for splitting them up so cleanly!

::: mobile/android/app/mobile.js:574
(Diff revision 1)
> +pref("apz.allow_immediate_handoff", false);

Move this up into the MOZ_ANDROID_APZ block. It doesn't really matter now but eventually we'll remove that #ifdef and use the prefs unconditionally, and if you move it now they'll stay alpha-ordered.

Comment 36

[Botond Ballo [:botond]]

I addressed review comments locally, and tested this on a device. Seems to be working as intended!

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe18b8f63cd6
https://hg.mozilla.org/integration/mozilla-inbound/rev/b298c61ba235
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef389501eca7
https://hg.mozilla.org/integration/mozilla-inbound/rev/c408f06c47c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/155f1d5d8d95
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d4007900be0
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9aecd134e38

Comment 38

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #32)
> Also would it make sense to MOZ_ASSERT(!mScrolledApzc) at the top of the setter function?

I just realized, I forgot about this comment.

However, I don't think we can do this. We call the setter on every touch-move event that scrolls by a non-zero amount, so subsequent touch-moves in a single input block would trip it.

Comment 39

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c7bebd09158

Comment 40

[Botond Ballo [:botond]]

(In reply to Pulsebot from comment #39)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/1c7bebd09158

This is a small follow-up to change an EXPECT_EQ in the gtest to an ASSERT_NEAR, to account for a fact that a fling can end leaving an scroll frame within COORDINATE_EPSILON of the end of its scroll range, rather than right at the end.

(The assertion was passing for me locally, but failing in automation.)

Comment 41

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

(In reply to Botond Ballo [:botond] from comment #38)
> 
> However, I don't think we can do this. We call the setter on every
> touch-move event that scrolls by a non-zero amount, so subsequent
> touch-moves in a single input block would trip it.

How about MOZ_ASSERT(!mScrolledApzc || mScrolledApzc == aApzc)?

Comment 42

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #41)
> (In reply to Botond Ballo [:botond] from comment #38)
> > 
> > However, I don't think we can do this. We call the setter on every
> > touch-move event that scrolls by a non-zero amount, so subsequent
> > touch-moves in a single input block would trip it.
> 
> How about MOZ_ASSERT(!mScrolledApzc || mScrolledApzc == aApzc)?

That should work. Thanks for suggesting it!

Added it in a follow-up:

https://hg.mozilla.org/integration/mozilla-inbound/rev/08dca8061836

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fe18b8f63cd6
https://hg.mozilla.org/mozilla-central/rev/b298c61ba235
https://hg.mozilla.org/mozilla-central/rev/ef389501eca7
https://hg.mozilla.org/mozilla-central/rev/c408f06c47c4
https://hg.mozilla.org/mozilla-central/rev/155f1d5d8d95
https://hg.mozilla.org/mozilla-central/rev/2d4007900be0
https://hg.mozilla.org/mozilla-central/rev/e9aecd134e38
https://hg.mozilla.org/mozilla-central/rev/1c7bebd09158
https://hg.mozilla.org/mozilla-central/rev/08dca8061836