Reland "PageView scroll physics to match Android #95423" #97166

nt4f04uNd · 2022-01-24T22:47:35Z

Fixes #38357
Reland #95423
Which was reverted to be treated as breaking change here #97150

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

nt4f04uNd · 2022-01-24T22:55:28Z

@Piinks is the removal of dependency on spring what broke the customer? So a description of a breaking change would be something like "PageScrollPhysics now only use ScrollPhysics.spring when going out of range (for example on iOS), while previously it could be used to change the whole animation from user swipes".

Piinks · 2022-01-24T23:12:10Z

Thanks for re-opening! Maybe that was it, what I saw in the customer tests were golden file images. They were capturing pictures of the screen after swiping through pages, but instead of being on the last page as expected, they were on the first page.
The swiping in the test was engaged with a fling gesture. I saw this exact same thing happen across two different customers. WDYT? I can try to get more context if you need it.

nt4f04uNd · 2022-01-24T23:15:55Z

Thanks for the details, it might be the case that this is because of the change of minFlingDistance and/or minFlingVelocity to bigger values. More context would certainly be helpful.

Piinks · 2022-01-24T23:42:48Z

Customer A

Test code to swipe to next page:

await fling(customerApp, Offset(-200, 0), 200);

I do not know if it makes a difference, but the screen ratio of this test is like a phone in landscape mode, longer across than from top to bottom.

Customer B

Same issue, swipes did not end up on the expected page index. Screen size is similar to portrait oriented phone. Swipe gesture from test:

await tester.fling(
        customerWidget,
        Offset(-200, 0),
        200,
      );

I think the way to proceed here is to add a temporary opt-in flag. I cannot change the customer tests ahead of time, because the behavior would not be right without this change.

What we usually do here is

we add a temporary flag,
land the change,
migrate the affected customers to the new behavior using the opt in flag,
change the default of the opt in flag to make it an opt out flag
update customer code to stop using the flag
remove flag
release a migration guide documenting the breaking change

I think the ScrollBehavior might be the easiest way to do it, that way I can just opt the whole customer's app in pretty easily, and update the tests, but I'd like to know what you think about all this. Let me know and we can find a way for this to move forward. :)

nt4f04uNd · 2022-02-03T20:41:17Z

(I do not forget about this and my other PRs, it's just that I don't have much time for open source now, anyway I'll try my best to move this forward)

Piinks · 2022-02-17T22:00:35Z

Hey @nt4f04uNd, no worries at all. I really appreciate all of your contributions, they are often non-trivial solutions and very valuable to everyone - so thanks!
If you like, I can pick this one up and work with the affected customers to re-land it. Let me know.

Either way we'll still be here ready to work together when you come back. 😉

nt4f04uNd · 2022-02-23T16:45:00Z

packages/flutter/lib/src/widgets/page_view.dart

+    return targetPage - page;
+  }
+
+  double _getTargetPixels(ScrollMetrics position, Tolerance tolerance, double velocity) {


@Piinks I just thought whether would it make sense to make some of these methods public, so people can restore the old implementation if they want (or just change it to whatever else)?

I don't know if that would be super beneficial, ultimately the changes you are introducing are the way it should be, there are just a lot of tests out there that expect the old behavior.

nt4f04uNd · 2022-02-23T16:45:27Z

If you like, I can pick this one up and work with the affected customers to re-land it. Let me know

Thanks, I'd appreciate if you taken this part.

Piinks · 2022-02-25T00:19:17Z

If you like, I can pick this one up and work with the affected customers to re-land it. Let me know

Thanks, I'd appreciate if you taken this part.

Just confirming, I am going to close this PR and clone it to re-open another one that I can manage landing with affected customers?

nt4f04uNd · 2022-02-27T10:24:56Z

Yeah, sure 🙂

Piinks

Hey @nt4f04uNd you recently reached out and asked if this was still on my radar. It is, although I have not had the cycles to work on it actively. There are some related issues that need resolution before we can re-land this, as noted in the comments of the test you wrote below.

I also think this should be only applied when the TargetPlatform is Android. The changes here are specific to Android and as you previously established (#97064), platforms like iOS have their own unique behavior. Especially since this broke customer tests, I think it would be prudent to only apply the change to the platform it is intended to reflect. Applying it to every platform would break more customers - who may be broken again later when we adjust the behavior for other platforms, creating a lot of churn for users.

Piinks · 2022-05-16T19:35:22Z

packages/flutter/test/widgets/scroll_physics_test.dart

-    // Changing velocity for time 0 may cause a sudden, unwanted damping/speedup effect
-    expect(bounce.createBallisticSimulation(position, 1000)!.dx(0), moreOrLessEquals(1000));
-    expect(clamp.createBallisticSimulation(position, 1000)!.dx(0), moreOrLessEquals(1000));
-    expect(page.createBallisticSimulation(position, 1000)!.dx(0), moreOrLessEquals(1000));


This being removed and the comments below it seem like the other issue should really be resolved first.

Piinks · 2022-05-23T16:51:50Z

Cross-referencing #11599 to maintain link for next planning session.

nt4f04uNd added 6 commits January 22, 2022 19:24

PageView scroll physics to match Android

9ed0329

from discussion + tests

23d0f7c

add TODO

2c93da3

unused import

e3f03fe

+

850d4ee

comment fixes

f0ba2c3

flutter-dashboard bot added f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. labels Jan 24, 2022

goderbauer requested a review from Piinks January 26, 2022 22:36

nt4f04uNd commented Feb 23, 2022

View reviewed changes

nt4f04uNd closed this Feb 27, 2022

Piinks reviewed May 16, 2022

View reviewed changes

Piinks mentioned this pull request Jul 15, 2022

Update Ballistic animation & ClampingScrollSimulation #107735

Merged

8 tasks

nt4f04uNd mentioned this pull request Dec 3, 2022

PageView scroll physics to match Android (as an opt-in) #116457

Closed

8 tasks

Reland "PageView scroll physics to match Android #95423" #97166

Reland "PageView scroll physics to match Android #95423" #97166

Uh oh!

Conversation

nt4f04uNd commented Jan 24, 2022

Pre-launch Checklist

Uh oh!

nt4f04uNd commented Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Piinks commented Jan 24, 2022

Uh oh!

nt4f04uNd commented Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Piinks commented Jan 24, 2022

Uh oh!

nt4f04uNd commented Feb 3, 2022

Uh oh!

Piinks commented Feb 17, 2022

Uh oh!

nt4f04uNd Feb 23, 2022

Choose a reason for hiding this comment

Uh oh!

Piinks Feb 25, 2022

Choose a reason for hiding this comment

Uh oh!

nt4f04uNd commented Feb 23, 2022

Uh oh!

Piinks commented Feb 25, 2022

Uh oh!

nt4f04uNd commented Feb 27, 2022

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Piinks May 16, 2022

Choose a reason for hiding this comment

Uh oh!

Piinks commented May 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nt4f04uNd commented Jan 24, 2022 •

edited

Loading

nt4f04uNd commented Jan 24, 2022 •

edited

Loading