adobe / react-spectrum

A collection of libraries and tools that help you build adaptive, accessible, and robust user experiences.
https://react-spectrum.adobe.com
Apache License 2.0
12.54k stars 1.09k forks source link

Button onPress event on iOS can select the wrong element when virtual keyboard is open #6512

Open subvertallchris opened 3 months ago

subvertallchris commented 3 months ago

Provide a general summary of the issue here

On iOS, the Button's onPress function will dismiss a virtual keyboard immediately at the start of the press. If the onscreen keyboard is open, this can trigger a change of viewport height, at which point the end of the press can select an entirely different element than what the user intended.

🤔 Expected Behavior?

The react-aria-components button should behave like the native button element: do not dismiss the virtual keyboard at the start of the press.

😯 Current Behavior

On iOS, if the onscreen keyboard is open, it will be dismissed immediately at the start of a press event.

💁 Possible Solution

I can't offer a good explanation for why it happens or how to fix it. I'm more concerned with the wrong element being clicked than the keyboard disappearing. Perhaps it's possible to detect a viewport size change in the middle of a click and adapt to it?

🔦 Context

This caused a major incident for us. On one version of our cart checkout page, if a user on an iPhone SE tries to click "Continue" before closing their virtual keyboard, the viewport will move and they'll click on "Remove from cart" instead, which will at best remove one item and at worst surprisingly cancel the entire order. It's a real fax-machine-into-shredder moment and very serious. For the time being I am going to adjust the page so users can't stumble into trouble but I'm considering moving away from this Button component entirely because I don't know where or when else a similar problem might occur.

🖥️ Steps to Reproduce

This is somewhat tricky to reproduce but there is a sandbox here https://codesandbox.io/p/sandbox/currying-cookies-g8fms9. There's also a video I captured from my simulator here https://www.dropbox.com/scl/fi/c0ekpyg7hklzuy3eu3y0w/react-aria-button-viewport-shift.mp4?rlkey=tcrdt2g0e4nvm2vmpnztzizte&dl=0.

To reproduce it, use the iOS Simulator and power up an iPhone SE 3rd Edition. I'm testing using iOS 17 but the reproduction in the wild uses iOS 16.

Now try that again but use the "native press me" button. You'll see that the virtual keyboard is not dismissed and as a result the click occurs with a shift in viewport height.

Version

1.2.1

What browsers are you seeing the problem on?

Safari

If other, please specify.

No response

What operating system are you using?

iOS 16 and 17

🧢 Your Company/Team

No response

🕷 Tracking Issue

No response

subvertallchris commented 3 months ago

This reads as having the same root cause as a similar issue, https://github.com/adobe/react-spectrum/issues/1497#issuecomment-768495097. It's been three years and hopefully this can be discussed and reevaluated.

devongovett commented 1 month ago

I tried many times but I couldn't reproduce this. Does it still happen for you?

In theory I guess what could happen is that focus moves to the button on pointer down, the keyboard hides, and then you have something that triggers on pointer up. This can also occur in other cases, such as if a modal or popover opens on pointer down revealing another element on top of it that triggers on pointer up. Usually the way to prevent this is by checking that a pointer up event was preceded by a pointer down event on the same element. Our press events do this for you already, but if you have other elements with pointer up handlers you might need to do the same check there too.

We cannot change focus to occur on pointer up unfortunately. That would break many behaviors that rely on focus moving on pointer down, such as focus restoration from popovers. This is required to normalize the behavior across browsers.

subvertallchris commented 1 month ago

Hi @devongovett , thanks for checking. I just reproduced it using the latest React Aria Components and an iPhone SE (3rd Generation) in the iOS Simulator. The sandbox was built for that model because this bug relies on a bad combination of element distance + screen size. It can and does happen on other devices but you need that perfect storm to trigger it. This video in my issue demonstrates it perfectly https://www.dropbox.com/scl/fi/c0ekpyg7hklzuy3eu3y0w/react-aria-button-viewport-shift.mp4?rlkey=tcrdt2g0e4nvm2vmpnztzizte&dl=0

In the very embarrassing case that our users encountered in production, it was an attempt to click a React Aria Components Button that ultimately clicked a React Aria Components Link that was owned by a sibling component on the page. There's no special handling for pointer up events in this case. Can you please provide an example of what you think we might have to do to avoid this?

We cannot change focus to occur on pointer up unfortunately.

I don't understand this. The problem here is that the pointer down event is dismissing the keyboard but not actually targeting the button. It behaves correctly with native HTML buttons on iOS: you can click the button with the keyboard open and it targets correctly.

We've had other problems related to React Aria Components handling of pointer down and up events, especially where iOS is concerned. It has shaken our confidence in the library tremendously. While I appreciate the pursuit of normalizing behavior across browsers, I worry that the result is often suboptimal defaults and surprising experiences. We have recently removed our Buttons, Modal + Dialog, Tooltip. Select is on the way out. I expect others will follow.

devongovett commented 1 month ago

Can you reproduce on a real device? I got it lined up and the link doesn't trigger for me on my iPhone. I'll see if anyone else can reproduce it.

I don't understand this.

Just using the native behavior causes different problems. You can read more about it here: https://react-spectrum.adobe.com/blog/building-a-button-part-3.html#ensuring-consistent-focus-behavior Unfortunately, I'm not sure there's a way to move focus but keep the keyboard open.

Link might be different since the click would be handled natively by the browser. I'm a bit surprised that they would record a click on an element as occurring when pointer down occurred on a different element - that seems like a potential browser bug. We can look into if there's anything we can do to about it.

subvertallchris commented 1 month ago

Reproduced on my iPhone 12 just now. Here’s a video. https://www.dropbox.com/scl/fi/kp5whxmpji18tijz2j6km/iphone-react-aria-components-button.MP4?rlkey=krozj6sdnrou5ilxzwr5p74n7&dl=0

In this case it’s a React Aria Components button but it’s triggering an a tag with an onClick handler. I increased the font size to make it easier reproduce but it still took a few tries, then it was easier once I got the knack for it.

When trying to reproduce, you need to keep pressing down, don’t just tap and release. Press and hold, the keyboard will close and the scroll position will change, then release.

I'm not sure there's a way to move focus but keep the keyboard open.

Is moving focus the same as triggering the button’s click event? If you use the same sandbox and try to reproduce with the native button element, it’s not closing the keyboard on press start, which suggests it’s not changing focus there. I don’t know enough about what’s happening under the hood to speculate whether it’s some magic iOS Safari thing, or an implementation decision on the React Aria side.

devongovett commented 1 month ago

Ah, I see it now. Thanks for making the hit target larger, that seems to make it easier to reproduce. We can look into it.

subvertallchris commented 1 month ago

Thank you! This is super high priority for us, it led to lost revenue and lost confidence from users. I really appreciate your work and I'm happy to contribute to an improvement if I can.