Markus Stange [:mstange] Reporter
	Description • 8 years ago

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.

	Botond Ballo [:botond]
	Comment 2 • 8 years ago

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

	Botond Ballo [:botond]
	Comment 3 • 8 years ago

(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?

	Botond Ballo [:botond]
	Comment 4 • 8 years ago

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

	Botond Ballo [:botond]
	Comment 5 • 8 years ago

Oh shoot. Wrong bug.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 6 • 8 years ago

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.

	Botond Ballo [:botond]
	Comment 7 • 8 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 8 • 8 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 9 • 8 years ago

The gtest requires bug 1267471 to be fixed.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 10 • 8 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 11 • 8 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 12 • 8 years ago

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

	Botond Ballo [:botond]
	Comment 13 • 8 years ago

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

	Botond Ballo [:botond]
	Comment 14 • 8 years ago

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.

	Botond Ballo [:botond]
	Comment 15 • 8 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 16 • 8 years ago

(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

	Botond Ballo [:botond]
	Comment 17 • 8 years ago

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

	Botond Ballo [:botond]
	Comment 18 • 8 years ago

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

	Pulsebot
	Comment 19 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 20 • 8 years ago
bugherder

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 21 • 8 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 22 • 8 years ago

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

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 23 • 8 years ago

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.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 24 • 8 years ago

Logging and test also to uplift?  Just making sure.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 25 • 8 years ago

(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.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 26 • 8 years ago

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

[Triage Comment]

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 27 • 8 years ago

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

[Triage Comment]

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 28 • 8 years ago
bugherder uplift

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