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

[Flare] focus state not updated when moving to another Focus target with node.focus() #15942

Closed
aweary opened this issue Jun 20, 2019 · 7 comments

Comments

@aweary
Copy link
Contributor

aweary commented Jun 20, 2019

@trueadm working with Focus alongside @reach/combobox I encountered what looks like a bug related to programmatically calling node.focus() in an effect after a state update from onFocusVisibleChange (edit: and onFocusChange), where the other Focus target that was blurred due to the focus() call doesn't call register that it's been blurred.

That's a mouth-full, so here's a sample that repro's it for me:

// Simple wrapper around <input> that styles the element on focus
function Input() {
  const [isFocused, setFocused] = React.useState(false);
  const backgroundColor = isFocused ? 'red' : 'white';
  const inputRef = React.useRef();
  // This unconditionally steals focus after every update, which isn't very likely, but it's
  // possible components like @reach/combobox will attempt to do this conditionally
  React.useEffect(() => {
    inputRef.current.focus();
  })
  return (
    <Focus
    onFocusVisibleChange={setFocused}>
      <input ref={inputRef} style={{backgroundColor}} placeholder="Input"/>
    </Focus>
  )
}

// Simple wrapper around <a> that styles the element on focus
function Link() {
const [isFocused, setFocused] = React.useState(false);
  const color = isFocused ? 'red' : 'black';
  return (
    <Focus onFocusVisibleChange={setFocused}>
      <a href="#" style={{color}}>Link!</a>
    </Focus>
  )
}

function Application() {
  return (
    <div>
      <Input />
      <Link />
    </div>
  )
}

How to Reproduce

Running this example with a build from master (deployed here http://react-events-focus-effect-bug.surge.sh/)

  1. Open the page and focus the input by tabbing to it if it's not already focused
  2. Attempt to focus the link by tabbing again

What Happens?

The link will register as focused (coloring it red) but that state gets stuck once the input is focused again.

What Should Happen?

Once the input gets focused again the Focus target in Link should call onFocusVisibleChange with false so the focus state gets reset.

@trueadm
Copy link
Contributor

trueadm commented Jun 20, 2019

cc @necolas

@necolas
Copy link
Contributor

necolas commented Jun 20, 2019

Does the same thing happen with onFocusChange?

@aweary
Copy link
Contributor Author

aweary commented Jun 20, 2019

@necolas yeah, it happens with both onFocusChange and onFocusVisibleChange

@aweary aweary changed the title [Flare] onFocusVisibleChange when focus is removed with node.focus() in effect [Flare] focus state not updated when moving to another Focus target with node.focus() Jun 20, 2019
@necolas
Copy link
Contributor

necolas commented Jun 21, 2019

This looks like a problem with synchronously moving DOM focus within the callbacks. You can see the same behaviour when using the existing event system: https://codesandbox.io/s/experimental-event-api-hlm4o (and how it is fixed by the setTimeout). Something like this came up in React Native for Web text inputs too.

If you select the input and link elements in devtools and use monitorEvents($0, [ 'blur', 'focus' ]) in the console, it shows the native DOM event order in your example becomes: 'input:blur', 'link:blur', 'input:focus', 'link:focus'. This is because focus is being moved during the first blur event causing the next blur/focus pair to be dispatched between the first blur/focus events. I'm not sure why browsers still need to dispatch the focus event in this case, but they do :/

@necolas
Copy link
Contributor

necolas commented Jun 21, 2019

I don't know if there are other scenarios in which an element could find itself blurred before it is focused, so using that to infer anything in the responder might end up breaking something else.

@necolas
Copy link
Contributor

necolas commented Jun 24, 2019

I looked into this a bit more. It might be that using focusin and focusout events could support both sync and async programmatic focus setting, as Chrome doesn't dispatch a focusin event at the end of the sequence.

@necolas
Copy link
Contributor

necolas commented Dec 17, 2019

Closing as we're no longer developing this API. See #15257 (comment)

@necolas necolas closed this as completed Dec 17, 2019
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

No branches or pull requests

3 participants