1265510 - Mandatory scroll snapping can be interrupted by scrolling a nested scroll frame

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Markus Stange [:mstange]]

STR:
1. Load the attached testcase. It has two scrollable frames: one with a black border and one with a blue border. The blue frame is inside the black frame and not visible initially.
2. Inside the black frame, quickly scroll down, twice. The first scroll will scroll the black box, and the second should scroll the blue box which has moved under the mouse.

Scrolling the blue box interrupts the scroll snap of the black box. It shouldn't do that.

I don't know if this is a regression.

Comment 1

[Markus Stange [:mstange]]

Attachment: testcase

Comment 2

[Botond Ballo [:botond]]

I can't reproduce this with a mouse wheel. I'll try with touch events on a touchscreen laptop.

Comment 3

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #2)
> I'll try with touch events on a touchscreen laptop.

I can't reproduce it with touch events either. I guess the issue is specific to pan gesture events?

Comment 4

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1265510 - Don't start a scroll snap animation if we're already at the destination. r=kats

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

Comment 5

[Botond Ballo [:botond]]

Oh shoot. Wrong bug.

Comment 6

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

I see this on OS X Aurora as well. On 45 I can't really reproduce it because the inner scrollframe doesn't scroll until the outer one is snapped, it looks like the transaction is locked to the outer frame for longer.

Comment 7

[Botond Ballo [:botond]]

As this issue is specific to pan gesture events, it needs to be debugged on a Macbook. As mentioned in today's daily meeting, I can debug it if someone lets me borrow their Macbook, or I can guide someone with a Macbook through debugging it.

Comment 8

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

This is easy enough to fix, trying to write a gtest for it though.

Comment 9

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

The gtest requires bug 1267471 to be fixed.

Comment 10

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

Attachment: MozReview Request: Bug 1265510 - Add some scroll-snapping logging to APZC. r?botond

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

Comment 11

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

Attachment: MozReview Request: Bug 1265510 - Add a gtest for interrupting a scroll snap. r?botond

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

Comment 12

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

Attachment: MozReview Request: Bug 1265510 - Ensure that new input blocks still allow APZCs with interrupted animations to scroll-snap. r?botond

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

Comment 13

[Botond Ballo [:botond]]

Comment on attachment 8745124 [details]
MozReview Request: Bug 1265510 - Add some scroll-snapping logging to APZC. r?botond

https://reviewboard.mozilla.org/r/48861/#review45671

Comment 14

[Botond Ballo [:botond]]

Comment on attachment 8745125 [details]
MozReview Request: Bug 1265510 - Add a gtest for interrupting a scroll snap. r?botond

https://reviewboard.mozilla.org/r/48863/#review45683

::: gfx/layers/apz/test/gtest/TestSnapping.cpp:16
(Diff revision 1)
> +class APZCSnappingTester : public APZCTreeManagerTester
> +{
> +protected:
> +  UniquePtr<ScopedLayerTreeRegistration> registration;
> +
> +  void CreateBug1265510LayerTree() {

In bug 1257288 I started just putting the code that would go into CreateXXXLayerTree() directly into the test, for layer trees only used for one test. It seems more natural to me to have these two pieces of code together in one place.

Feel free to do likewise here if you agree.

::: gfx/layers/apz/test/gtest/TestSnapping.cpp:61
(Diff revision 1)
> +    mcc->AdvanceByMillis(5);
> +    outer->AdvanceAnimations(mcc->Time());
> +  }
> +  // Now do another wheel in a new transaction. This should start scrolling the
> +  // inner frame; we verify that it does by checking the inner scroll position.
> +  TimeStamp newTransactionTime = now + TimeDuration::FromMilliseconds(gfxPrefs::MouseWheelTransactionTimeoutMs() + 100);

Can we avoid this (using a timestamp in the future without advancing animations to that point, which feels fishy) by moving the mouse instead? That should timeout the transaction early.

Comment 15

[Botond Ballo [:botond]]

Comment on attachment 8745126 [details]
MozReview Request: Bug 1265510 - Ensure that new input blocks still allow APZCs with interrupted animations to scroll-snap. r?botond

https://reviewboard.mozilla.org/r/48865/#review45685

Comment 16

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

(In reply to Botond Ballo [:botond] from comment #14)
> In bug 1257288 I started just putting the code that would go into
> CreateXXXLayerTree() directly into the test, for layer trees only used for
> one test. It seems more natural to me to have these two pieces of code
> together in one place.
> 
> Feel free to do likewise here if you agree.

Yeah, I agree that makes more sense. I'll update this to do that.

> ::: gfx/layers/apz/test/gtest/TestSnapping.cpp:61
> > +  // Now do another wheel in a new transaction. This should start scrolling the
> > +  // inner frame; we verify that it does by checking the inner scroll position.
> > +  TimeStamp newTransactionTime = now + TimeDuration::FromMilliseconds(gfxPrefs::MouseWheelTransactionTimeoutMs() + 100);
> 
> Can we avoid this (using a timestamp in the future without advancing
> animations to that point, which feels fishy) by moving the mouse instead?
> That should timeout the transaction early.

We can, but it requires interleaving WidgetEvent-type events with InputData-type events, because the mousemove is only handled as a WidgetEvent, AFAICT [1]. To me that's just as fishy, but I can change it if you still prefer that.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp?rev=2d88322c06d8#1002

Comment 17

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #16)
> > Can we avoid this (using a timestamp in the future without advancing
> > animations to that point, which feels fishy) by moving the mouse instead?
> > That should timeout the transaction early.
> 
> We can, but it requires interleaving WidgetEvent-type events with
> InputData-type events, because the mousemove is only handled as a
> WidgetEvent, AFAICT [1]. To me that's just as fishy, but I can change it if
> you still prefer that.

Ah, good point. Let's leave it like this then.

Comment 18

[Botond Ballo [:botond]]

Comment on attachment 8745125 [details]
MozReview Request: Bug 1265510 - Add a gtest for interrupting a scroll snap. r?botond

https://reviewboard.mozilla.org/r/48863/#review45831

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/46d381873f3f
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b158eeb4bcc
https://hg.mozilla.org/integration/mozilla-inbound/rev/0de26e36ddd1

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/46d381873f3f
https://hg.mozilla.org/mozilla-central/rev/8b158eeb4bcc
https://hg.mozilla.org/mozilla-central/rev/0de26e36ddd1

Comment 21

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

Comment on attachment 8745126 [details]
MozReview Request: Bug 1265510 - Ensure that new input blocks still allow APZCs with interrupted animations to scroll-snap. r?botond

Approval Request Comment (for all patches)
[Feature/regressing bug #]: APZ
[User impact if declined]: in some cases mandatory scroll snapping is not respected, and scrollframes can be left at unsnapped positions
[Describe test coverage new/current, TreeHerder]: the second patch on this bug is a test
[Risks and why]: low risk. this patch (part 3) is the actual fix, part 1 is logging and part 2 is a test. the fix is a one-liner
[String/UUID change made/needed]: none

Comment 22

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

wontfix'ing for 47 because it doesn't seem important enough to uplift to beta when we're halfway through the cycle.

Comment 23

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

Comment on attachment 8745126 [details]
MozReview Request: Bug 1265510 - Ensure that new input blocks still allow APZCs with interrupted animations to scroll-snap. r?botond

Let's get all the APZ fixes into aurora.

Comment 24

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

Logging and test also to uplift?  Just making sure.

Comment 25

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

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #24)
> Logging and test also to uplift?  Just making sure.

Yes please. They're not functional changes but would be nice to uplift as well.

Comment 26

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

Comment on attachment 8745125 [details]
MozReview Request: Bug 1265510 - Add a gtest for interrupting a scroll snap. r?botond

[Triage Comment]

Comment 27

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

Comment on attachment 8745124 [details]
MozReview Request: Bug 1265510 - Add some scroll-snapping logging to APZC. r?botond

[Triage Comment]

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c16fd534aba8
https://hg.mozilla.org/releases/mozilla-aurora/rev/7b8aae1bc01e
https://hg.mozilla.org/releases/mozilla-aurora/rev/6ddc792201a2