autoFocus on Modal's children does not work with multiple Modals

[larrydahooster]

I have two modals, both containing an input field with autoFocus. The modals use the standard props

autoFocus: true, enforceFocus: true, restoreFocus: true

When I launch them separately, each input field can claim it's focus and the modal allows it cause it finds focus in a own child of it. https://github.com/react-bootstrap/react-overlays/blob/v0.7.0/src/Modal.js#L461

When I launch the second Modal with an input that claims focus on top of the first modal, the input claims it (due to the render cycle of React that calls children's componentDidMount before parent's componentDidMount) before the componentDidMount hook of the new Modal got executed and the Modal could register to the Manager.

What happens now is that the first launched Modal revokes the claim of focus and takes it back cause it still thinks it is the topModal https://github.com/react-bootstrap/react-overlays/blob/v0.7.0/src/Modal.js#L488

The next thing that happens is the newly launched modal claims it's focus back for itself in componentDidMount cause it is the topModal now but can't find the focus in it's content anymore.

Here you can see the described behaviour in an extended example of the Modal https://larrydahooster.github.io/react-overlays/#modals

Maybe one solution could be to add the Modal to ModalManager on componentWillMount hook to register it bevor the modal children get rendered

[taion]

Could bad things happen with SSR if using cWM? Since cWUM will never get called, and the default modal manager is a global singleton...

[larrydahooster]

Okay, I see. Never thought about potential SSR problems so far. I was able to fix it for my usecase by only injecting enforceFocus = true into my last launched modal. As I am using a redux based modal system my ModalHandler is always aware of all existing Modals.

If you have a vague idea for a potential clean solution, feel free to share it. Otherwise I might write a custom solution that does not need to take care about SSR. Thanks for your input.

[jquense]

Why are two stacked modals showing up at once? Honestly, and not to judge, but that seems like a UX nightmare :P

[larrydahooster]

Legit question but sadly no easy answer, we had lots of discussion about Modals vs No Modals, only one Modal at a time... But there are always exceptions. Now we strive to avoid stacked Modals wherever possible but in that specific use case we decided for reusability of functionality (create new dashboard) which is already embedded into a modal. Sure a nicer approach could be an inline control but it is not highly prioritised yet. Anyhow, this should not turn into a UX discussion but in order to preserve the honor I would not speak of UX nightmare and would still consider this as a legit usecase ;)

[jquense]

From your gif the modals aren't mounting at the same time, which doesn't sound like a race condition to me then. https://github.com/jquense/react-bootstrap-modal implements stacking modals on top of react-overlays and I (don't think) I have seen this issue. Maybe there is something there that can help?

[larrydahooster]

I guess there is some sort of Race-condition in the ModalManager caused by a too late registration of the Second Modal! I tried to describe it in the Issue itself already. The gif above already includes the fix where I only allow enforceFocus:true on the newly spawned Modal and false to all existing ones. So it shows how it should behave also without the manipulation of enforceFocus.

This is how the "unpatched" version behaves (as you can also see it in the extended example case I shared: https://larrydahooster.github.io/react-overlays/#modals).

The redux-form field validates on blur. So It gets focus first, but the first Modal claims it back and then the new Modal gets the focus (as described in the issue already)

[jquense]

sorry I misunderstood what you were saying the race condition was. The problem isn't that focus is trapped in the underlying modal, but that it breaks autofocus, because the bottom modal's focus handler fires as focus leaves which is to early. It's quite interesting that the DOM focusin event fires before didMount is called, and unintuitive.

It's hard to move any code to WillMount because it all does DOM manipulation...maybe we can jsut but the focus handler in a timeout tho? It's ugly but seems like the simplest approach

[larrydahooster]

Yeah sorry for the misleading bug description but that was what I wanted to describe with "the first Modal claims it back". I should have written how it claims it back... with the eventHandler :) And that is where the first Modal still thinks it is the topModal cause the new Modal has not registered itself to the ModalManager at this point.

In my opinion just the registration to the ModalManager should be done in the cWM hook if a Modal is show: true onMounting. Registering after it got mounted is too late, cause the cDM hooks of the children being triggered first and actually the claim of focus of the child element happens there, just before registration.

[jquense]

In my opinion just the registration to the ModalManager should be done in the cWM hook

Like I said tho, registration in CWM is too early since registration does DOM manipulation. It'd break SSR and potentially client code if a Modal is set to be open as the page loads

[larrydahooster]

Sorry my bad again. I wasn't aware of the fact that adding a Modal already triggers the DOM manipulations. Well, I guess without a refactor of the ModalManager it is not possible then. Maybe it would be better to have a clearer separation of responsibilities on the ModalManager.

But in the end it would mean that the ModalManager needs to be aware of the state of Modals and becomes a stateContainer :-/