adopted-ember-addons / ember-sortable

Sortable UI primitives for Ember.js
https://ember-sortable.netlify.app/
MIT License
298 stars 148 forks source link

when sortable is disabled, scrolling is blocked on ios #506

Closed st-h closed 1 year ago

st-h commented 1 year ago

I just came across a bug on iOS when sorting is disabled. When the scroll handle is touched, scrolling would suddenly no longer work at all. This is particularly problematic when the handle is the same element as the sortable item. Then the only way to scroll is by touching the background between elements (in case it is visible).

It turned out that this is caused by the css style touch-action: none.

This PR sets touch-action to initial when sorting is disabled, so that the list is scrollable again without issues.

st-h commented 1 year ago

Are there any issues preventing this from being merged?

NullVoxPopuli commented 1 year ago

I don't have an iOS device to test on, personally

st-h commented 1 year ago

@NullVoxPopuli thanks for the information. How do we address this then?

NullVoxPopuli commented 1 year ago

I posted in discord about this pr, hoping to get someone to review soon

gilest commented 1 year ago

Hey @st-h I'm struggling to understand exactly how to test this:

When the scroll handle is touched, scrolling would suddenly no longer work at all.

What exaclty is the scroll handle? Is it a sortable element or part of the iOS UI?

Anyway, I had a go by pausing the test runner here ( within Safari within the iOS Simluator. Here's a recording of what it looks like:

https://user-images.githubusercontent.com/36919/234255316-8ff96d2b-6488-4c7d-9dec-792e18b9ece4.mov

https://user-images.githubusercontent.com/36919/234254803-0adc9291-228e-437f-8b85-60140a5df47b.mov

With both the patched and unpatched version I couldn't reproduce "scrolling would suddenly no longer work at all" but dragging on the sortable items does not cause any scroll.

Hope this helps 🤷🏻

st-h commented 1 year ago

Sorry about the confusion. This was meant to say sortable-handle. The handle within the sortable element. This add-on sets the style touch-action to none, which disables touch action events. Therefore this also disables scrolling. I had a patched version of the dummy app, which replicated this bug, but I revered that a few weeks ago, not thinking I would need it again. I can try to put something to reproduce together today.

st-h commented 1 year ago

@gilest please give the test-app in https://github.com/st-h/ember-sortable/tree/reproduce-ios-bug a try. The dummy app has a section "scrollable". Please try to touch the handle on the right side and try to scroll the list. All touch events are blocked on the handle. This is a huge issue when the whole scrollable item is a scroll handle. Then this bugs prevents scrolling the entire list. This is caused by setting touch-action to 'none'.

Screenshot 2023-04-25 at 13 41 16

st-h commented 1 year ago

I just tried and the issue also is reproducible in simulator: https://user-images.githubusercontent.com/5768353/234270544-7e8ac442-b87d-4961-bc8b-dd009c99d11b.mov

gilest commented 1 year ago

Thanks for the repro @st-h I can confirm that this fixes the scroll-blocking issue when testing in iOS simulator.

https://user-images.githubusercontent.com/36919/234493462-ce4ce049-10af-436e-b0ce-3531fec773c6.mov

st-h commented 1 year ago

@gilest thanks for confirming. Is there anything that keeps us from merging?

gilest commented 1 year ago

@gilest thanks for confirming. Is there anything that keeps us from merging?

Not from my perspective. Will need a maintainer to merge though. @NullVoxPopuli do you have maintainer permission?

NullVoxPopuli commented 1 year ago

Yup. Seems good to me, thanks!