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

autoFocus doesn't work with SSR in React 16 #11159

Closed
sebmarkbage opened this issue Oct 9, 2017 · 12 comments
Closed

autoFocus doesn't work with SSR in React 16 #11159

sebmarkbage opened this issue Oct 9, 2017 · 12 comments

Comments

@sebmarkbage
Copy link
Collaborator

<input autoFocus /> works on the client but not when hydrated.

That's because this used to be implemented in JS as a special case but hydrateInstance doesn't cause a commit effect to be scheduled that can call .focus() like finalizeChildren does.

The question here is, should we even bother implementing this in JS anymore or should we just emit the autofocus attribute in SSR and let the browser take care of it.

@nhunzaker
Copy link
Contributor

I think we should emit autofocus as an attribute.

It looks like the key drawback is that autofocus is not supported in IE9, iOS Safari, and Android browser. Are there any other drawbacks that I am missing? How hard/mechanical would it be to add autofocus via JS?

@sebmarkbage
Copy link
Collaborator Author

If we always do it in JS, it's pretty easy, but it seems pretty bad for semantics. It can take a while for all the scripts to load before it actually gets focused. The user can focus other things in the meantime and then suddenly it becomes focused. Since with Fiber we can asynchronously rehydrate and we might also want to do partial hydration it might make that even worse than it already is.

We can do both but then you have weird artifacts where it might get focused, you switch off and then it gets focused again.

We could potentially only do it in JS if the normal one fails, but not sure how to detect it and that could add a lot of code.

@sebmarkbage
Copy link
Collaborator Author

I'm not sure what the right tradeoff is here given the browser support issue.

@nhunzaker
Copy link
Contributor

It can take a while for all the scripts to load before it actually gets focused.

I think this is a very strong case for emitting an attribute. It sounds like we can't guarantee correctness, even if we check document.activeElement, the user might already be in a flow or have scrolled down the page. We don't want the page to jump up or have someone start typing into another input box midway through a key stroke.

Sorry if some of that is a rehash, but that'd be the argument I would make if someone filed an issue about it.

@Merri
Copy link

Merri commented Oct 9, 2017

Emitting attribute sounds like the right way to go. If I want autofocus to work on browsers that don't support it it seems most sane to add a script to end of body:

    <script>!function(el){el&&el.focus()}(document.querySelector('[autofocus]'))</script>

Which I guess only causes issues if the HTML page is very large. I guess it could also check for activeElement (if none or body then focus) but that might be overkill.

@gaearon
Copy link
Collaborator

gaearon commented Oct 9, 2017

cc #3066

@syranide
Copy link
Contributor

syranide commented Oct 10, 2017

The initial reason for not emitting the autofocus-attribute was that browser implementations were widely inconsistent in how they treated it (some does not support, some only focus on page load, etc). Which IMHO means that anyone that is serious about autofocus had to apply it manually via JS anyway. However, I vaguely remember there now being mobile browsers that don't really listen to JS focus, but do honor autofocus to some extent.

It's a mess, but there's some merit to just emitting the attribute, and if you feel strongly about it then you focus manually. You're given all the tools.

@gaearon
Copy link
Collaborator

gaearon commented Oct 11, 2017

I chatted with @trueadm about this and I think this sounds like a plausible way forward:

React 16

  • Emit it in SSR HTML.
  • Ensure we don’t call JS polyfill for hydrated elements.
  • Keep blacklisting it on the client-created elements and use JS polyfill for those.

React 17

  • Remove JS polyfill completely.
  • Warn if there is more than one autoFocus element added during a commit (since this is where the browser inconsistencies occur).

Does this make sense?

@syranide
Copy link
Contributor

Warn if there is more than one autoFocus element added during a commit (since this is where the browser inconsistencies occur).

Browsers, at least not long ago, are inconsistent in how they interpret the autoFocus attribute as well. I know that e.g. FF would only honor it on pageload and totally ignore any that were dynamically inserted. But I haven't surveyed it in a long time now.

Still, I think the approach is correct.

@gaearon
Copy link
Collaborator

gaearon commented Oct 11, 2017

After some basic investigation it looks like most modern browsers except Safari don’t respect autoFocus after page load at all.

It probably made sense for HTML documents, but in apps I would expect autoFocus to be respected for dynamically inserted elements since they’re basically “page transitions” as much as the first page load in SPAs.

So we should probably keep the polyfill.

@gaearon
Copy link
Collaborator

gaearon commented Oct 11, 2017

It gets weirder.

Chrome respects dynamically added elements with autoFocus if there hasn’t been one before. Even creating an input with autoFocus, adding it to the document, and removing it in the next tick is enough to disable autofocusing of any elements added in the future in Chrome.

Firefox just doesn’t care about any dynamically added elements with autoFocus at all.

And Safari always respects them.

@gaearon
Copy link
Collaborator

gaearon commented Oct 11, 2017

Fixed in #11192.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants