theKashey / react-focus-on

🎯 Solution for WAI ARIA compatible modal dialogs or full-screen tasks, you were looking for
MIT License
336 stars 14 forks source link

Ability to control lock/unlock with prop #25

Closed alexandernanberg closed 4 years ago

alexandernanberg commented 4 years ago

Sometimes you want to be able to lock and unlock before the component has fully unmounted, e.g. when doing animations.

To illustrate my problem I've created a simple codesandbox with react-spring.

https://codesandbox.io/s/runtime-lake-76g5v

When you open the dialog everything works as expected, however when you close it you can notice a obvious delay. The focus isn't returned until the component has fully unmounted and the scroll is locked too. This behavior isn't ideal imo.

Is there some way to solve this without any API change?

theKashey commented 4 years ago

Have you tried enabled prop? Look like it's almost working, but there is some problem with returnFocus (not working) https://codesandbox.io/s/react-focus-on-react-spring-5m3uv

alexandernanberg commented 4 years ago

Yeah tried with that but not returning focus is a deal breaker, you also get a Can't perform a React state update on an unmounted component. warning from react

theKashey commented 4 years ago

As long as I am not aware of the problem - I'll handle it and fix soon. Like tomorrow should be ok-ish. Thank you for letting me know

theKashey commented 4 years ago

3.1.4 should fix it

alexandernanberg commented 4 years ago

Awesome, thanks! 👍

A minor nitpick would be that the dialog content jumps little when closing it. I guess it's because the scroll-lock is removed before the element has fully unmounted. Would it be possible to solve this somehow too? This is not a deal breaker though, just a nice to have

theKashey commented 4 years ago

That's interesting. And easy doable. However, I could not propose any other syntax except additional prop to override scrollLock settings. Right now there is only one to disable it, not forcible enable...

<FocusOn
 enabled={controlsFocusLockAndScrollLock}
 forceScrollLock={controlsScrollLock}
>
alexandernanberg commented 4 years ago

Thought a bit more about this and I don't think the scroll should be locked while the dialog is closing. So immediately when you close you should be able to scroll (like it's currently) but the scrollbar style applied to the dialog needs to stay until it's unmounted to avoid the jump. This will probably be messier to implement but will provide a better user-experience.

Do we still need the additional prop in this case? I think we should be able to just go with the enabled prop still

theKashey commented 4 years ago

Right now you can:

  1. set focusLock prop to false, this would disable focus lock, keeping scroll lock active
  2. set enabled to false after animation end.

That would solve your problem.

However, I could not unlock scroll and keep scrollbar hidden. The hidden scrollbar would keep scrolling disabled. However, you might mitigate the "jump" applying an extra classname to your Modal - https://github.com/theKashey/react-focus-on#exposed-from-react-remove-scroll, like you need the first one (classNames.fullWidth)

alexandernanberg commented 4 years ago

Alright, adding the classname got me the closest to what I want, however I wasn't able to get the gapMode to use padding instead of margin. Maybe because the overlay is the parent of the FocusOn? I've updated the codesandbox with the changes. Last thing from this being the perfect solution 😄