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

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mstange, Assigned: kats)

Tracking

({regression})

Trunk
mozilla49
Unspecified
Mac OS X
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 unaffected, firefox46 wontfix, firefox47 wontfix, firefox48 fixed, firefox49 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

3 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.
(Reporter)

Comment 1

3 years ago
Created attachment 8742480 [details]
testcase
I can't reproduce this with a mouse wheel. I'll try with touch events on a touchscreen laptop.
(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?
Created attachment 8742572 [details]
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/
Attachment #8742572 - Flags: review?(bugmail.mozilla)
Oh shoot. Wrong bug.

Updated

3 years ago
Attachment #8742572 - Attachment is obsolete: true
Attachment #8742572 - Flags: review?(bugmail.mozilla)
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.
Blocks: 1013364
status-firefox45: --- → unaffected
status-firefox46: --- → wontfix
status-firefox47: --- → affected
Keywords: regression
OS: Unspecified → Mac OS X
Whiteboard: [gfx-noted]
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.
This is easy enough to fix, trying to write a gtest for it though.
Assignee: nobody → bugmail.mozilla
The gtest requires bug 1267471 to be fixed.
Depends on: 1267471
Created attachment 8745124 [details]
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/
Attachment #8745124 - Flags: review?(botond)
Attachment #8745125 - Flags: review?(botond)
Attachment #8745126 - Flags: review?(botond)
Created attachment 8745125 [details]
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/
Created attachment 8745126 [details]
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 on attachment 8745124 [details]
MozReview Request: Bug 1265510 - Add some scroll-snapping logging to APZC. r?botond

https://reviewboard.mozilla.org/r/48861/#review45671
Attachment #8745124 - Flags: review?(botond) → review+
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.
Attachment #8745125 - Flags: review?(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
Attachment #8745126 - Flags: review?(botond) → review+
(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
(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.

Updated

3 years ago
Attachment #8745125 - Flags: review+
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 20

3 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
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Updated

3 years ago
Target Milestone: mozilla48 → mozilla49
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
Attachment #8745126 - Flags: approval-mozilla-aurora?
wontfix'ing for 47 because it doesn't seem important enough to uplift to beta when we're halfway through the cycle.
status-firefox47: affected → wontfix
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.
Attachment #8745126 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Logging and test also to uplift?  Just making sure.
Flags: needinfo?(bugmail.mozilla)
(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.
Flags: needinfo?(bugmail.mozilla)
Comment on attachment 8745125 [details]
MozReview Request: Bug 1265510 - Add a gtest for interrupting a scroll snap. r?botond

[Triage Comment]
Attachment #8745125 - Flags: approval-mozilla-aurora+
Comment on attachment 8745124 [details]
MozReview Request: Bug 1265510 - Add some scroll-snapping logging to APZC. r?botond

[Triage Comment]
Attachment #8745124 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.