sveltejs / svelte

Cybernetically enhanced web apps
https://svelte.dev
MIT License
78.84k stars 4.14k forks source link

PreventDefault on touchstart not working! #12639

Closed legendSabbir closed 2 months ago

legendSabbir commented 2 months ago

Describe the bug

<input type="text">
<button ontouchstart={(e) => e.preventDefault()}>click</button>

While the input has focus clicking the button on a touch device shouldn't take the focus away from the input . In svelte 4 it is working fine . I'm guessing under the hood passive: true is added .

Severity

annoyance

trueadm commented 2 months ago

This is intentional because we now use event delegation, if you use on:touchstart as per Svelte 4, it should work fine and thus isn't a breaking change. Although I think we're missing documentation around this. You can additionally just create your own event handler using on from svelte/events and handle that in an action or an effect.

huntabyte commented 2 months ago

@trueadm this works as expected? REPL

trueadm commented 2 months ago

@trueadm this works as expected? REPL

We specifically made PassiveDelegatedEvents for ['touchstart', 'touchmove', 'touchend']. Maybe we should revise that list though.

legendSabbir commented 2 months ago

This is intentional because we now use event delegation, if you use on:touchstart as per Svelte 4, it should work fine and thus isn't a breaking change. Although I think we're missing documentation around this. You can additionally just create your own event handler using on from svelte/events and handle that in an action or an effect.

The issue still exists even if i use old syntax (on:touchstart) on svelte-5 . As a workaround i can do this btn.addEventListener('touchstart', (e) => e.preventDefault(), { passive: true }) but this not ideal

trueadm commented 2 months ago

This is intentional because we now use event delegation, if you use on:touchstart as per Svelte 4, it should work fine and thus isn't a breaking change. Although I think we're missing documentation around this. You can additionally just create your own event handler using on from svelte/events and handle that in an action or an effect.

The issue still exists even if i use old syntax (on:touchstart) on svelte-5 . As a workaround i can do this btn.addEventListener('touchstart', (e) => e.preventDefault(), { passive: true }) but this not ideal

What are you adding on:touchstart to? It seems onpointerdown works for me?

Edit: you deleted your comment?

legendSabbir commented 2 months ago

The issue still exists even if i use old syntax (on:touchstart) on svelte-5 . As a workaround i can do this btn.addEventListener('touchstart', (e) => e.preventDefault(), { passive: true }) but this not ideal

What are you adding on:touchstart to? It works fine for me locally, as does onpointerdown?

Edit: you deleted your comment?

https://github.com/user-attachments/assets/3aa3af5f-3fa6-4724-9778-77ffc85b214a On click of the button they input loses focus , In svelte 4 repl its working fine

trueadm commented 2 months ago

Nice catch. I have a PR that fixes the on: version of this, https://github.com/sveltejs/svelte/pull/12645

dummdidumm commented 2 months ago

Reopening since we still need to document this for runes mode (and/or decide if the default passive is the right move)

TheRealSeber commented 2 months ago

Right now I have the following situation:

I have a html canvas element which should work both on mouse events and touch events. I need to prevent default behaviour to avoid scrolling the screen when touching the canvas. According to @trueadm, this would imply that I have to use on: version, but it at the same time this causes Mixing old (on:touchstart) and new syntaxes for event handling is not allowed. Use only the ontouchstart syntaxsvelte error, if someone would like to use new event delegation system. On the other hand I can create a special action to handle that stuff when usingontouchstart/ontouchmove/ontouchend just to adjust the passive value (I don't know how it should look like, since I am not that experienced dev, so if you don't mind providing an example I would be grateful) .

In summary, I feel this overcomplicate achiving the goal, which is properly working component in Svelte 5.

legendSabbir commented 2 months ago

Nice catch. I have a PR that fixes the on: version of this, #12645

The issue isn't fixed . Here is a REPL . Please test on a mobile device .

Here is a screenrecord from mobile

https://github.com/user-attachments/assets/b84bcd6e-57b1-4d8c-a076-bddee2a54f26

trueadm commented 2 months ago

Nice catch. I have a PR that fixes the on: version of this, #12645

The issue isn't fixed . Here is a REPL . Please test on a mobile device .

Here is a screenrecord from mobile

SVID_20240804_105430_1.mp4

I’ll investigate tomorrow on my Android device.

Update: landed a PR that should fix this particular issue.

Rich-Harris commented 2 months ago

Noticed that we're not currently making wheel events passive (demo), only touch events

trueadm commented 2 months ago

@Rich-Harris This used to work? Let me take a look.