-
Notifications
You must be signed in to change notification settings - Fork 46.4k
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
Add unknown property warning for use of autofocus
#6461
Conversation
@hkal updated the pull request. |
Went ahead and added the test-case after seeing some other tests for @jimfb - noticed you set the label on the original bug. Let me know if this adequately addresses the problem. |
@hkal updated the pull request. |
I think so. Afaik this event already exists, check out #6800. You might want to tweak that code. |
getPossibleStandardName: __DEV__ ? {autofocus: 'autoFocus'} : null, @hkal the values in |
bf506a1
to
af6bea3
Compare
@aweary my bad. Somehow I missed these notifications. Updated the pull request, let me know if anything else needs changing. |
Thanks @hkal, looks good to me 👍 |
I'm actually going to revert this - it actually has more impact than just the warning and changes how autoFocus handling will behave. I can't demonstrate it in jsfiddle because frames and focus don't play nicely. But just take an example and render 2 inputs with autoFocus={true}. 15.x will consistently focus the 2nd. After this change the first will get focused (in Chrome) and we're otherwise subject to whatever the browser decides to do when nodes are updated or removed. That's not to say we should necessarily be handling this ourselves, but for the time being we shouldn't change it. When we get to removing the whitelist we'll have to deal with that side of things. For now, if we want this to work, we need to handle this without adding the property to the whitelist as we don't want it to end up in the DOM. |
Thanks for clarifying @zpao, I had tested that autoFocus still worked before merging but didn't notice the order may have been inconsistent. |
Thanks for explaining the issue, @zpao. Didn't even think to test that case. @aweary I'm pretty confident we can still get the warning without any unintentional side affects if we go back to pre-defining the autofocus property in |
edit: see #3248 for discussion |
Resolves #3248
This PR also:
ReactDOMFeatureFlags.useCreateElement
is onI wrote some tests for this work, but didn't notice any other
__DEV__
specific warnings being tested. I can include them with this PR if they're wanted.