inconsistent isPressed state when focus is moved to another element

[deebov]

🐛 Bug Report

We have a Select component that uses react-aria's hooks. When the select popover opens we move the focus to another dom element inside the popover. When we move the focus to another element like this, button's isPressed stays true.

🤔 Expected Behavior

isPressed should be false when we change the focus from button to another element

😯 Current Behavior

button's isPressed state stays true when focus is moved to another dom element. And since isPressed true button will ignore press/click event triggers.

🔦 Context

we have a components library that uses react-aria

💻 Code Sample

https://codesandbox.io/s/naughty-blackwell-nnsxu?file=/src/App.tsx

[LFDanLu]

@deebov I tried your codesandbox and it seems to toggle the input element appearance on/off just fine. The console.logs seem to indicate that isPressed is being set to false on mouse up as well.

[deebov]

@deebov I tried your codesandbox and it seems to toggle the input element appearance on/off just fine. The console.logs seem to indicate that isPressed is being set to false on mouse up as well.

oh sorry, I forgot to be more specific.

1. Refresh the page
2. Set the focus to button with Tab
3. Press Space
4. Click the button with mouse and it should ignore the click

[snowystinger]

Looks like we have something that's supposed to be dealing with that https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/interactions/src/usePress.ts#L226 but isn't, I'd start debugging here

[LFDanLu]

Thanks for the repro steps @deebov, will need to investigate why that line that @snowystinger linked doesn't seem to be handling this situation properly

[alirezamirian]

Experiencing the same issue. A similar re-production scenario sandbox: https://codesandbox.io/s/react-spectrum-template-forked-vy3n1m?file=/src/App.js

Did some digging and here is the result:

When the input is focused immediately after keydown, keyup happens on the input. To handle this case, onKeyUp handler is set on document (probably what @snowystinger linked too as well). And there is even a test case for that: https://github.com/adobe/react-spectrum/blob/a162e5b235fcdc79087ea6d64c726664dc9d4179/packages/%40react-aria/interactions/test/usePress.test.js#L2071

BUT, in that test case, keyup is triggered on body. It's different when it's triggered on an input, because isValidKeyboardEvent here would return false in that case: https://github.com/adobe/react-spectrum/blob/9498c7d858b08e03b4c36fac491b9e4434453da0/packages/%40react-aria/interactions/src/usePress.ts#L292

And state.isPressed would not reset to false.

I adjusted the test case in this branch to cover this case too: https://github.com/adobe/react-spectrum/commit/e5740c3971986932226325b046f9fbb765c812ea

Removing isValidKeyboardEvent check in onKeyUp would pass the updated test without breaking any other test case, but I don't have a good overview of usePress and am not sure if everything in that if block should run in this case.

---

UPDATE: I just realized @devongovett pushed an apparently unrelated commit yesterday that seems to fix this issue. At least it fixes the issue with getting stuck in isPressed state. The modified test also passes with that change. But it breaks the codesandbox example mentioned above by not triggering onPress in this case. Maybe that's the right behavior, and in that example, the state should be toggled in onPressStart or onPressEnd, if it's expected to have onPressStart and onPressEnd triggered but not onPress. Here is a fork of that sandbox, using the latest nightly: https://codesandbox.io/s/react-spectrum-template-forked-jbumgf?file=/src/App.js

[alirezamirian]

I can still open a small PR from https://github.com/adobe/react-spectrum/commit/e5740c3971986932226325b046f9fbb765c812ea to adjust the existing test case to cover this scenario. Let me know if it makes sense.

[snowystinger]

We'd love it if you contributed a test. The better and more accurate our tests the less likely we are to regress and that's fantastic