react-native-windows icon indicating copy to clipboard operation
react-native-windows copied to clipboard

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

Open luisnaranjo733 opened this issue 3 years ago • 13 comments
trafficstars

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

luisnaranjo733 avatar Dec 16 '21 22:12 luisnaranjo733

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.

ghost avatar Dec 16 '21 22:12 ghost

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).

NickGerleman avatar Dec 16 '21 22:12 NickGerleman

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 :)

luisnaranjo733 avatar Dec 16 '21 23:12 luisnaranjo733

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.

NickGerleman avatar Dec 17 '21 02:12 NickGerleman

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 😄

luisnaranjo733 avatar Dec 17 '21 05:12 luisnaranjo733

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.

NickGerleman avatar Dec 17 '21 06:12 NickGerleman

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).

rozele avatar Dec 27 '21 18:12 rozele

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 avatar Jan 03 '22 19:01 chrisglein

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.

rozele avatar Jan 24 '22 17:01 rozele

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 avatar Feb 13 '22 16:02 NickGerleman

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 avatar Feb 13 '22 16:02 NickGerleman

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.

NickGerleman avatar Feb 13 '22 16:02 NickGerleman

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

chiaramooney avatar Aug 16 '22 20:08 chiaramooney

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

chiaramooney avatar Jan 09 '23 19:01 chiaramooney

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

jonthysell avatar Jan 23 '23 18:01 jonthysell