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

Remove absorbPanEventsOnScrollables option #1455

Conversation

rorystephenson
Copy link
Contributor

@rorystephenson rorystephenson commented Mar 2, 2023

The only reason to disable absorbing of pan events is because some plugins which use drag gestures rely on it. It turns out if those plugins detect vertical/horizontal drag instead of pan this flag is not necessary.

Note that affected plugins need to be updated and this should be released in a major version bump as it is a breaking change.

See #1454 discussion leading to this PR.

Before releasing

  • Notify affected plugin authors of the required change.
  • Create new major version with changelog entry to describe how other plugins must migrate.

Understanding more

  • When absorbPanEventsOnScrollables is true a horizontal & vertical drag gesture detector are added to FlutterMap and if they are triggered FlutterMap's scale gesture logic is triggered since the vertical/horizontal drag gesture and scale gesture are in the same GestureArenaTeam and the scale gesture is the captain of that team. Horizontal/vertical drag gestures are recognised faster than scale/pan gestures and therefore when there are competing gestures they will prevail.
  • Scrollable Flutter widgets detect horizontal/vertical drag gestures which is why FlutterMap needs to detect them and then defer to its scale gesture if it wants to be pannable inside of a ListView.
  • flutter_map_dragmarker and likely other plugins detect pan gestures instead of horizontal/vertical drag gestures. This means that when absorbPanEventsOnScrollables is enabled those plugins don't work because FlutterMap's horizontal/vertical drag gestures have precedence. This is a simple fix, affected code should listen to onVerticalDrag(Start/Update/End) and onHorizontalDrag(Start/Update/End) instead of onPan(Start/Update/End).

Demo

With absabsorbPanEventsOnScrollables removed such that horizontal/vertical gestures are always detected in FlutterMap (see this PR's code changes) and a modified flutter_map_dragmarker (detecting horizontal/vertical drag instead of pan) is used it is possible to have a pan-able FlutterMap inside of a ListView containing drag markers which can be dragged:

demo.mp4

@JaffaKetchup
Copy link
Member

Thanks @rorystephenson, we'll try to have a look at this soon!

Copy link
Collaborator

@TesteurManiak TesteurManiak left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@JaffaKetchup
Copy link
Member

@rorystephenson When you get a moment from working on #1475, can you resolve the conflicts here for merge? (Also note that #1475 needs the same conflicts resolved).

The only reason this has been kept around is because some plugins which
used drag gestures relied on it. It turns out if those plugins detect
vertical/horizontal drag instead of pan this flag is not necessary.
@rorystephenson rorystephenson force-pushed the remove-absorb-pan-events-on-scrollables-option branch from 7993046 to 06b76dc Compare April 2, 2023 18:10
@rorystephenson
Copy link
Contributor Author

@rorystephenson When you get a moment from working on #1475, can you resolve the conflicts here for merge? (Also note that #1475 needs the same conflicts resolved).

Rebased both 👍 :)

@JaffaKetchup
Copy link
Member

@rorystephenson You happy for this to be merged now? Not sure if it will cause more conflicts.

@rorystephenson
Copy link
Contributor Author

@rorystephenson You happy for this to be merged now? Not sure if it will cause more conflicts.

Yep I think it's good to go.

@JaffaKetchup JaffaKetchup merged commit 870aeea into fleaflet:master Apr 2, 2023
@rorystephenson rorystephenson deleted the remove-absorb-pan-events-on-scrollables-option branch April 3, 2023 09:39
@JaffaKetchup JaffaKetchup linked an issue Apr 4, 2023 that may be closed by this pull request
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Remove absorbPanEventsOnScrollables flag.
3 participants