Nothing Special   »   [go: up one dir, main page]
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.

Sign up for GitHub

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

Jump to bottom

Support for options in .focus() #83

Closed
benoitgrelard opened this issue Sep 12, 2019 · 10 comments · Fixed by #84
Closed

Support for options in .focus() #83

benoitgrelard opened this issue Sep 12, 2019 · 10 comments · Fixed by #84

Comments

@benoitgrelard
Copy link
Contributor

Hey @theKashey, I am looking at adding support for focus options when calling .focus() in returnFocus.

See here: https://github.com/theKashey/react-focus-lock/blob/master/src/Lock.js#L60

The reason for that is that currently when the focus gets returned, if the page has been scrolled down and the element supposed to receive the focus back is not in view anymore, the browser will scroll to reveal that element being focused.

I understand that setting the returning the focus is absolutely important for accessibility and I wouldn't change that. However, it can be a bit disorientating for users to be scrolled to a difference place for what seems to be no apparent reason (to them).

Most latest browsers actually support an option to disable that behaviour if needed.
See the preventScroll option (false by default) on mdn.

Here's the current support for it.
I think support is not too much of an issue as this can be seen as an enhancement anyway.

I actually need this feature as part of react-focus-on but I know the code for the returnFocus handler actually lives in react-focus-lock.

Would you be willing to add a new prop returnFocusOptions to support that optional behaviour?

If so, I can draft you a PR today.

Let me know!

✌️
@benoitgrelard benoitgrelard mentioned this issue Sep 12, 2019
Closed
@theKashey
Copy link
Owner

Drilling a new prop thought all components would be not super easy, however - do we need a new prop?

  • returnFocus=false do not return focus
  • returnFocus=true return focus
  • returnFocus="smooth" (however, sounds weird) - this behavior.
  • returnFocus="preventScroll" sounds less weird, but is too concrete.

Any other ideas in mind?

@benoitgrelard
Copy link
Contributor Author

That's true, it could work with the one prop.
I agree "smooth" sounds weird.

Another option to be even more future proof (which is what I was trying to do with another prop) could be:

  1. returnFocus={false} do not return focus
  2. returnFocus={true} return focus
  3. returnFocus={{ preventScroll: true }} wanted behaviour (but could also support other options in the future)
  4. returnFocus={{ preventScroll: false }} equivalent to 2.

Essentially, returnFocus could accept a boolean, or focus options.

What do you think?

@theKashey
Copy link
Owner

I like it! That's the real future proof solution!

@benoitgrelard
Copy link
Contributor Author

Great, I'll draft a PR then. 👍

@benoitgrelard benoitgrelard mentioned this issue Sep 12, 2019
Merged
@benoitgrelard
Copy link
Contributor Author

@theKashey here's the PR: #84

@theKashey
Copy link
Owner

Not yet released

@theKashey theKashey reopened this Sep 12, 2019
@theKashey
Copy link
Owner
  • RFL - 2.1.0
  • RFO - 3.1.0
    Just released

@benoitgrelard
Copy link
Contributor Author

🙏 awesome

@klimashkin
Copy link
klimashkin commented Sep 13, 2019 •
edited
Loading

There seem to be focus-options-polyfill for edge/safari

@ufukomer
Copy link

still jumping to the top on chrome when used with react-popper.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

add support for focus options on returnFocus.
4 participants
@klimashkin @theKashey @benoitgrelard @ufukomer