-
Notifications
You must be signed in to change notification settings - Fork 47.1k
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
Make autofocus show up in rendered markup #3066
Comments
@zpao I never really liked the way it turned out. It does focus once mounted, so it's decent the way it is now at least... but it also does so if you've already manually focused elsewhere, which is not very nice at all. This is kind of tricky, should we just ignore browsers that doesn't support As far as So it seems to me that to solve this, we need something like #1979 I think? Make server- and client-rendering detectable and reusing should incur an additional render where it transitions from server- to client-state (allowing properties to be removed, etc). And this is something that seems to be creeping up more and more elsewhere, having to be able to tell server- and client-rendering apart and being able to generate different markup for the two. |
What is the issue with using |
@JSFB "We" decided not to use it because every browser has their unique interpretation of how it should work, literally all browsers do it differently. Overloading it and rendering it to the markup at the same time seems like a bad idea. |
Ok, we're probably going to get rid of the tag whitelist eventually anyway, so we would be sending autofocus to the DOM. If the behavior of autofocus is browser-dependent, user beware. Maybe we can do something to clean up the behavior and make it more consistent, but it's not entirely clear we should (though if it can be done easily/well, it seems reasonable). Either way, I think the assumption that autofocus can't be used client-side shouldn't be taken as a given/assumption going forward. The correct behavior, as I see it, would be to make autofocus work as consistently as possible in both environments (ie. it should be isomorphic). |
That's what we did? The problem is that IIRC FF autofocuses only on pageload, Chrome autofocuses only on initial element creation, IE autofocuses every time the element becomes visible, iOS does not autofocus at all, etc, or something like that, my memory is fuzzy but it's a total mess. It's literally useless now for anything but pageload or targetting a specific browser. Overloading is how we normalized the behavior.
I know it's been talked about, but I just can't see it happening practically, the translation between attribute and property is arbitrary, how would we handle boolean attributes and avoid garbage ending up in the markup? Should attributes really be forwarded as-is? React DOM to me needs to be a good and safe abstraction, removing the whitelist is going in the opposite direction regardless of how enticing it is. But you know more about this than me. |
+1, I believe this also prevents you from changing the focus when you render: |
Given the contested correct behavior of autoFocus, I'd rather abandon it until browsers agree upon its usage. I'd rather be able to do something like:
or
Since that syntax is probably not doable, maybe something like
|
FWIW, I was able to get this working using |
Up! Is there any news? |
Is it possible to set Update: |
Since React is not supporting this, is there a way/hack/workaround to force attributes not officially supported by React? |
This seems to be a clear bug in React. Not passing a valid attribute to a form element is not the right behavior, especially since React supports server side rendering. Any chance this behavior will change soon? |
In React 16 you can now add any DOM attributes. So you can add However, it would be nice if server side rendering translated |
I believe React will warn you about this, since We‘re probably planning to pass the normal |
Fixed in #11192. |
We currently don't because we handle this specially during runtime, but for server-rendered content it would be good to make this actually work.
cc @syranide
The text was updated successfully, but these errors were encountered: