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

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

Merged
merged 2 commits into from
May 6, 2023

Conversation

st-h
Copy link
Contributor

@st-h st-h commented Nov 28, 2022

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
Copy link
Contributor Author

st-h commented Apr 21, 2023

Are there any issues preventing this from being merged?

@NullVoxPopuli
Copy link
Contributor

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

@st-h
Copy link
Contributor Author

st-h commented Apr 21, 2023

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

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Apr 21, 2023

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

@gilest
Copy link

gilest commented Apr 25, 2023

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:

unpatched.mov
patched.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
Copy link
Contributor Author

st-h commented Apr 25, 2023

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 added a commit to st-h/ember-sortable that referenced this pull request Apr 25, 2023
@st-h
Copy link
Contributor Author

st-h commented Apr 25, 2023

@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
Copy link
Contributor Author

st-h commented Apr 25, 2023

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
Copy link

gilest commented Apr 26, 2023

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

fixed.mov

@st-h
Copy link
Contributor Author

st-h commented May 6, 2023

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

@gilest
Copy link

gilest commented May 6, 2023

@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
Copy link
Contributor

Yup. Seems good to me, thanks!

@NullVoxPopuli NullVoxPopuli merged commit 8536901 into adopted-ember-addons:master May 6, 2023
@github-actions github-actions bot mentioned this pull request Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants