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

Add api for focus management #1791

Open
bmeck opened this issue Jul 5, 2014 · 12 comments
Open

Add api for focus management #1791

bmeck opened this issue Jul 5, 2014 · 12 comments

Comments

@bmeck
Copy link

bmeck commented Jul 5, 2014

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

current problems

  • this.getDOMElement().focus()

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

  • this.refs.x.getDOMElement().focus()

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

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

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
Copy link
Contributor

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
Copy link
Author

bmeck commented Jul 5, 2014

@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
Copy link
Contributor

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
Copy link
Contributor

syranide commented Jul 5, 2014

@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
Copy link
Author

bmeck commented Jul 6, 2014

@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 describe basically what this.getDOMNode().focus() does; it simply gives focus and keyboard event handling to a Node.
  • Delegation describes this.refs.x.getDOMNode().focus() and is used to provide a means to activate a child when an action occurs (usually things like a dialog opening, accordion opening, rich text area activating, screen change, etc.).
  • Ignoring is pretty simple, it just ignores the request; it is mostly used for things like containing focus to a modal dialog.

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:

  • shouldComponentUpdate needs to be able to delegate to a child. Unsure how this would work without refs.
  • Setting isFocused on a component may not relate to that component at all. isFocused could be setting state on child components (since we can't change props of child components until a render occurs). Once the child has the props changed. Every child that delegates will want to be sure to handle this property and state transition in a uniform way (mixin seems possible?). However, all of this means nothing if your component does not actually cause a focus change (see <ul style="display:none">).
  • Having a autoFocus property that only occurs during updates means your focus/blur events will need a callback to the data layer for the data layer to track current focus if it is preserved (only really a problem if delegation is present since activation can just use a singleton), and that binds UI state to data rather than the other direction. Unsure how people feel about this, but does not feel right to be tracking transient UI state in the data layer.

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
Copy link

ekryski commented Aug 3, 2014

👍 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
Copy link

👍 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
Copy link
Contributor

jquense commented Sep 26, 2014

also 👍 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
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants