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

Setting a Pressable as "disabled" from enter key press locks its internal focused state as true #2357

Closed
1 task done
AlanSl opened this issue Aug 8, 2022 · 5 comments · Fixed by #2374
Closed
1 task done
Labels
Milestone

Comments

@AlanSl
Copy link

AlanSl commented Aug 8, 2022

Is there an existing issue for this?

  • I have searched the existing issues

Might be a side effect of the fix for #2127

Describe the issue

If a Pressable's onPress function causes it to become disabled, and this is triggered by tabbing in with the keyboard and triggering the onPress function from the keyboard using the Enter key, then the Pressable's internal focused state becomes locked as true even after the actual HTML element has blurred.

For example, here, the first Pressable was disabled and blurred using the Enter key and the second Pressable is the focused element, but the first Pressable thinks it is focused still.

image

Expected behavior

A Pressable's focused state should not be true if it is not focused, regardless of how it came to be not focused, and it should never be both disabled and focused.

Steps to reproduce

  • Create a Pressable that shows its internal focused state visibly
  • Set its disabled state to true in an onPress handler
  • Select it using the keyboard tab key and trigger the onPress handler with the Enter key

Test case

https://codesandbox.io/s/quirky-williamson-v3r94g
https://snack.expo.dev/@alansl/amused-cheese

Additional comments

There's also a possibly-related issue described in comments in the linked example, where it's possible to programmatically focus a disabled Pressable just before disabled is removed, and then when disabled is removed, the element is focused but the focused state is stuck as false. Which issue occurs depends on whether mouse or keyboard events are used and timing of when the focus switch happens. Suggests the issue hinges on the timing of rerenders triggered by different event types.

In the provided example, it's almost possible to work around this issue with careful timing - programmatically blurring the element after setting disabled to false in the target but before setting it to true in the trigger element. But this creates some strange timing issues when mouse clicks are used. Also, in real-life examples, this may not be an option: for example if the elements' disabled state is controlled by one selected state, it may not be possible to set a moment when both are enabled.

Suggested fix

It looks like all focus / blur handling is suppressed as soon as disabled is applied, and the timing of this relative to event handler timing varies by event type. Suggestion:

  • When disabled flips from false to true, if focused is still true, it shouldn't be and the element is losing focus; so call onBlur (including setting internal state) while it still can.
  • When disabled flips from true to false, if the element has become actually focused in the DOM but focused is false, it is legitimately gaining focus but the event handlers were skipped due to event / re-render timing; so call onFocus (including setting internal state)
@AlanSl AlanSl added the bug label Aug 8, 2022
@AlanSl
Copy link
Author

AlanSl commented Aug 8, 2022

Best workaround for my particular case so far seems to be to control an isFocused state in the Pressable's parent using onBlur / onFocus and not allow disabled to be true if isFocused is true; so, regardless of the timing around the triggering event, if an action causes the parent's disabled prop to be true on next render and moves focus away, it will not apply disabled as true until the re-render caused by the onBlur handler setting isFocused to false.

@necolas
Copy link
Owner

necolas commented Aug 10, 2022

Test case should be a codesandbox (as asked for in the template) using the latest published version

@AlanSl
Copy link
Author

AlanSl commented Aug 11, 2022

@necolas
Copy link
Owner

necolas commented Aug 11, 2022

If there's a mandatory format and template, may I suggest linking to it in the issue template

Screenshot_20220811-081834_Chrome

@necolas
Copy link
Owner

necolas commented Aug 24, 2022

try this patch #2374

rnike pushed a commit to VeryBuy/react-native-web that referenced this issue Sep 13, 2022
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 a pull request may close this issue.

2 participants