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

Mobile Item Selection Fails Starting from Version 3.22.3 of @react-aria/interactions #7157

Closed
elianarlivingston opened this issue Oct 7, 2024 · 19 comments · Fixed by #7190
Closed
Assignees
Labels
bug Something isn't working

Comments

@elianarlivingston
Copy link

elianarlivingston commented Oct 7, 2024

Provide a general summary of the issue here

Since version 3.22.3 of the @react-aria/interactions package, item selection functionality on mobile devices has stopped working correctly. This issue arises when using the headless version of the @react-aria/listbox, which depends on @react-aria/interactions. The problem was not present in version 3.22.2.

🤔 Expected Behavior?

Items should be selectable on mobile devices.

😯 Current Behavior

Selection fails or does not respond on mobile devices.

💁 Possible Solution

Suspected #7026

🔦 Context

No response

🖥️ Steps to Reproduce

Codesandbox

Version

@react-aria/[email protected]

What browsers are you seeing the problem on?

Chrome, Safari, FireFox

If other, please specify.

No response

What operating system are you using?

Mac

🧢 Your Company/Team

No response

🕷 Tracking Issue

No response

@elianarlivingston elianarlivingston changed the title Item Selection on Mobile Breaks Starting from Version 3.22.3 in @react-aria/interactions Mobile Item Selection Fails Starting from Version 3.22.3 of @react-aria/interactions Oct 7, 2024
@shift4id
Copy link

shift4id commented Oct 8, 2024

It appears that this issue is also affecting all pressable elements.

Button and Link started having unreliable behavior in this version. Takes several taps to get them to work.

@LFDanLu
Copy link
Member

LFDanLu commented Oct 8, 2024

I've reproduced this on my phone as well, will need to dig to see what exactly is going on. @shift4id Are these React Aria Component/React Spectrum Buttons and Links that are having problems or ones create from the hooks directly?

@shift4id
Copy link

shift4id commented Oct 8, 2024

@LFDanLu — I'm using react-aria-components. Not sure if the issue exists for components created from the hooks, but I assume it will still be there as it seems to be coming from @react-aria/interactions.

@andreslemusm
Copy link

andreslemusm commented Oct 10, 2024

I'm experiencing the same with [email protected]. On mobile devices, the Link component ignores clicks to child SVG elements.

Codesandbox

@AnYiEE
Copy link

AnYiEE commented Oct 11, 2024

I also encountered this issue. The problem is that the click event on mobile devices does not work. When I reverted to version 3.22.2, everything worked fine.

I used NextUI's Tooltip component to wrap a span element with a click event listener, as shown below:

<Tooltip content="content">
	<span
		onClick={() => {}}
		onKeyDown={() => {}}
		aria-label="aria-label"
		role="button"
		tabIndex={0}
		className="custom-styles"
	/>
</Tooltip>

Currently, my temp solution is using pnpm overrides:

"pnpm": {
	"overrides": {
		"@react-aria/interactions@^3.22.3": "3.22.2"
	}
}

AnYiEE added a commit to AnYiEE/touhou-mystia-izakaya-assistant that referenced this issue Oct 11, 2024
build: update dependencies
Add override for `@react-aria/interactions` version 3.22.3 -> 3.22.2,
until adobe/react-spectrum#7157 is resolved.
@LFDanLu
Copy link
Member

LFDanLu commented Oct 12, 2024

Thank you for the additional examples everyone. I did some digging and this seems to be the problematic line:

if (shouldPreventDefaultUp(e.target as Element)) {
e.preventDefault();
}
. In particular, the target that is detected for these examples are the li/path within the svg/etc which then hits the preventDefault because they fail this check.

@elianarlivingston For your example it could perhaps be argued that handleClose should be provided as onAction to the ListBox as a whole, upon which it will properly close since it takes a different path in the usePress code rather than relying on onClick set directly on the list item and thus will also benefit from the interaction normalization that usePress aims to provide. However, the Link not triggering in @andreslemusm 's example is definitely problematic, ideally we would detect that we are pressing on a direct child of the anchor and treat it as a valid press on the link

@devongovett
Copy link
Member

That should have been currentTarget rather than target.

@LFDanLu
Copy link
Member

LFDanLu commented Oct 14, 2024

Opened #7190 to fix this for Links. However, this will not change the current behavior where onClick is not triggering when applied to an element that uses the pressProps from usePress or whose aria hook calls usePress under the hood (aka useOption).

@AnYiEE I'm not entirely familiar with NextUI's tooltip but I what I can see from their implementation, the span you show in your example is the tooltip trigger element correct? If so, from what I can tell there shouldn't be anything from usePress making it onto that trigger and preventing the on click from working since they are just passing the tooltipTrigger props down. I've tried to repoduce via https://codesandbox.io/p/sandbox/autumn-paper-pc6kjz?file=%2Fsrc%2FApp.js%3A10%2C5 but the onClick seems to work just fine, mind creating a reproduction?

@LFDanLu LFDanLu added the bug Something isn't working label Oct 14, 2024
@github-project-automation github-project-automation bot moved this to ✏️ To Groom in RSP Component Milestones Oct 14, 2024
@LFDanLu LFDanLu self-assigned this Oct 14, 2024
@LFDanLu LFDanLu moved this from ✏️ To Groom to 🏗 In Progress in RSP Component Milestones Oct 14, 2024
@github-project-automation github-project-automation bot moved this from 🏗 In Progress to ✅ Done in RSP Component Milestones Oct 14, 2024
@AnYiEE
Copy link

AnYiEE commented Oct 14, 2024

@LFDanLu

Hello, I'm trying to import my existing repository on codesandbox.io, but it failed to compile, reporting that a dependency cannot find a file it needs. If you have time, could you try cloning my repository locally to reproduce the problem (I just tried and can still reproduce it).

Here are the steps to reproduce:

  1. Clone touhou-mystia-izakaya-assistant
  2. Switch tag to v1.3.3 (because I fixed the version of @react-aria/interactions at v1.3.4, and there is only one change between v1.3.3 and v1.3.4)
  3. Add 127.0.0.1 test.com in the local hosts file, yes, accessing directly through 127.0.0.1 cannot be reproduced, I don't know why. Also because of this, I did not notice this issue locally at that time until it was deployed in production under the domain name later found out.
  4. Create an .env.local file and write SELF_HOSTED=true
  5. Run pnpm i && pnpm dev
  6. Access http://test.com:3000/customer-rare
  7. Click the first button in the dialog to skip the process guide, then click on the first green avatar, then click on the second tab "料理"
  8. Click on the icon in row one column three of the table (green seaweed), clicking will open a new window
  9. Open console simulating mobile device. Repeats step 8 - Unable properly open new window after clicking
  10. Stop development server, delete node_modules folder, switch branch master (or tag v1.3.4)
  11. Repeat steps 5-9 on mobile devices successfully triggers opening the new window

More details: The version changes of @react-aria/interactions occurred between v1.2.14 and v1.2.15, upgrading from 3.22.2 to 3.22.3, so this issue starts from v1.2.15 until v1.3.3. Related source code: recipeTabContent.tsx#L30 sprite.tsx#L98

@elianarlivingston
Copy link
Author

elianarlivingston commented Oct 15, 2024

Gracias a todos por los ejemplos adicionales. He investigado un poco y esta parece ser la línea problemática:

if (shouldPreventDefaultUp(e.target as Element)) {
e.preventDefault();
}

En particular, el objetivo que se detecta para estos ejemplos es el li/path dentro de svg/etc que luego alcanza el preventDefault porque no pasa esta verificación.
@elianarlivingstonPara su ejemplo, tal vez se podría argumentar que handleClosese debería proporcionar en cuanto onActional ListBox en su totalidad, sobre el cual se cerrará correctamente ya que toma una ruta diferente en el usePresscódigo en lugar de depender del onClickconjunto directamente en el elemento de la lista y, por lo tanto, también se beneficiará de la normalización de interacción que usePressse pretende proporcionar. Sin embargo, el enlace no se activa en@andreslemusmEl ejemplo de es definitivamente problemático, idealmente detectaríamos que estamos presionando sobre un hijo directo del ancla y lo trataríamos como una presión válida sobre el enlace.

Hi! Thank you so much for the help.

We're also a library, and we need to access the click event as it's part of our current public API. For this reason, it's crucial for us to be able to intercept the event when using onAction.

Is there any way to directly access the click event from onAction without compromising the existing functionality? We would really appreciate any suggestions that help us maintain compatibility with our API.

Thanks again for the support!

@snowystinger
Copy link
Member

@AnYiEE I see in your pnpm lockfile that you have duplicates of some of our packages installed. This can lead to strange bugs. I suggest reducing them all to a single copy and seeing if that fixes your problems.
https://github.com/AnYiEE/touhou-mystia-izakaya-assistant/blob/9f3c5dba095f221f42ea9c6848c73361533911e7/pnpm-lock.yaml#L1817

It's a pretty common issue due to how package managers work because they were originally built for node code, not javascript front ends.

@AnYiEE
Copy link

AnYiEE commented Oct 15, 2024

@snowystinger My project does not directly use @react-aria, so I may not be able to intervene too much in the versions of dependencies of other dependencies, as this would make things complicated. For me, the simplest solution is, as I mentioned in my previous reply, to lock the version of @react-aria/interactions to 3.22.2, and the problem is resolved.

@snowystinger
Copy link
Member

@snowystinger My project does not directly use @react-aria, so I may not be able to intervene too much in the versions of dependencies of other dependencies, as this would make things complicated. For me, the simplest solution is, as I mentioned in my previous reply, to lock the version of @react-aria/interactions to 3.22.2, and the problem is resolved.

You should be able to control it. Either through overrides as you've done or through dedupe tools or by clearing those packages out of your lock and reinstalling. There are probably some other ways to handle it as well.

You'll want to use a bundle analyzer (ie webpack analyzer plugin) to make sure you only have one copy of each package in your bundled code as well.

I'm glad the issue is resolved if you manage the version of @react-aria/interactions. Just a heads up that there was a bug in version 3.22.3, so 3.22.4 is now out.

@snowystinger
Copy link
Member

@elianarlivingston i believe your issue is of a different topic. Please feel free to open a discussion after searching through our existing Issues and Discussions as this topic has come up before in various contexts.

#2100
#6086

@AnYiEE
Copy link

AnYiEE commented Oct 15, 2024

@snowystinger

I tested @react-aria/interactions 3.22.4 locally, and it seems the issue remains, so I kept with 3.22.2.

To me, this should be the simplest solution. @react-aria is a dependency of NextUI. I tried deleting the node_modules folder and pnpm-lock.yaml, then reinstalled with pnpm i. However, different versions of @react-aria/* sub-packages still got installed. I don't want to intervene too much with the dependencies' dependencies versions anymore.

Thank you for your other suggestion, I used pnpm dedupe to removed some duplicate packages.

@LFDanLu
Copy link
Member

LFDanLu commented Oct 17, 2024

@AnYiEE I did some brief digging, it seems to be that the touchEnd event from usePress is happening on the Table's row rather than the span button within, hence causing that touch event to be default prevented. I've reproduced it locally in our own storybook with a native button in a table, but it seems like using a RAC button instead of a native button seems to circumvent this problem

@AnYiEE
Copy link

AnYiEE commented Dec 29, 2024

but it seems like using a RAC button instead of a native button seems to circumvent this problem

@LFDanLu Hello, it's been a while. I have fixed react-aria/interactions to version 2.3.2 using pnpm override, and everything is working fine. Now, I tested the latest @react-aria/interactions 2.3.5, and it still has the same issue.

You mentioned using the RAC button instead of the native button. Could you please elaborate on that? My project does not directly use react-aria or react-spectrum. Do you think I should provide issue to the UI library I am using, or is it an issue with react-aria/interactions itself? After all, version 2.3.2 is available.

@LFDanLu
Copy link
Member

LFDanLu commented Jan 2, 2025

You mentioned using the RAC button instead of the native button. Could you please elaborate on that?

Sure, from what I remember using a React Aria Components Button instead of a native button in a Table locally in our storybook solved the issue where tapping on the button on a mobile device was causing the row to be selected instead of triggering the button's action. The relevant code causing the preventDefault is here, where essentially the touchEnd gets triggered on the table row due to the pointer event propagating through the native button, thus calling preventDefault and stopping the onClick of the native button from working. If you call stopPropagation in onPointerStart for the button in the row, this issue goes away. If possible, are you able to install react-aria-components and see if replacing your problematic icon with a RAC button solves the issue? Alternatively, you've could try calling stopPropagation in onPointerStart for the icon handling the click.

As for where to file an issue, the team will be working some usePress refactoring and making event propagation stoppage more consistent throughout the library which may change the behavior you are seeing here. Feel free to file a separate issue against us with a base level reproduction so we can track if our work there might fix it.

@AnYiEE
Copy link

AnYiEE commented Jan 2, 2025

you've could try calling stopPropagation in onPointerStart for the icon handling the click.

The problem disappeared after calling stopPropagation in onPointerDown.

with a base level reproduction

Unfortunately, it may be difficult for me to provide a minimal reproducible environment. For me, this issue does not even occur on the development server under 127.0.0.1; it only appears when I access it through a domain name (for example, by modifying the hosts file to resolve 127.0.0.1 to test.com), I don't know why. One black box after another makes it hard for me to debug and leaves me completely at a loss. So, thank you for the method you provided, it is very helpful.

AnYiEE added a commit to AnYiEE/touhou-mystia-izakaya-assistant that referenced this issue Jan 2, 2025
…nd stop event propagation

See the issue here for more details: adobe/react-spectrum#7157
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

7 participants