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

breaking(Portal|Popup|Modal): do not autofocus #1341

Merged
merged 1 commit into from
Feb 17, 2017

Conversation

levithomason
Copy link
Member

@levithomason levithomason commented Feb 17, 2017

Breaking Change

What changed?

Portals no longer autofocus on mount. This means Modals and Popups are no longer automatically keyboard accessible after opening.

What's the solution?

If your app needs to focus a Portal powered component on mount, you should do so explicitly by locating the DOM node that needs focus and calling .focus() when appropriate.


Fixes #1324

Why?

Introduced in #1154, the Portal was updated to take focus on mount and restore focus on unmount. This was done to allow keyboard access inside of Modals after they opened. However, the Portal powers all components that appear on the page out of context, Modals, Popups, Confirms, and Dimmers.

It is impossible for the library to know when it is appropriate for a portal in an app to steal focus or when it is appropriate to restore focus to the originally active element on close. Because of this, the responsibility of focus management is removed from the library and is now the responsibility of the app implementing the component.

This PR removes the final remaining focus feature, namely, focus on mount.

@codecov-io
Copy link

Codecov Report

Merging #1341 into master will decrease coverage by -0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1341      +/-   ##
==========================================
- Coverage   99.75%   99.74%   -0.01%     
==========================================
  Files         140      140              
  Lines        2401     2394       -7     
==========================================
- Hits         2395     2388       -7     
  Misses          6        6
Impacted Files Coverage Δ
src/addons/Portal/Portal.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5ff572...d1c7e17. Read the comment docs.

@levithomason levithomason merged commit e73b40f into master Feb 17, 2017
@levithomason levithomason deleted the fix/remove-portal-focus branch February 17, 2017 16:52
@levithomason
Copy link
Member Author

Released in [email protected]

harel pushed a commit to harel/Semantic-UI-React that referenced this pull request Feb 18, 2017
@fracmak
Copy link
Member

fracmak commented Feb 21, 2017

question, what is the scenario where a modal does not take focus? Would it be worthwhile adding the focus stealing functionality back but put it only there?

@levithomason
Copy link
Member Author

levithomason commented Feb 21, 2017

The issues came up one at a time:

One issue with taking focus on open (#1324) is that if the user has their cursor in an input, then they mouse over a completely unrelated popup somewhere, then the input looses focus since the popup steals focus.

I could see this also being the case if focus were required for any arbitrary element and some other unrelated Portal powered component (Dimmer, Popup, etc) were to open and take focus.

It seems there are very limited cases where changing focus makes sense, such as in some Modals and some Popups. However, only the developer knows which Popups and Modals make sense to focus and when. For instance, a Popup with a Button or Input might make sense to focus, but a tooltip does not. We have no way of knowing this information from the context of the Portal.

The conclusion is that focus management should be handled on a case by case basis using the proprietary knowledge of the implementation.

@fracmak
Copy link
Member

fracmak commented Feb 21, 2017

I was just curious if you had a specific use case for when a modal doesn't steal focus, cause in my mind modals always take over the screen and hence always take focus

@levithomason
Copy link
Member Author

I considered only having modals take focus, though, given the other issues I opted to leave it to the user. I think even with Modals, it only makes sense to take focus if there are controls within the Modal, such as links or buttons.

Although it makes sense to focus some Modals, I think the issues related to unnexpectedly stealing focus from other elements outweigh the benefits. There is no telling if stealing focus from elsewhere will be detrimental to the UX.

Lastly, SUI core modals do not take focus on mount either. I considered this in deciding against it by default.

Taken all together, it is sometimes useful to take/restore focus for Modals but I don't think it is always proper to do so.

If we get some more requests for focusing modals on mount, I'd like to consider adding this as a prop(s) to the Portal. This would enable users to opt-in to focus management for any Portal component. Given more requests, it may be worth the added complexity.

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

Successfully merging this pull request may close these issues.

Popups and Tooltips steal focus from input fields
3 participants