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

Fixing setting focus when RCTView is not yet in window #1398

Merged
merged 7 commits into from
Sep 7, 2022

Conversation

nakambo
Copy link
Collaborator

@nakambo nakambo commented Aug 30, 2022

Please select one of the following

  • I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
  • I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
  • I am making a fix / change for the macOS implementation of react-native
  • I am making a change required for Microsoft usage of react-native

Summary

I have a scenario where I send the focus command to a component from it's componentDidMount lifecycle, which leads native code to try to set focus to a RCTView with a zero frame (okay...) that isn't in a window (also seems unexpected). Not surprisingly, not being in a window means the view can't be focused. For comparison, I have the same behavior working as expected on the Win32\Office flavor of React Native.

I see there's existing code that tries to deal with this for a particular RCTBaseTextInputView, but nothing generically for RCTView. I'm trying to extend that to the RCTView. As part of that, I considered using the existing pending focus flag, but that seems suspect because we could have multiple views competing for focus -- so I am opting to change it to a single, weak ref for the currently pending focus view (which we clear if someone else grabs, or attempts to grab focus, or if the pending view later resigns focus, in addition to the scenario already handled viz. pending focus view gets focus).

Changelog

[macOS/iOS] [Changed] - Extend reactFocusIfNeeded to work with any generic RCTView

Test Plan

I have been able to validate these changes against 0.66 for my scenario.
I have added a test page to RNTester to check before\after (repro before, resolved after)

I have a scenario where I send the `focus` command to a component from it's `componentDidMount` lifecycle, which leads native code to try to set focus to a RCTView with a zero frame (okay...) that isn't in a window (also seems unexpected).

I see there's existing code that tries to deal with this for a particular RCTBaseTextInputView, but nothing generically for RCTView. I'm trying to extend that to the RCTView. As part of that, I considered using the existing pending focus flag, but that seems suspect because we could have multiple views competing for focus -- so I am opting to change it to a single, weak ref for the currently *pending* focus view (which we clear if someone else grabs, or attempts to grab focus, or if the pending view later resigns focus, in addition to the scenario already handled viz. pending focus view gets focus).

I have been able to validate these changes against 0.66 for my scenario.
@nakambo nakambo requested a review from Saadnajmi August 30, 2022 22:29
@nakambo nakambo requested a review from a team as a code owner August 30, 2022 22:29
@Saadnajmi
Copy link
Collaborator

Saadnajmi commented Aug 31, 2022

Two questions:

  1. This feels like something we could upstream to RN Core, now that iOS/iPadOS is more window aware. Maybe we can see what we can do to drive that..

  2. Supposing we can't upstream to RN Core, any objection to keeping the reactFocusIfNeeded naming? My reason for that is I'd like to minimize diffs if possible. It looks like RN Core implemented that logic specifically for since that's probably the only iOS control they actually care about focus for

EDIT: I see now you kept reactFocusIfNeeded, and reactViewDidMoveToWindow is on top of that. So better questions:

  1. Can we upstream the "pendingFocusView" stuff
  2. If not, we'll need to add more //TODO(macOS) tags to mark the diff.

@nakambo
Copy link
Collaborator Author

nakambo commented Sep 1, 2022

Two questions:

  1. This feels like something we could upstream to RN Core, now that iOS/iPadOS is more window aware. Maybe we can see what we can do to drive that..
  2. Supposing we can't upstream to RN Core, any objection to keeping the reactFocusIfNeeded naming? My reason for that is I'd like to minimize diffs if possible. It looks like RN Core implemented that logic specifically for since that's probably the only iOS control they actually care about focus for

EDIT: I see now you kept reactFocusIfNeeded, and reactViewDidMoveToWindow is on top of that. So better questions:

  1. Can we upstream the "pendingFocusView" stuff
  2. If not, we'll need to add more //TODO(macOS) tags to mark the diff.

How about I add the tags for now?

I need this to mitigate a11y concerns for my feature, which can't proceed without these fixes, which is also why for expediency I am making (proposing) these changes to the Microsoft fork.

React/Views/RCTView.m Outdated Show resolved Hide resolved
@nakambo nakambo linked an issue Sep 1, 2022 that may be closed by this pull request
Copy link
Collaborator

@Saadnajmi Saadnajmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wanna double check the diffs with Core, but tested and works fine! Textinput autofocus isn't broken on iOS or macOS, and the new FocusOnMount page works on macOS :)

@nakambo nakambo merged commit 8b8fc61 into main Sep 7, 2022
@nakambo nakambo deleted the user/nakambo/react-focus-if-needed branch September 7, 2022 23:12
nakambo pushed a commit that referenced this pull request Sep 8, 2022
nakambo pushed a commit that referenced this pull request Sep 8, 2022
nakambo added a commit that referenced this pull request Sep 8, 2022
Cherry picking from main branch -- 8b8fc61

Co-authored-by: Navneet Kambo <nakambo@nakambo-mm.guest.corp.microsoft.com>
nakambo added a commit that referenced this pull request Sep 9, 2022
* Fixing setting focus when RCTView is not yet in window (#1398)

Cherry picking from main branch -- 8b8fc61

Co-authored-by: Navneet Kambo <nakambo@nakambo-mm.guest.corp.microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upstream fix: Can't focus component that was just mounted
3 participants