Bug fix for issue 5202

[antsmartian]

Hi All,

Bug fix for #5202. It looks fine to me, however let me know if anything is missing. If not, can add test case and close this! Thanks.

[jimfb]

Thanks @antoaravinth!

[quantizor]

No tests?

[jimfb]

@yaycmyk It's a little annoying to add unit tests for every type of event. Since the native events need to be forged, they're of minimal value anyway, since all you're testing is the forged event. Such tests would make a lot more sense if/when we do in-browser testing (but the tests would need to be written differently anyway, using webdriver/whatever, which we currently don't support).

That said, if you'd like to submit a PR that adds a test for this event type, we'd gladly accept.

[gaearon]

Was this the correct fix?

I find it a bit weird that we seem to already normalizing focusin and focusout as part of focus and blur support. Supporting them explicitly seems contrary to the goal of having a normalized event system, especially considering that they are not supported in Firefox.

[sebmarkbage]

This isn't feature detected. Note:

https://developer.mozilla.org/en-US/docs/Web/Events/focusin

Firefox (Gecko): Not supported.

This would need to be normalized, but if we do then it works exactly like focus/blur. So I think we should revert.

[jimfb]

I think we should still support the event name (or at the very least warn and say "use focus/blur", but that seems dumb). If the behavior is the same in our polyfilled environment, we should just map them to the same behavior/implementation.

[gaearon]

If the behavior is the same in our polyfilled environment, we should just map them to the same behavior/implementation.

So far we have tried to have only way to do something. Why break this tradition? It’s the same as class vs className, onClick vs onclick: if we chose one, let’s stick with it.

at the very least warn and say "use focus/blur", but that seems dumb

IMO it actually sounds really sensible. This is a great opportunity to warn the user in a very specific and useful way. I think we should revert this PR but add a warning.

[sebmarkbage]

Warning is fine. Not sure about mapping it since that means we need to have logic and checks for it in prod. Why is it important to map if there is a warning and we never supported it in the past?

[jimfb]

👍

[gaearon]

This has been reverted in #6274. Sorry about the back and forth on this!
If there’s something about the existing onFocus and onBlur that doesn’t work for you, please let us know. Cheers!

[antsmartian]

@gaearon @sebmarkbage Thanks. That make sense and I agree with you guys. Probably I will add warnings as discussed here.