Skip to content

Add macOS-specific scroll physics #108298

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
auto-submit merged 5 commits into from
Sep 19, 2022
Merged

Add macOS-specific scroll physics #108298

auto-submit merged 5 commits into from
Sep 19, 2022

Conversation

@moffatman
Copy link
Contributor

@moffatman moffatman commented Jul 25, 2022

image

Dotted line = BouncingScrollPhysics
Solid line = new BouncingDesktopScrollPhysics
X-marks = Recorded data

Fixes #107752

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.

@flutter-dashboard flutter-dashboard bot added f: gestures flutter/packages/flutter/gestures repository. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. labels Jul 25, 2022
@goderbauer goderbauer requested a review from Piinks July 26, 2022 22:13
@tomgilder tomgilder mentioned this pull request Jul 29, 2022
Closed
@moffatman moffatman mentioned this pull request Jul 30, 2022
Merged
8 tasks
@hellohuanlin
Copy link
Contributor

hellohuanlin commented Aug 1, 2022

im having trouble understanding this graph. is y-axis the initial distance or initial speed?

@Piinks Piinks added platform-mac Building on or for macOS specifically a: desktop Running on desktop a: fidelity Matching the OEM platforms better labels Aug 1, 2022
@moffatman
Copy link
Contributor Author

moffatman commented Aug 2, 2022

im having trouble understanding this graph. is y-axis the initial distance or initial speed?

The y-axis is velocity

@Piinks
Copy link
Contributor

Piinks commented Aug 9, 2022

Thanks so much for this @moffatman! And apologies for the delay, I should be finished reviewing today

Piinks
Piinks reviewed Aug 9, 2022
View reviewed changes
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You want to update the getScrollPhysics of CupertinoScrollBehavior as well. 👍

packages/flutter/lib/src/gestures/velocity_tracker.dart Outdated
Copy link
Contributor

@Piinks Piinks Aug 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the docs for the iOS version, there is a reference to https://developer.apple.com/documentation/uikit/uiscrollviewdelegate/1619385-scrollviewwillenddragging, is that also relevant here? Or is there another, more relevant reference?

Copy link
Contributor Author

@moffatman moffatman Aug 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There isn't a direct analogue, in AppKit the scroll momentum state is seen based on the incoming events' [NSEvent momentumPhase]. The part which converts the velocity to the momentum is not exposed at all.

@goderbauer goderbauer requested a review from Piinks August 23, 2022 22:09
Piinks
Piinks reviewed Sep 14, 2022
View reviewed changes
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking really good. Apologies for the delay while I was gone.

packages/flutter/lib/src/physics/friction_simulation.dart Outdated
Comment on lines 20 to 24
double g = guess;
for (int i = 0; i < iterations; i++) {
g = g - (f(g) - target) / df(g);
}
return g;
Copy link
Contributor

@Piinks Piinks Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The style guide discourages abbreviations https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#avoid-abbreviations

Can you expand this shorthand so it is easier to understand?

Piinks
Piinks approved these changes Sep 19, 2022
View reviewed changes
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really great, thank you for your incredible attention to detail. :)
LGTM!

@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 19, 2022
@auto-submit auto-submit bot merged commit 70b19ff into flutter:master Sep 19, 2022
This was referenced Sep 20, 2022
Merged
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 20, 2022
@engine-flutter-autoroll
70b19ff Add macOS-specific scroll physics (flutter/flutter#108298)
19abad2
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 20, 2022
@engine-flutter-autoroll
70b19ff Add macOS-specific scroll physics (flutter/flutter#108298)
7970f84
@moffatman moffatman mentioned this pull request Nov 21, 2022
Merged
8 tasks
@appinteractive
Copy link

appinteractive commented Dec 21, 2022

which flutter version will this land in?

@moffatman
Copy link
Contributor Author

moffatman commented Dec 21, 2022

@appinteractive Has been in beta channel for a while, should be in the next stable version 3.7. No idea when that releases though

@hellohuanlin
Copy link
Contributor

hellohuanlin commented Dec 21, 2022

IIRC next release will be around next Jan @appinteractive

@appinteractive
Copy link

appinteractive commented Dec 22, 2022 •
edited
Loading

Thanks for the response, tried it in 3.7 pre but I still have scroll that has way to little friction on macOS, do I need to specify anything for it to work? Using MaterialApp as a base because Cupertino is barely usable with plugins. (Missing Material)

@moffatman
Copy link
Contributor Author

moffatman commented Dec 22, 2022

@appinteractive Can you share some code which reproduces the issue? If you are setting custom scroll physics on any widget, the correct physics for macOS should be BouncingScrollPhysics(decelerationRate: ScrollDecelerationRate.fast). But if you never set physics on any widget, it should be getting the correct physics by default.

@GroovinChip
Copy link
Contributor

GroovinChip commented Jan 27, 2023 •
edited
Loading

@Piinks After the release of Flutter 3.7 with this included, I'm seeing The Scrollbar's ScrollController has no ScrollPosition attached. errors for apps using macos_ui. I assume I need to remove or alter some of the custom scrolling code in the library, but my efforts so far haven't yielded any results. I read the desktop scrolling migration guide and I am only more confused. Should I remove all usages of our custom MacosScrollbar, as well as our custom MacosScrollBehavior?

EDIT: It's also worth mentioning that the errors I'm getting do not trace which scrollable the issue is arising from :/

@Piinks
Copy link
Contributor

Piinks commented Jan 27, 2023

Hi @GroovinChip, it sounds like there may be a regression. Can you please file an issue with the last flutter version you were using and a minimal reproduction?
We won’t be able to diagnose a potential regression in an old PR.

@GroovinChip
Copy link
Contributor

GroovinChip commented Jan 27, 2023

@Piinks Sure thing! Didn't realize it could be a regression :)

@GroovinChip GroovinChip mentioned this pull request Jan 27, 2023
Closed
@GroovinChip
Copy link
Contributor

GroovinChip commented Jan 27, 2023

Hi @GroovinChip, it sounds like there may be a regression. Can you please file an issue with the last flutter version you were using and a minimal reproduction? We won’t be able to diagnose a potential regression in an old PR.

Done - #119323

@xu-baolin xu-baolin mentioned this pull request Feb 7, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Piinks Piinks Piinks approved these changes

Assignees

No one assigned

Labels

a: desktop Running on desktop a: fidelity Matching the OEM platforms better autosubmit Merge PR when tree becomes green via auto submit App f: gestures flutter/packages/flutter/gestures repository. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. platform-mac Building on or for macOS specifically

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Scrolling with trackpad continues momentum animation for too long

5 participants

@moffatman @hellohuanlin @Piinks @appinteractive @GroovinChip