Closed Bug 1265510 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Panning and Zooming, defect)

Unspecified
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox45 --- unaffected
firefox46 --- wontfix
firefox47 --- wontfix
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: mstange, Assigned: kats)

References

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(4 files, 1 obsolete file)

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.
Attached file 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?
Oh shoot. Wrong bug.
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: apz-desktop
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
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.
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
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.
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.