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

ref.focus() doesn't work after 1st render, requires ugly workaround #9292

Closed
luisnaranjo733 opened this issue Dec 16, 2021 · 15 comments
Closed
Labels
Area: Focus bug Needs: Dev Design Partner: Facebook Partner: Xbox (label may be applied by bot based on author) Recommend: Not Planned Recommend that issue should be given Not Planned milestone.

Comments

@luisnaranjo733
Copy link
Member

Problem Description

Calling ref.focus() on a touchable or pressable component doesn't work right away.
But if you set a 1ms timeout and then try again, it works.

More details in the README of repro of this issue I posted

May be related to being a child of a flat list. This doesn't seem to happen in simpler screens that don't have a flat list (i.e. just a plain old screen with a few buttons and manually managed refs). But in the flat list scenario it happens consistently.

Steps To Reproduce

  1. Clone this repository
  2. Launch the app
  3. Observe that all 3 boxes are red (means none of them are receiving initial focus, despite the call to ref.focus())

More details in the comments of App.js

Expected Results

  1. Clone this repository
  2. Launch the app
  3. Observe that boxes 1 and 3 are red, but box 2 is white (indicating that the call to ref.focus() worked)

More details in the comments of App.js

CLI version

6.3.1

Environment

C:\code\rnw66focusrepro>npx react-native info
info Fetching system and libraries information...
System:
    OS: Windows 10 10.0.19044
    CPU: (20) x64 Intel(R) Xeon(R) W-2255 CPU @ 3.70GHz
    Memory: 39.22 GB / 63.69 GB
  Binaries:
    Node: 14.18.2 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.5 - C:\Program Files (x86)\Yarn\bin\yarn.CMD       
    npm: 6.14.15 - C:\Program Files\nodejs\npm.CMD
    Watchman: Not Found
  SDKs:
    Android SDK: Not Found
    Windows SDK:
      AllowDevelopmentWithoutDevLicense: Enabled
      AllowAllTrustedApps: Enabled
      Versions: 10.0.18362.0, 10.0.19041.0
  IDEs:
    Android Studio: Version  4.0.0.0 AI-193.6911.18.40.6626763    
    Visual Studio: 16.11.32002.261 (Visual Studio Enterprise 2019)
  Languages:
    Java: Not Found
  npmPackages:
    @react-native-community/cli: Not Found
    react: 17.0.2 => 17.0.2 
    react-native: 0.66.0 => 0.66.0 
    react-native-windows: 0.66.5 => 0.66.5 
  npmGlobalPackages:
    *react-native*: Not Found

Target Platform Version

10.0.19041

Target Device(s)

Xbox

Visual Studio Version

Visual Studio 2019

Build Configuration

Debug

Snack, code example, screenshot, or link to a repository

https://github.com/luisnaranjo733/rnw-66-focus-issue

@ghost ghost added the Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) label Dec 16, 2021
@ghost
Copy link

ghost commented Dec 16, 2021

Many of our core contributors are taking some much needed vacation throughout December 2021. Thank you for being patient while we relax, recharge, and return to a normal responsiveness in January 2022. In the meantime feel free to continue to pose questions, open issues, and make feature requests - you just may not get a response right away.

@NickGerleman
Copy link
Collaborator

useEffect is called after the React component is rendered to JS, but the underlying native view is rendered later. I know ref.measure does not work correctly until the component is first presented, for example.

ref.focus internally goes through UIManager.focus, which just calls TryFocusAsyc. Putting a breakpoint on the native side of that would make it a bit clearer as to what's happening. This sort of scenario seems like the kind of thing that would be useful to enable without too many hoops.

Managing focus with FlatList/VirtualizedList is particularly tricky in separate ways, since items under certain conditions can be rendered asynchronously in batches, or virtualized away while still focused (that one will be fixed).

@luisnaranjo733
Copy link
Member Author

luisnaranjo733 commented Dec 16, 2021

useEffect is called after the React component is rendered to JS, but the underlying native view is rendered later. I know ref.measure does not work correctly until the component is first presented, for example.

Good to know. So is the solution here to delay the ref.focus() call with some kind of special post native view render effect hook? Kind of like useLayoutEffect but after useEffect instead of before? Or is the timeout the best we can do at the moment?

ref.focus internally goes through UIManager.focus, which just calls TryFocusAsyc. Putting a breakpoint on the native side of that would make it a bit clearer as to what's happening. This sort of scenario seems like the kind of thing that would be useful to enable without too many hoops.

I think I found the right function and I'm hitting the breakpoint. Anything I should be looking for in particular?
image
image

Managing focus with FlatList/VirtualizedList is particularly tricky in separate ways, since items under certain conditions can be rendered asynchronously in batches, or virtualized away while still focused (that one will be fixed).

Agreed. I just dealt with this and ended up writing a fairly complex helper hook to try and abstract this away so I didn't have to re-implement this focus support for every screen that has a flat list with focusable stuff inside it. But maybe you meant tricky from the RNW implementation perspective. It's probably tricky from the app author perspective and the RNW maintainer perspective :)

@NickGerleman
Copy link
Collaborator

NickGerleman commented Dec 17, 2021

Good to know. So is the solution here to delay the ref.focus() call with some kind of special post native view render effect hook? Kind of like useLayoutEffect but after useEffect instead of before? Or is the timeout the best we can do at the moment?

Yeah, I really wish we had a hook for this...

I added this hack a while back to guarantee the condition reliably, but it does have the potential to add some latency.

function onNativeRender(callback: () => void) {
  // We need to wait until native has rendered a frame before measuring will
  // return non-zero results. Use RAF to schedule work on the next render, to
  // then shceduled work on the render after (at which point we should be all
  // good).
  requestAnimationFrame(() => {
    requestAnimationFrame(() => {
      callback();
    });
  });
}

Not being able to focus synchronously itself opens up to race conditions. E.g. the UI thread could move focus by keyboard or screenreader before ref.focus() is called. The Flyout native component tries to solve this issue via passing an autofocus prop, where the component focuses itself when mounted.

I think I found the right function and I'm hitting the breakpoint. Anything I should be looking for in particular?
From the screenshot, we found a shadownode for the React component on the JS side? Does the ShadowNode have a view attached to it? Any parents on the view?

This would let us know the state of the view tree, when we are trying to focus with useEffect, to confirm the speculated lifecycle issue. E.g. if the view has been created and added to the ShadowNode, if the view is part of a live tree, and if the view has been presented yet.

@luisnaranjo733
Copy link
Member Author

Here is the view on the JS side, as seen in Chrome debugger
image

And here are the expanded contents of the shadowNode on the native side.
image

On the m_view, I do see a Parent property, but it is set to a null value
image

I'm not sure if the this variable is relevant but sharing anyways just in case
image

Honestly I'm not super familiar with the guts of RNW but I hope this is helpful anyways 😄

@NickGerleman
Copy link
Collaborator

We can see then that a ShadowNode exists with a view, and a parent shadow node, but the view doesn't have a parent. This means for the <View> we try to focus in React has had a corresponding native UIElement created, but the element hasn't been connected to the app's visual tree.

So we cannot focus the component, because the UIElement hasn't been attached to the window's view tree yet.

This feels a little bit funky to me, but I'm not sure how much of this lifetime is controlled by us vs React/RN. I think @acoates-ms did a lot of the work for this before my time.

@rozele
Copy link
Collaborator

rozele commented Dec 27, 2021

The way we've worked around this in the past, e.g., for autoFocus properties on components like TextInput, is to wait for the XAML FrameworkElement::Loaded event. I wonder if we should add a FrameworkElement::IsLoaded() check to the UIManager::focus method, and if it's not loaded, add a Loaded callback. It's probably not quite that simple as you'd also need to ensure the focus call wasn't superceded by some other call (so you'd need a revoker that gets revoked before attempting to focus anything in case there is still a pending focus call after Loaded).

@chrisglein chrisglein added Area: Focus Needs: Dev Design and removed Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) labels Jan 3, 2022
@chrisglein
Copy link
Member

Do any of the other platforms have to handle this case where there's a timing difference between the nodes being available and the focus request?

I don't believe XAML has a way to declare the focus presence up front. This needs some thought for how to fix reliably.

@chrisglein chrisglein added this to the 0.69 milestone Jan 3, 2022
@rozele
Copy link
Collaborator

rozele commented Jan 24, 2022

This needs some thought for how to fix reliably.

I think the workaround described above could fix things pretty reliably - keeping a deferred focus state in UIManager in case imperative focus is called on a view that has not been loaded, and leveraging the same code for any autoFocus behaviors, e.g., for TextInput or Flyout.

In terms of how other platforms handle this, I think TextInput is the best example. I'm guessing the autoFocus prop emerged for this reason.

@NickGerleman
Copy link
Collaborator

NickGerleman commented Feb 13, 2022

I think the queue-based solution is a good one. If a focus request comes in, on something not yet connected, we buffer subsequent UIManager focus/blur requests until the next is available. That would preserve app-side focus state correctly, as well.

@NickGerleman
Copy link
Collaborator

NickGerleman commented Feb 13, 2022

We would want to replay the list of focus transitions within the same batch of work, for atomicity. But if that is replayed, synchronously on UI thread before other intersecting work, the focus state will maintain correctness.

@NickGerleman
Copy link
Collaborator

An edge case is if native focus state mutates in between the ref.focus call on the JS thread, and then UI thread execution. We would need a strategy to reconcile. E.g. throw out the JS batch if focus changes on native first, or vice versa.

@chrisglein chrisglein added Partner: Xbox (label may be applied by bot based on author) Partner: Facebook labels Feb 24, 2022
@jonthysell jonthysell modified the milestones: 0.69, 0.70 May 10, 2022
@chiaramooney
Copy link
Contributor

Unassigned currently. Due to release timeline; moving to 71.

@chiaramooney chiaramooney modified the milestones: 0.70, 0.71 Aug 16, 2022
@chiaramooney chiaramooney modified the milestones: 0.71, 0.72 Jan 9, 2023
@chiaramooney chiaramooney added the Needs: Attention 👋 An issue that had been tagged "Needs: Author Feedback" has received activity (label applied by bot) label Jan 9, 2023
@chiaramooney
Copy link
Contributor

Flagging. Issue has had a milestone since 69 but remains unassigned. Let's assign to a dev or issue should be moved to backlog.

@jonthysell jonthysell modified the milestones: 0.72, Backlog Jan 23, 2023
@jonthysell jonthysell removed the Needs: Attention 👋 An issue that had been tagged "Needs: Author Feedback" has received activity (label applied by bot) label Jan 23, 2023
@jonthysell
Copy link
Contributor

Moving this to the backlog until someone hits this with an up-to-date version of RNW.

@TatianaKapos TatianaKapos added the Recommend: Not Planned Recommend that issue should be given Not Planned milestone. label Aug 24, 2023
@chrisglein chrisglein closed this as not planned Won't fix, can't repro, duplicate, stale Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Focus bug Needs: Dev Design Partner: Facebook Partner: Xbox (label may be applied by bot based on author) Recommend: Not Planned Recommend that issue should be given Not Planned milestone.
Projects
None yet
Development

No branches or pull requests

7 participants