[Fixed issue #464]

[kloots]

Fixes #464.

Changes proposed:

• Adds a new prop shouldReturnFocusAfterClose allowing explicit control over if a modal should restore focus to the element that had focus prior to its displayed.
• Does not try to restore focus if focus wasn't transferred to the modal in the first place (shouldFocusAfterRender was set to false)
• Fixes a typo in the Modal props so that shouldFocusAfter is now correctly shouldFocusAfterRender
• Fixes a number of line-length warnings

Acceptance Checklist:

• All commits have been squashed to one.
• The commit message follows the guidelines in CONTRIBUTING.md.
• Documentation (README.md) and examples have been updated as needed.
• If this is a code change, a spec testing the functionality has been added.
• If the commit message has [changed] or [removed], there is an upgrade path above.

[coveralls]

Coverage increased (+0.07%) to 86.445% when pulling 9fe0b63 on kloots:add-prop-to-prevent-restoring-focus into 5f5c2f0 on reactjs:master.

[diasbruno]

Hi @kloots There are 3 PRs in one. If you can stack 3 commits, one for each fix, it will show up better on the log and changelog.

[diasbruno]

[added] shouldReturnFocusAfterClose to control focus.
[fixed] correct property name shouldFocusAfterRender.
[chore] refactoring tests and lint.

Something like this.

[diasbruno]

...and thank you for taking a look on this.

[diasbruno]

This line also needs a condition ModalPortal.js#L151, otherwise the activeElement will be pushed on the focus stack and might cause troubles when open/close other modals (unlikely but possible).

[kloots]

@diasbruno All your suggestions are done, done and done

[coveralls]

Coverage increased (+0.1%) to 86.514% when pulling c2f2ef5 on kloots:add-prop-to-prevent-restoring-focus into 5f5c2f0 on reactjs:master.

[kloots]

@diasbruno any reason this hasn't been merged yet? Asking because we need this for Slack.

[diasbruno]

@kloots @ilikepies101 I'd be in favor of the first PR, just because it was the first...but it seems that the other PR will need to be rebased and also this one is more complete. So, I'll merge this one.

Thank you both for working on this issue.

[diasbruno]

Released v3.1.0.

[Hypnosphi]

I believe this should be && not ||. Currently I have to pass both shouldFocusAfterRender={false} and shouldReturnFocusAfterClose={false} to opt out of refocusing.

[diasbruno]

Looking at this now, if shouldReturnFocusAfterClose=false, it would also disable the 'focus after render'. They must be independent.

[diasbruno]

NOTES:

If shouldFocusAfterRender=true and shouldReturnFocusAfterClose=false, we put the element in the focus manager and on close we just pop it out.

shouldFocusAfterRender=false and shouldReturnFocusAfterClose=true must be noop just like shouldFocusAfterRender=false and shouldReturnFocusAfterClose=false.

shouldFocusAfterRender=true and shouldReturnFocusAfterClose=true is the default behavior.

Does this make sense?

[Hypnosphi]

Yeah, sounds reasonable

[Hypnosphi]

Btw, what are the usecases for shouldFocusAfterRender? Looks like it just focuses a div without any keyboard handlers

[diasbruno]

To set the scope for tabbing, so it won't leave the modal when it's open.