search

flutter / flutter

Flutter makes it easy and fast to build beautiful apps for mobile and beyond
https://flutter.dev
BSD 3-Clause "New" or "Revised" License
162.18k stars 26.64k forks source link

Interactable ScrollView content when settling a scroll activity #145848

Open Michal-MK opened 1 month ago

Michal-MK commented 1 month ago

Currently when the user:

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 #145330

Pre-launch Checklist

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.

Michal-MK commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

https://github.com/flutter/flutter/assets/12874766/5e4ce3d4-b6d7-4d0e-8649-5605a9720ff8

Michal-MK commented 1 month ago

"I think this can be fixed by making the vertical scroll views in the tab bar view not use the primary scroll controller." I am not sure how this can be done without it being a breaking change. Since now it registers taps on the status bar which scroll the scrollable to the top on iOS. By making it non primary this functionality is lost no? @Piinks

Piinks commented 1 month ago

I am not sure how this can be done without it being a breaking change.

Oh no, I meant to go into the gallery code and fix the issue there, rather than changing the framework. If the gallery is broken on master without this change, we should fix it, which will unblock this change. The code lives in flutter/dev/integration_tests/flutter_gallery.

Piinks commented 1 month ago

I think https://github.com/flutter/flutter/pull/146403 is trying to solve the same problem, and not having an issue with the Gallery.

Michal-MK commented 1 month ago

I'll try altering the Gallery then, I was not sure if that was allowed :) With that I'll see what else needs changing to make this a proper PR.

Michal-MK commented 1 month ago

Alright, with the explicit ScrollController for the Scrollbar in the gallery we are all green.

So I'll do some cleanup and write tests tomorrow. Then I'll change it from a Draft to a normal PR.

Michal-MK commented 1 month ago

@Piinks It seems I hit another block, this time customer tests are failing.

| 00:07 +201: /Volumes/Work/s/w/ir/x/t/flutter_customer_testing.super_editor.qUNvnh/tests/super_editor/test/super_editor/supereditor_scrolling_test.dart: SuperEditor scrolling stops momentum on tap down and doesn't place the caret (on Android)
| ══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
| The following TestFailure was thrown running a test:
| Expected: <1194.3136212798695>
|   Actual: <1191.6569144503435>
|
| When the exception was thrown, this was the stack:
| #4      main.<anonymous closure>.<anonymous closure> (file:///Volumes/Work/s/w/ir/x/t/flutter_customer_testing.super_editor.qUNvnh/tests/super_editor/test/super_editor/supereditor_scrolling_test.dart:501:7)
| <asynchronous suspension>
| #5      testWidgetsOnAndroid.<anonymous closure> (package:flutter_test_runners/src/platform_runners.dart:235:7)
| <asynchronous suspension>
| #6      testWidgets.<anonymous closure>.<anonymous closure> (package:flutter_test/src/widget_tester.dart:183:15)
| <asynchronous suspension>
| #7      TestWidgetsFlutterBinding._runTestBody (package:flutter_test/src/binding.dart:1017:5)
| <asynchronous suspension>
| <asynchronous suspension>
| (elided one frame from package:stack_trace)
|
| This was caught by the test expectation on the following line:
|   file:///Volumes/Work/s/w/ir/x/t/flutter_customer_testing.super_editor.qUNvnh/tests/super_editor/test/super_editor/supereditor_scrolling_test.dart line 501
| The test description was:
|   stops momentum on tap down and doesn't place the caret (on Android)
| ════════════════════════════════════════════════════════════════════════════════════════════════════

Those are not part of the repository as far as I know.

Michal-MK commented 1 month ago

Thanks for the link, I looked at it and from local testing it seems that my current code pumps one extra frame when tapping on a scrollable. In my sample app tap to stop works so I will have to figure out what it up with that test and why my changes break it.

Michal-MK commented 1 month ago

@Piinks I got the customer tests working somehow :D

And I managed to remove the changes to the nested_scroll_view and remove one extra warnIfMissed below, as now it will correctly detect hits when the scrollable is over-scrolled.

With that there are a couple of things that need doing: 1) I do not like the type cast in BallisticScrollActivity to the the ScrollPosition from the delegate, but if I introduce it to the delegate directly, it would mean editing(polluting) a lot of classess again, the outOfRange was fine as most of the implementations already defined the field/getter; but nobody defines updateIgnorePointer currently. Thoughts on this?

2) There is still a difference between native. That is long presses. Native iOS does not allow long press on over-scrolled views. That would require setting a timeout and after that making the shouldIgnorePointer reset to true. But by then the interactable widget (e.g. Button) would already be highlighted? I have not touched and I have 0 experience with input handling. But I have a feeling that will not be as simple to implement (timeout sync/cancelling etc...). Should that be part of this PR?

3) Other scrollables besides PageView. Here I implemented it because it was annoying as shown in the videos at the top, but I did not actually test the native behavior of either platform. Maybe it "does not work" there either so it is expected. Because if that was the case, the whole preferredBallisticIgnorePointer would go away, simplifying this PR quite a bit. So I'll need to hack together a native app and test this still.

Michal-MK commented 1 month ago

I hacked together a simple page view using Chat GPT in Kotlin and the same janky behavior is present there as well. So... what now? From a UX point of view it is horrible, but it is consistent with the native behavior.

Michal-MK commented 3 weeks ago

https://github.com/flutter/flutter/assets/12874766/dd8fda07-2e68-45d5-aeaf-a29a9dc60b24

That the native world has the same issue, so is it then an issue? Should I bother fixing it here if it may be intended?

Michal-MK commented 3 weeks ago

@Piinks How about I remove the code related to the PageView functionality for now, that way we do not need to care about the Colors Gallery issue and at least get the out of bounds scrolling interactivity merged, since that is what I wrote tests for. I think that this functionality is ready. I will create a separate issue with the PageView with the videos from here and another one for the GalleryApp Colors trackpad bug. So we can track it separately and not clutter this one?

Piinks commented 3 weeks ago

How about I remove the code related to the PageView functionality for now, that way we do not need to care about the Colors Gallery issue and at least get the out of bounds scrolling interactivity merged, since that is what I wrote tests for. I think that this functionality is ready. I will create a separate issue with the PageView with the videos from here and another one for the GalleryApp Colors trackpad bug. So we can track it separately and not clutter this one?

I am not sure what that would look like? I have not done a thorough review as you've told me a couple times it is not ready or just a proof of concept. The gallery issue is not specific to this change and needs to be fixed anyways. As far as the changes this PR is making, I would rather we sort out the intended behavior before merging any partial fix. We typically do not land partial fixes, or changes where we don't quite know what the behavior should be.

Michal-MK commented 3 weeks ago

@Piinks "The gallery issue is not specific to this change and needs to be fixed anyways." Yes that is what we are both saying :D Since you said they are unrelated and should be fixed in a separate PR that is what I repeated.

"As far as the changes this PR is making, I would rather we sort out the intended behavior before merging any partial fix. We typically do not land partial fixes, or changes where we don't quite know what the behavior should be." I agree with that, that is again why I suggested removing the PageView "fix", since native behaves in the same "broken" way, or moving it into a separate issue where we can "figure out what the behavior should be". As for the interactivity when a scrollable is over scrolled, that is working and is consistent with the native behavior on iOS/MacOS. The attached videos show this. So I am not sure if by "partial fix" you meant the PageView, the long press or this PR as a whole?

Piinks commented 2 weeks ago

Hi @Michal-MK, I've reread the comments above a couple of times, and I apologize I do not understand the state of the proposal. It sounds like some of it works, some of it does not and some of it is matching native behavior. Can you help me understand, from your last comment, if you remove the PageView changes, what problem is this PR solving? What I meant by partial fix was that it sounded like this would only apply to some scrollable classes rather than all of them.

Michal-MK commented 2 weeks ago

@Piinks Sure, when we remove the PageView change (which currently behaves the way native tabs do in Android) we are left with the change to BouncingScrollPhysics not allowing interaction with the contents of the scrollable. When a scrollable with bouncing scroll physics is out of bounds, native iOS allows taps on the scrollable content, however Flutter currently forwards the input to the scrollable and children are "hit test invisible", so now you can only resume the scroll gesture, but not perform a tap on a button in the scrollable.

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

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

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

These are the relevant videos.

The changes are only in scroll_activty.dart, scroll_position.dart and scroll_position_with_single_context.dart. Then classes that extend/implement these need to now have outOfRange boolean getter, and most do already, so I assume most of the scrollable implementations in Flutter should support it without changes to the source code in them directly (I had to modify only nested_scroll_view.dart to make it compile). I will have to check 2D scrollables as I have not used them yet (even in my apps). The other thing that needs to be looked at, is that long press gestures should not happen on overscrolled scrollables, with this change they do. I asked if someone who has more experience in input handling would want to look at this, but I guess I will have to dig in myself and somehow make the long press gesture bail out of the arena when we are overscrolled. Before I start looking at it, i want to reduce the scope of this PR as now it is addressing 3 unrelated issues at once.

Piinks commented 2 weeks ago

Thank you for explaining!

I want to reduce the scope of this PR as now it is addressing 3 unrelated issues at once.

Yes, I think that may be where I keep getting mixed up. Let's focus on one issue and one change at a time. I really appreciate your contributions to fix these things! What of the 3 issues you mentioned would you like to fix first?

Michal-MK commented 1 week ago

Sorry I am just too busy right now; First I want to fix the issue in the video, since that is what I originally reported. Hopefully I will have the changes ready by the end of the week. Then the Gallery, and the PageView last, if we actually decide to change it.

Piinks commented 1 week ago

Sorry I am just too busy right now; First I want to fix the issue in the video, since that is what I originally reported. Hopefully I will have the changes ready by the end of the week. Then the Gallery, and the PageView last, if we actually decide to change it.

No worries at all, I appreciate your work on this, and if you need to come back later when you have time, or if you don't have time at all, that's all ok too.

If you do work further on this, just let me know when you'd like another review, I'll be happy to take a look and discuss further. :)

Michal-MK commented 1 day ago

@Piinks Hi, I created separate issues for the other two parts that were originally here. Now this PR has only the overscroll interactivity fix.

Michal-MK commented 5 hours ago

Alright I made some changes (hopefully the tests pass 🙏), and added comments to your review notes, thanks for those. Now, "throws the imaginary ball your way" :D @Piinks