facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
229.27k stars 46.96k forks source link

Add api for focus management #1791

Open bmeck opened 10 years ago

bmeck commented 10 years ago

There are currently a couple of problems with focus management in React.

current problems

does not guarantee the node is on a document/visible. visibility can be hard to detect due to other components firing render().

does not guarantee that x has finished any pending renders. if x renders, focus is lost.

this fires on the child nodes before parents so in the case of:

<ul style="display:none">
  <li><button>can't focus</button></li>
</ul>

if we want to show the <ul/> and focus the button.

the component of the <button/> focusing during componentDidUpdate has no affect because the <ul/> is still hidden.

discussion

if looks like some of the PRs to react are using raf or timeouts to achieve fixes to the problems listed. This can cause race conditions, and can be fixed with a lifeCycle addition, or just a hidden lifeCycle purely for focus management.

The issue comes down to not having a lifecycle able to fire a function after all rendering is done, not just an individual component.

I would suggest we add a simple API of component.blur()/component.focus() that queues the requests and fires them after all rendering is done. The fix is fairly simple, but I wonder how people feel about this.

volkanunsal commented 10 years ago

Just a thought. Have you thought about using shouldComponentUpdate() to prevent updates from happening on the form element that can be focused? I would guess that might prevent the issues with focus being lost when a component gets updated.

bmeck commented 10 years ago

@volkanunsal shouldComponentUpdate() is a very complex approach to this. We would need to still check for the problems listed above, and still does not cover how to manage the <ul/> <button/> example above since the button would be the component being focused, and the <ul/> component is completely separate. Maybe I don't understand your use of shouldComponentUpdate()

volkanunsal commented 10 years ago

What I am suggesting is making x a component rather than grabbing a reference to it through a ref. That way you can pass in props that would be checked by the shouldComponentUpdate() callback, where you can say whether the node should be re-rendered or not.

syranide commented 10 years ago

@volkanunsal Delaying focus() until the next requestAnimationFrame/setTimeout is probably your safest bet. It seems that even though elements have finished rendering and are attached to the DOM, that is not a guarantee for focus() succeeding.

Personally I wouldn't mind adding a focus() method on the base DOM-component that could incorporate this "fix", but it's tricky decision I think, since there are a lot of other features that could be considered equally necessary, but doesn't really have to be supported by React (you can just have your own external focus-helper). It makes sense to only keep the required features part of React in the core and anything that is optional out of the core.

bmeck commented 10 years ago

@volkanunsal took a bit to grok what was being said. If we do add properties like autoFocus to the component accessed by a ref in my example we can move focus tracking outside of React and keep our data layer ensuring isFocused (or aptly named alternative) occurs in the right place. This works well enough if the data layer wants to track transient UI state as focus/blur events occur. However, I would like to bring up just a basic reason we really want to be able to use refs. Focus management really handles 3 major actions:

Activation works fine to well depending on if you want to wrap all the React.DOM.* components.

If we flatten all the components we get into some odd cases for delegation:

Ignoring works fine to well as long as your data layer can filter out when components would not be valid targets for focus. This means we will need to use the result of React.renderComponent to fire methods based upon current props/state. A bit odd but w/e works.

ekryski commented 10 years ago

:+1: Would love to see this land. Either implementation would be fine for me. Using an autoFocus attribute or using this.refs.foo.getDOMNode().focus().

laurilehmijoki commented 10 years ago

:+1: I would also love to see a built-in support for this focus issue.

My current workaround is this (in CoffeeScript):

render: ->
  # other code...
  if @state.captchaIsLoading == false
    setTimeout( => @refs.captchaAnswerField.getDOMNode().focus())
  # other code ...

While the above solution solves the problem, it leaves me with a feeling that I have a hack in an otherwise elegant React component.

(All in all, thanks for React. It is such a wonderful tool. I love to work with it!)

jquense commented 10 years ago

also :+1: this, My use case is trying to maintain focus on a single input while you interact with its child elements (such as the the dropdown in a selct widget). I have given up trying to manage focus with state, it has only led to confusing bugs in browsers, and really hard to understand order of execution.

I ended up using a timeout in each widget, which is triggered (among other places) onBlur/onFocus. The timeout also helps manage when focus moves within a parent component. I also need a isFocused state for adding/removing classNames, leaving the whole thing pretty wonky, and easy to get out of sync.

I am not sure on the best implementation, but for me the issue is not setting initial focus, but maintaining it on the correct input, the isFocused input. I feel like I ended up with the autoFocus option, but I agree that that might be a weird thing for React to generalize to...

ryanflorence commented 9 years ago

I work pretty hard to create accessible UI and have found React to be a breath of fresh air for focus management.

I haven't seen any discussion around using the setState callback for focus management.

this.setState(newStuff, () => {
  this.refs.email.getDOMNode().focus(); // will be rendered
});

Here's a complex one taken straight out of an app I'm working on:

    this.setState({ gotEmail: true }, () => {
      this.refs.thanks.getDOMNode().focus();
      // setTimeout has nothing to do with focus, just wanting to display a "thank you"
      // for two seconds
      setTimeout(() => {
        var focusHasNotMoved =
          this.refs.thanks.getDOMNode() === document.activeElement;
        this.setState({ gotEmail: false }, () => {
          if (focusHasNotMoved)
            this.getDOMNode().focus();
        });
      }, 2000);
    });
ryanflorence commented 9 years ago

@laurilehmijoki

Don't use render, that's for rendering, use the componentDidMount lifecycle hook.

#render: ->
componentDidMount: ->
  # other code...
  if @state.captchaIsLoading == false
    @refs.captchaAnswerField.getDOMNode().focus()
  # other code ...

But probably you should do it wherever you setState on captchaIsLoading:

@setState(captchaIsLoading: no, () => @refs.captcha.getDOMNode().focus())

You only want to focus the captcha the first time the state changes, not every time any data on the view changes and causes a render.

ryanflorence commented 9 years ago

I never address the OP. All of these scenarios are manageable with the lifecycle hooks provided by React.

this.getDOMNode().focus() does not guarantee the node is on a document/visible. visibility can be hard to detect due to other components firing render().

do this in the setState callback or lifecycle hooks that guarantee render is complete like componentDidUpdate and componentDidMount

this.refs.x.getDOMNode().focus() does not guarantee that x has finished any pending renders. if x renders, focus is lost.

Same as the last notes, if you use the right hooks, render is guaranteed. If focus is lost by x rendering again then your node completely changed. When react applies the VD diff it doesn't blow away focus just because of a render, that only happens if your render returns UI that no longer contains the node with focus.

componentDidUpdate this fires on the child nodes before parents so in the case of:

<ul style="display:none">
  <li><button>can't focus</button></li>
</ul>

if we want to show the <ul/> and focus the button. the component of the <button/> focusing during componentDidUpdate has no affect because the <ul/> is still hidden.

When are you trying to focus and how does the <ul/> become visible? My guess is it becomes visible via a state change or receiving new props (and then no more display: none), in which case you can use the setState callback where you changed the state, or in componentDidUpdate check the state or props and then focus, though I would discourage you from using anything other than the setState callback because you don't want to be focusing the button every time any state changes in the component, only on the original transaction that made you want to focus in the first place.

likethemammal commented 8 years ago

For anyone stumbling around the internet trying to find a fix, @volkanunsal was right on the money. I had a similar situation where the focus() call wasn't hitting the element because it wasn't completely visible when componentDidUpdate ran.

As suggested, delaying the focus() with a requestAnimationFrame (or setTimeout) fixed the issue. Although, setting the callback this way setTimeout(this.refs.item.focus, 10) gives an Illegal invocation error. This can be resolved by dropping the focus call in a function. So the final resultl would look something like this:

setTimeout(function() {
    this.refs.item.focus()
}.bind(this), 10)