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

Keep autoFocus attribute in the DOM #11192

Merged
merged 4 commits into from
Oct 11, 2017
Merged

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Oct 11, 2017

This is the simplest thing we could do. Changes to always emit autofocus into the DOM, both on the server and on the client. We still have the JS hook too, although it doesn’t run during hydration.

We’ll need to also add a test verifying that the current behavior (Fiber doesn’t call JS focus on hydration) doesn’t regress. It was unintentional but we now consciously want to do it to prevent JS focus jumps when hydrating. I’ll work on that next.

(Done.)

We could still blacklist autoFocus on the client, and only emit it on the server. The argument for doing that is it’s less likely to conflict with our JS hooks and matches the current behavior for non-SSR apps. The argument against it is that it’s an extra weird special case, and it will be confusing to people to see autoFocus only on SSR’d content.

In this PR, I emit it on the client too. I haven’t decided if that’s right, and the input is welcome.

I decided to always omit it on the client for extra safety.

cc @syranide @aweary @sebmarkbage

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 11, 2017

It seems to me like it can’t be harmful to set it on the client too because whatever the browser does, we then override it with focus(). And most browsers seem to ignore it after the page load anyway (#11159 (comment)).

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 11, 2017

Although this would be dangerous if mobile browsers respected autoFocus more than they do focus() calls. This could be possible.

@syranide
Copy link
Contributor

@gaearon It's also important to note that you may want a field to autofocus on desktop, but not on mobile (IIRC this seems to be how browsers generally handle autofocus). Do any mobile browsers still support forced focus()?

@jquense
Copy link
Contributor

jquense commented Oct 11, 2017

mobile browsers still allow manually calling focus AFAIK (I have not heard complaints recently on react-widgets), there is a caveat with IOS which only allows focus() synchronously inside of an event handler, i.e. in response to a user initiated interaction

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 11, 2017

I pushed a commit that ignores setting autofocus on the client side.
This way we’re keeping the exact current behavior on the client, but also enable it for SSR.

@syranide
Copy link
Contributor

@jquense So then doesn't it make sense to categorize autoFocus as unobtrusive focus and focus() as obtrusive focus. I.e. autoFocus will mostly not work on mobile because it will bring up the keyboard, whereas it will on desktop because it's unobtrusive.

Now it's not an exact science for different reasons, but if this categorization is somewhat true, then it seems bad to merge them into one. An out-of-view search field being autofocused on desktop makes perfect sense, it bringing up a keyboard and zooming in on mobile does not. I'm assuming this is largely what autoFocus is meant to solve.

But I don't know the full matrix.

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 11, 2017

According to http://caniuse.com/#search=autofocus and https://www.wufoo.com/html5/attributes/02-autofocus.html (which has correct info on other points), autofocus attribute is not supported on iOS at all.

@jquense
Copy link
Contributor

jquense commented Oct 11, 2017

generally I agree with that characterization on mobile, it's very often not what you want. We should, as much as possible, just defer to browser behavior in any case. In vacuum i'd say that lends itself to emitting on the client and YOLOing to browser, and not polyfilling anything for mobile, but that is probably far more involved than anyone wants. It does seem like the right move here to not emit on client at least for this PR :)

@@ -286,6 +287,9 @@ function setInitialDOMProperties(
propKey === SUPPRESS_HYDRATION_WARNING
) {
// Noop
} else if (propKey === AUTOFOCUS) {
// We polyfill it separately on the client during commit.
// We blacklist it here rather than in the property list because we emit it in SSR.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it's time to fork that file then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to rework how it's done entirely (see #10805), and it would be more convenient if it was still centralized before I did that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't like that we have this extra special case here. :/ Mostly because I hope we can simplify this whole thing and special cases might hang around.

We could instead special case it on the server to minimize code. But I'm not sure that's any better for the maintainability. At least with this it's clear that it is probably the client that needs to change in the future.

@bogas04
Copy link

bogas04 commented Nov 13, 2017

Hi @sebmarkbage @gaearon !

I think there's a big change between 15 & 16 because of this. While I understand that you want to simply emit autofocus in SSR output due to reasons stated above, React 15 used to autofocus based on the value of said prop. However, now that we simply emit it, autoFocus="false" will cause browser to autoFocus anyway coz that's how HTML attributes work. So maybe we can check the value before emitting it ?

// Test 1
<input autoFocus={false} />

// SSR Output in React 15. Behaviour: no auto-focussing 👍🏼
<input data-reactroot="" />
// SSR Output in React 16. Behaviour: auto-focussing 👎🏼
<input autoFocus="false" data-reactroot="" />

// Test 2
<input autoFocus={null} />
<input autoFocus={undefined} />

// SSR Output in React 15 & 16. Behaviour: no auto-focussing 👍🏼
<input data-reactroot="" />

@gaearon
Copy link
Collaborator Author

gaearon commented Nov 13, 2017

That looks like a bug. Fixed in #11543.

@gaearon
Copy link
Collaborator Author

gaearon commented Nov 13, 2017

Fix is out in 16.1.1.

@bogas04
Copy link

bogas04 commented Nov 13, 2017

Wow that was quick. Thank you so much!!

@gaearon
Copy link
Collaborator Author

gaearon commented Nov 13, 2017

Thanks for reporting!

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

Successfully merging this pull request may close these issues.

6 participants