Skip to content
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.

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

Interactable ScrollView content when settling a scroll activity #145848

Merged
merged 52 commits into from
Jul 2, 2024

Conversation

Michal-MK
Copy link
Contributor

@Michal-MK Michal-MK commented Mar 27, 2024

Currently when the user:

  • flings a scrollable
  • overscrolls, and the scrollable is trying to settle
  • applies DrivenScrollActivity
  • swipes from one tab to the next

All inputs are discarded or forwarded directly to the Scrollable widget (scroll activity).

This leads to situations like these:

2024-03-27.19-45-59.mp4
2024-03-27.19-46-19.mp4
2024-03-27.19-46-46.mp4

Which leads to poor experience on iOS. The native behavior of iOS is to allow touches while a scrollable is settling:

native.mp4

This PR alters the shouldIgnoreTouches of BallisticScrollAvtivity and DrivenScrollActivity to not make the child of the scrollable ignore touches.

Fixes #145330

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].
  • I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
  • All existing and new tests are passing.

Currently tests that test tap to stop are not working as the taps now register when they should not. Because there is no distinction between flings inside and flings that go out of range.

@github-actions github-actions bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. labels Mar 27, 2024
@Piinks Piinks self-requested a review March 29, 2024 19:07
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.

Hey @Michal-MK it looks like this is still a draft, so I'll wait for your say so when it's ready for review. :)

@Michal-MK
Copy link
Contributor Author

Michal-MK commented Mar 29, 2024

@Piinks Yeah, I am sort of stuck as I reverted all the meaningful changes and the tests are still failing (other than analyze, that one I expect to fail) looking at the output widget_tester_test is something I did not touch so I assume something else is going on? Or the:
E.g. PathNotFoundException: Cannot open file, path = 'C:\b\s\w\ir\x\w\recipe_cleanup\flutter_logs_dir/fake_result.json' (OS Error: The system cannot find the file specified.

I'll continue after Easter :)

EDIT: Ok seeing as other PRs have functioning tests I'll comment out everything and try that.

@Michal-MK
Copy link
Contributor Author

The three failing checks are in the FlutterGallery in the Style->Colors section. The issue there is the use of Scrollbar, widget. There are tabs, and as the test swipes through them it, at some point, it tries to render a second Scrollbar which then fails, as both are reading the primary scroll controller. I validated this on master using trackpad scrolling which is what this PR effectively brings to finger gestures and it fails on master as well. The test just does not pick it up as it is not using a trackpad to scroll (.fling/.trackpadFling and other alternatives). I do not see how to fix this without the TabBarView somehow providing a custom ScrollController to its children that is somehow connected to the primary one once the transition animation finishes. @Piinks

@Piinks
Copy link
Contributor

Piinks commented Apr 3, 2024

Interesting, I tried using a trackpad to reproduce this at https://flutter-gallery-archive.web.app/#/demo/colors but did not see an error from the scrollbar. Do you know how this change could have caused it?

I think this can be fixed by making the vertical scroll views in the tab bar view not use the primary scroll controller. This would be unrelated to TabBarView, since the list of colors is separate.

@Michal-MK
Copy link
Contributor Author

It happens on master without any changes, may be limited to MacOS then? Here is a video

Screen.Recording.2024-04-04.at.17.13.37.mp4

@Michal-MK
Copy link
Contributor Author

Yep, that looks good. Just removed the trailing space so "Linux analyze" is happy

@Piinks
Copy link
Contributor

Piinks commented Jul 2, 2024

Ah thank you! I was trying to save you another round of edits!

@Piinks
Copy link
Contributor

Piinks commented Jul 2, 2024

Ok, so this will just need a second approval. Seeking someone out now.

Copy link
Member

@nate-thegrate nate-thegrate left a comment

Choose a reason for hiding this comment

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

LGTM!

Thanks for making this improvement and for the thorough testing.
I made an optional suggestion, but I'd be happy to merge right away 👍

Comment on lines 636 to 638
bool get outOfRange {
return (_outerPosition?.outOfRange ?? false) || _innerPositions.any((_NestedScrollPosition position) => position.outOfRange);
}
Copy link
Member

Choose a reason for hiding this comment

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

Not a big deal whatsoever, but it would be nice to stay within the 80 characters style guideline:

bool get outOfRange {
  if (_outerPosition?.outOfRange ?? false) {
    return true;
  }
  return _innerPositions.any((_NestedScrollPosition position) => position.outOfRange);
}

Copy link
Member

Choose a reason for hiding this comment

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

Honestly, this is very mild, compared to other things I've seen. I'll go ahead and throw on autosubmit :)

@nate-thegrate nate-thegrate added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 2, 2024
@auto-submit auto-submit bot merged commit 46030f1 into flutter:master Jul 2, 2024
73 checks passed
hello-coder-xu added a commit to hello-coder-xu/flutter that referenced this pull request Jul 3, 2024
* master: (88 commits)
  Fix scheduler event loop being stuck due to task with Priority.idle (flutter#151168)
  Fix result propagation in RenderSliverEdgeInsetsPadding.hitTestChildren (flutter#149825)
  docImports for flutter_test (flutter#151189)
  Interactable ScrollView content when settling a scroll activity (flutter#145848)
  [flutter_tools] Update the mapping for the Dart SDK internal URI (flutter#151170)
  Roll pub packages (flutter#151129)
  Fix typo (flutter#151192)
  [tool] Fix `stdin.flush` calls on processes started by `FakeProcessManager` (flutter#151183)
  Roll Flutter Engine from 433d360eee11 to 44278941443e (4 revisions) (flutter#151186)
  Use `ErrorHandlingFileSystem.deleteIfExists` when deleting .plugin_symlinks (flutter#151073)
  ScrollEndNotification example: auto-scroll based on RenderSliver constraints and geometry (flutter#143538)
  Roll Packages from 412ec46 to d2705fb (13 revisions) (flutter#151169)
  docimports for painting (flutter#151143)
  docimports for scheduler (flutter#151126)
  `dismissible.dart` code cleanup (flutter#150276)
  docimports for physics (flutter#151125)
  docimports for services (flutter#151134)
  docimports for cupertino (flutter#151149)
  docimports for gestures (flutter#151123)
  Docimports for foundation (flutter#151119)
  ...
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 4, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 4, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 4, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 4, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 5, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 5, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jul 7, 2024
Roll Flutter from af913a7 to fafd67d34d68 (41 revisions)

flutter/flutter@af913a7...fafd67d

2024-07-05 [email protected] Roll Flutter Engine from 74d40c160e48 to 4ee09d3b7f3b (1 revision) (flutter/flutter#151346)
2024-07-05 [email protected] Roll Flutter Engine from ba9c7b6336ef to 74d40c160e48 (1 revision) (flutter/flutter#151340)
2024-07-05 [email protected] Roll Flutter Engine from 1f0f950ea02a to ba9c7b6336ef (1 revision) (flutter/flutter#151331)
2024-07-05 [email protected] Roll Flutter Engine from 3c6a373bda3e to 1f0f950ea02a (1 revision) (flutter/flutter#151326)
2024-07-04 [email protected] Roll Flutter Engine from 79a91e38c587 to 3c6a373bda3e (2 revisions) (flutter/flutter#151318)
2024-07-04 [email protected] Roll Packages from d2705fb to 754de19 (3 revisions) (flutter/flutter#151315)
2024-07-04 [email protected] Roll Flutter Engine from 2b6bb516e7e6 to 79a91e38c587 (2 revisions) (flutter/flutter#151314)
2024-07-04 [email protected] Roll Flutter Engine from 8e2d05fa95d7 to 2b6bb516e7e6 (2 revisions) (flutter/flutter#151299)
2024-07-04 [email protected] Roll Flutter Engine from 4190543cb093 to 8e2d05fa95d7 (13 revisions) (flutter/flutter#151293)
2024-07-03 137456488 [email protected] Roll pub packages (flutter/flutter#151203)
2024-07-03 [email protected] Fix invalid URL suggestion for gradle incompatability (flutter/flutter#150999)
2024-07-03 423393 [email protected] Cupertino transparent navigation bars (flutter/flutter#149102)
2024-07-03 1961493 [email protected] Prepares semantics_update_test for upcoming link URL change (flutter/flutter#151261)
2024-07-03 [email protected] Add a message about spam/brigading (flutter/flutter#150583)
2024-07-03 [email protected] PinnedHeaderSliver example based on the iOS Settings AppBar (flutter/flutter#151205)
2024-07-03 [email protected] Update deprecation policy (flutter/flutter#151257)
2024-07-03 [email protected] SliverFloatingHeader (flutter/flutter#151145)
2024-07-03 34871572 [email protected] Remove warning when KGP version not detected (flutter/flutter#151254)
2024-07-03 34465683 [email protected] Feat: Add withOpacity to gradient (flutter/flutter#150670)
2024-07-03 [email protected] Fix references in examples (flutter/flutter#151204)
2024-07-03 82763757 [email protected] Fix link in tree hygene doc (flutter/flutter#151235)
2024-07-03 [email protected] content dimensions are not established get controller value error (flutter/flutter#148938)
2024-07-03 [email protected] chore: fix typos and link broken (flutter/flutter#150402)
2024-07-03 [email protected] Add example of goldenFileComparator usage in widget tests (flutter/flutter#150422)
2024-07-03 [email protected] Fix project name fallback (flutter/flutter#150614)
2024-07-03 [email protected] Roll Flutter Engine from a3e61c0fd1c2 to 4190543cb093 (1 revision) (flutter/flutter#151241)
2024-07-03 [email protected] Roll Flutter Engine from 8274f54f11be to a3e61c0fd1c2 (2 revisions) (flutter/flutter#151237)
2024-07-03 [email protected] Force regeneration of platform-specific manifests before running performance tests (flutter/flutter#151003)
2024-07-03 [email protected] Handle a SocketException thrown when sending the browser close command to Chrome (flutter/flutter#151197)
2024-07-03 [email protected] Roll Flutter Engine from a02e3f673da3 to 8274f54f11be (4 revisions) (flutter/flutter#151226)
2024-07-03 [email protected] Roll Flutter Engine from c5c0c54d6d1d to a02e3f673da3 (1 revision) (flutter/flutter#151212)
2024-07-03 [email protected] Roll Flutter Engine from 44278941443e to c5c0c54d6d1d (9 revisions) (flutter/flutter#151208)
2024-07-02 [email protected] Fix scheduler event loop being stuck due to task with Priority.idle (flutter/flutter#151168)
2024-07-02 [email protected] Fix result propagation in RenderSliverEdgeInsetsPadding.hitTestChildren (flutter/flutter#149825)
2024-07-02 [email protected] docImports for flutter_test (flutter/flutter#151189)
2024-07-02 [email protected] Interactable ScrollView content when settling a scroll activity (flutter/flutter#145848)
2024-07-02 [email protected] [flutter_tools] Update the mapping for the Dart SDK internal URI (flutter/flutter#151170)
2024-07-02 137456488 [email protected] Roll pub packages (flutter/flutter#151129)
2024-07-02 36861262 [email protected] Fix typo (flutter/flutter#151192)
2024-07-02 [email protected] [tool] Fix `stdin.flush` calls on processes started by `FakeProcessManager` (flutter/flutter#151183)
2024-07-02 [email protected] Roll Flutter Engine from 433d360eee11 to 44278941443e (4 revisions) (flutter/flutter#151186)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
...
TahaTesser pushed a commit to TahaTesser/flutter that referenced this pull request Jul 8, 2024
…ter#145848)

Currently when the user:
- flings a scrollable 
- overscrolls, and the scrollable is trying to settle
- applies `DrivenScrollActivity`
- swipes from one tab to the next

All inputs are discarded or forwarded directly to the Scrollable widget (scroll activity).

This leads to situations like these:

https://github.com/flutter/flutter/assets/12874766/51b7876f-5a91-4a86-aa21-c72f0b2c4263

https://github.com/flutter/flutter/assets/12874766/2f756a45-5e42-47d7-98a0-12f071d34e7c

https://github.com/flutter/flutter/assets/12874766/5eb998a1-b3b8-42a1-8b04-543f68823c2b

Which leads to poor experience on iOS. The native behavior of iOS is to allow touches while a scrollable is settling:

https://github.com/flutter/flutter/assets/12874766/e1ae61f8-d59c-40ae-a4c4-ad919f0dc6bf

This PR alters the `shouldIgnoreTouches` of `BallisticScrollAvtivity` and `DrivenScrollActivity` to not make the child of the scrollable ignore touches.

Fixes flutter#145330

Currently tests that test tap to stop are not working as the taps now register when they should not. Because there is no distinction between flings inside and flings that go out of range.
victorsanni pushed a commit to victorsanni/flutter that referenced this pull request Jul 8, 2024
…ter#145848)

Currently when the user:
- flings a scrollable 
- overscrolls, and the scrollable is trying to settle
- applies `DrivenScrollActivity`
- swipes from one tab to the next

All inputs are discarded or forwarded directly to the Scrollable widget (scroll activity).

This leads to situations like these:

https://github.com/flutter/flutter/assets/12874766/51b7876f-5a91-4a86-aa21-c72f0b2c4263

https://github.com/flutter/flutter/assets/12874766/2f756a45-5e42-47d7-98a0-12f071d34e7c

https://github.com/flutter/flutter/assets/12874766/5eb998a1-b3b8-42a1-8b04-543f68823c2b

Which leads to poor experience on iOS. The native behavior of iOS is to allow touches while a scrollable is settling:

https://github.com/flutter/flutter/assets/12874766/e1ae61f8-d59c-40ae-a4c4-ad919f0dc6bf

This PR alters the `shouldIgnoreTouches` of `BallisticScrollAvtivity` and `DrivenScrollActivity` to not make the child of the scrollable ignore touches.

Fixes flutter#145330

Currently tests that test tap to stop are not working as the taps now register when they should not. Because there is no distinction between flings inside and flings that go out of range.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: text input Entering text in a text field or keyboard related problems autosubmit Merge PR when tree becomes green via auto submit App f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BouncingScrollPhysics Capturing all input until settled
3 participants