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

Emit event when cooperative gestures blocks event #4470

Conversation

tomhicks
Copy link
Contributor

@tomhicks tomhicks commented Jul 30, 2024

Launch Checklist

Fixes #4464

Adds the ability to subscribe to the case when the CooperativeGestures handler prevents a gesture from being handled by the map. This allows users to add custom logic in this case to put up custom UI, for instance.

The events emitted are of type cooperativegestureprevented and have a gestureType: "wheel_zoom" | "touch_pan" and originalEvent: Event properties, so the developer has as much detail as possible. Arguably, gestureType can be deduced from the originalEvent but it feels nice to explain the gesture type in MapLibre-ish terms.

This also refactors the way in which coop gesture preventions are detected and notified. It wasn't obvious to me when I was working on this how the coop gesture class was preventing the gestures from being handled/bubbled/whatever. It took a while to realise that the touchmove and wheel events in this are run in "parallel" to the other gesture handlers.

The refactor makes the coop gesture handler have two distinct responsibilities:

  • answering questions about whether a gesture should be prevented
  • notifying the user of a prevention, when requested

This avoids duplicate logic and makes it easy to find how this module is used elsewhere as it has no intrinsic abilities: it performs logic and actions when requested by other modules.

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Add an entry to CHANGELOG.md under the ## main section.

@tomhicks
Copy link
Contributor Author

I have not been able to find where I should add this extra event to the documentation. If someone can tell me how to do that I would happily do it!

@codecov-commenter
Copy link

codecov-commenter commented Jul 30, 2024

Codecov Report

Attention: Patch coverage is 96.77419% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.84%. Comparing base (bf008aa) to head (0c0e1ce).

Files Patch % Lines
src/ui/handler/cooperative_gestures.ts 87.50% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4470      +/-   ##
==========================================
+ Coverage   87.53%   87.84%   +0.31%     
==========================================
  Files         246      246              
  Lines       33446    33450       +4     
  Branches     2212     2218       +6     
==========================================
+ Hits        29276    29384     +108     
+ Misses       3153     3055      -98     
+ Partials     1017     1011       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tomhicks tomhicks force-pushed the emit-event-when-cooperative-gestures-blocks-event branch 2 times, most recently from e6268c4 to 1f30526 Compare July 30, 2024 12:00
@HarelM
Copy link
Collaborator

HarelM commented Jul 30, 2024

I still don't understand how raising these events will allow replacing the current UI.
I don't mind adding these events, but I'm lacking a proper motivation.

@tomhicks
Copy link
Contributor Author

I still don't understand how raising these events will allow replacing the current UI.

.maplibre-cooperative-gestures-container {
  display: none !important;
}
map.on("cooperativegestureprevented", () => showToast("Press Ctrl"));
map.on("cooperativegestureprevented", () => someObservableState.gesturePrevented = Date.now());
map.on("cooperativegestureprevented", () => analytics.sendEvent("user_tried_to_scroll_map"));

I can do whatever I want with this event, including putting my own UI on top of MapLibre, in my host app, sending things to analytics, etc.

@HarelM
Copy link
Collaborator

HarelM commented Jul 30, 2024

I see, thanks for the info!
Can you please document this event in the handlers' doc comment?

@tomhicks tomhicks force-pushed the emit-event-when-cooperative-gestures-blocks-event branch from 1f30526 to 0c5915f Compare July 31, 2024 11:01
@tomhicks
Copy link
Contributor Author

OK, documented that and put the logic back to the individual handlers so things like touch counts, etc are all in the concerned handler.

Previously, the logic for preventing gestures and notifying the
user about those gestures was duplicated. Firstly, we checked if
the gesture should go through by things like counting touches in
other handlers. We would also check the same thing in the CoopGesture
handler class, too. This was a source of confusion for me as it
wasn't apparent how the Handler was able to prevent other handlers
from firing - the answer is that it wasn't and the the other
handlers had to check with the CooperativeGestureHandler themselves.

Now, this is made explicit as the logic for checking only exists
in one place, and the other handlers are responsible for asking
the CooperativeGestureHandler to notify the user about the fact
that the gesture was blocked.
@tomhicks tomhicks force-pushed the emit-event-when-cooperative-gestures-blocks-event branch from beac456 to ab393e1 Compare July 31, 2024 11:37
@HarelM
Copy link
Collaborator

HarelM commented Aug 1, 2024

Hi, please note that the following was not updated with this logic:
https://github.com/tomhicks/maplibre-gl-js/blob/619773155b3126a453cfded634b35ae8d506fc8d/src/ui/handler/two_fingers_touch.ts#L289

I'm not sure what should be the solution there, but please make sure to test all scenarios to make sure nothing is broken.
I have a feeling not all gesture preventions is covered by tests considering the above "miss".

I have updated some small parts of the code, just small refactoring...

@tomhicks
Copy link
Contributor Author

tomhicks commented Aug 2, 2024

Hi, please note that the following was not updated with this logic

@HarelM I'm not sure what change you are expecting there. The logic has not and should not change.

@HarelM
Copy link
Collaborator

HarelM commented Aug 2, 2024

There's no event in that case, and since you remove the move event from cooperative gesture class you broke part of this logic.
Did you test this code with touch devices (dev tool mobile mode)?

@tomhicks
Copy link
Contributor Author

tomhicks commented Aug 2, 2024

There's no event in that case, and since you remove the move event from cooperative gesture class you broke part of this logic.

I'm not sure what part of the logic is broken. What I moved is the code that shows the message if the number of touches is exactly equal to 1, which is now in touch_pan, because it relates to touch panning. There was never any message shown (and now there should be no event emitted) when using 2 fingers in an upwards motion (which in non-coop mode means pitch, but in coop mode means pan) because although it's changing the nature of the gesture, it's not blocking it per se.

Did you test this code with touch devices (dev tool mobile mode)?

Yes, and it all seems to work exactly as on main, and using a real mobile device and multiple fingers, too.

The behaviour on touch devices is:

normal mode:

  • 1 finger drag: pan
  • 2 fingers spin: rotate
  • 2 fingers vertical drag: pitch
  • 2 fingers non-vertical pan: pan

coop mode:

  • 1 finger drag: blocked -> show UI, emit event
  • 2 fingers spin: rotate
  • 2 fingers vertical drag: pan
  • 2 fingers non-vertical drag: pan
  • 3 fingers vertical drag: pitch

That's how it is on main and that's how it is on this branch.

@tomhicks
Copy link
Contributor Author

tomhicks commented Aug 2, 2024

One thing we could do would be to emit a cooperativegesturechanged and describe the change in that case, i.e. to convey "you might be expecting this two-finger vertical drag to result in a pitch gesture, but you actually need to use 3"

@HarelM
Copy link
Collaborator

HarelM commented Aug 2, 2024

Thanks for the clarification, I probably got it wrong, sorry.

@HarelM HarelM enabled auto-merge (squash) August 3, 2024 06:27
@HarelM HarelM merged commit e106c24 into maplibre:main Aug 3, 2024
15 checks passed
@tomhicks tomhicks deleted the emit-event-when-cooperative-gestures-blocks-event branch August 4, 2024 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fire events on cooperative gesture triggers
3 participants