-
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
onFocusIn/onFocusOut events #6410
Comments
FWIW, I agree with you. People know about focusin/focusout, and so we should just make them work, despite the fact that they would map to the exact same event handler we use for focus/blur. However, last time this was discussed, we opted to not do this (#6226) and decided to warn instead (https://github.com/facebook/react/pull/6296/files). I'd be fine with reversing this decision if you can get the momentum within the team. |
It looks like it was decided against because of an incorrect claim that they're indistinguishable from onFocus/onBlur. onFocusIn/onFocusOut is different in that when moving focus between two children of an element, neither event fires on the container. |
Doing it properly means adapting the logic in EnterLeaveEventPlugin and isn't as simple as adding it to SimpleEventPlugin. |
Ah, that makes sense. Thanks for explaining! Adding “good first bug” here. Note @spicyj’s comments: they need to behave like |
I'd like to work on it if nobody minds. Enough with the tests, time to add real source code! |
Hey @gaearon @spicyj apparently there is a confusion here, let me explain. Native Native Here is a jsfiddle I made to display this. Checked in Chrome only, as since MDN documentation states that this is intended behavior then I believe it should be the same in other browsers. If I am right about this (please correct me if I am wrong), then the following should be done:
|
Hmm, that sounds plausible. Thanks for the correction. Kinda annoying that we'll end up renaming these for people but maybe it's for the best. |
Hey, unfortunately, I cannot invest time in this right now and I must pause. I have added onFocusIn/Out by mimicing onFocus/onBlur but I am having issues with changing onFocus/onBlur so they don't bubble. |
Do we want them to not bubble? We had previously said that all events bubble. I'm fine with whatever, so I'll leave that decision to @spicyj since he has the most situational awareness here. |
Since this is tagged as a good first bug I thought I'd take a stab at this. Let me know if you're still working on it @cbrwizard ! |
I did a little more investigating and it looks like focusin and focusout events are not supported by Firefox. Is this still something you want to implement if all browsers don't support it? See here: https://developer.mozilla.org/en-US/docs/Web/Events/focusin and here: https://bugzilla.mozilla.org/show_bug.cgi?id=687787 |
@mrscobbler It should be possible to polyfill the event on any browsers that don't natively support it. We do this with several events that are not supported on particular browsers. |
Ok! Good to know. |
Any status on this? |
So I've spent a few hours figuring out the events code (I've never looked at it before) and I've made some progress. Possibly @cbrwizard could walk me through some of what he's done? I might get a better understanding of what needs to happen. @trigun539 is it something you want to tackle or do you just need/want it finished? |
@mrscobbler @spicyj Was it ever decided that this is something that should still be implemented? Seems like the discussion of whether or not this should be added based on the fact that all events currently bubble kind of ended without a consensus. I'd be willing to help out with this if we decide to go further with it. |
I am working on an app that needs to be 508 compliant, and have a flyout menu where the last inner submenu li element should fire a focusOut event (for the parent li) when you go past it. However, it doesn't fire it. I have added the onBlur event and it doesn't work as expected. I can take a look at this. |
Navigation is all done through keyboard. |
@trigun539 Can you post a code example by any chance? I just created a basic example with a ul and two li's that each have an inner anchor and after adding onFocus and onBlur events to the li's, the events fire correctly and bubble up from the inner anchors. |
@anthonybarsotti the set up is more around the following: `
If I tab when I am on the "last li" I should go to "Another test" li, but it should fire an onBlur event on the "parent item" li. I'll post a sample after I get it set up. |
@mrscobbler Can we please work on it together? I wish to contribute. |
@rishirajsurti Yes! I'll send you an email. @anthonybarsotti @spicyj has it been decided whether or not we should move forward with this? |
if anyone here needs some insight on the event code, feel free to ping me. I've some experience with it, (PR's in the past) and would also like to see this. ALSO the current mouseEnter/Leave logic is crazy I really don't think we should emulate it again for new features if possible. I've already spoken to @jimfb a bit about that though. For insight as well there is this outstanding PR which simplifies the mouse logic, it might be helpful stratgey for this case as well: #5762 |
Wait, are we trying to add focusIn/Out as analogs to mouseEnter/Leave? Personally I rarely want the behavior of native focus/blur events. I do however almost always want for focusEnter/Leave like events... |
@jquense Yeah I think that's the idea, the bigger question is that since the current onFocus and onBlur events bubble should those events just be renamed and have new events implemented for onFocus and onBlur that don't bubble? |
Thats where my confusion comes from. mouseEnter and mouseLeave do not bubble; that's much of the point of them, but they also do not behave like native focus/blur, which only fire when a the element that attached the handle gains focus not when it or any of its children gain focus. so I'm not sure why we'd need to invert the behavior, unless the goal is to not actually add something like focusEnter/Leave |
@M-Dahab I don't think you need
|
@craigkovatch this is precisely why we need |
FWIW, |
@jonathantneal's example curiously does not work as expected for me in Firefox 68, despite Firefox allegedly having support for the events. Can anyone else reproduce? |
@eneroth works fine for me on FF 68.0.1, with the caveat that FF does not focus buttons by default when you click them, so I had to tab to it to get the focusin handler to activate for that. |
@craigkovatch Alright, thanks. Here's the difference that I'm seeing: http://eneroth.com/gamX3cfz8yBAdiBx4qjxboGm/Firefox%20vs%20Chrome.mov |
@eneroth ok I see that too, but I think that is the same thing: buttons not focusing on click in Firefox -- so they wouldn't emit the bubbling focusin event necessary to keep the div displayed. i.e. the text field blurs but the clicked button doesn't focus. If you click into the text field you can tab into the revealed buttons. More bad news -- it's OS dependent: |
Easy to fix (as proposed here) document.addEventListener(
"click",
event => event.target.focus();
); |
I want to use onFocusOut to hide an https://caniuse.com/#search=focusout It seems that all modern navigators implements focusout |
The button focus behavior is inconsistent across operating systems and browsers. On macOS Firefox and Safari, the Input clear button was broken. A onFocusIn/onFocusOut solution is not possible due to React not supporting it. - https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#Clicking_and_focus - https://zellwk.com/blog/inconsistent-button-behavior/ - facebook/react#6410
Coming back to this thread, I'm not sure what is being asked here. If I understand correctly:
I'm not sure that everybody who commented this actually have the same request in mind. If this issue is still relevant to you, can you please restate what you are asking? Taking into account that "add |
I would have to think pretty hard to remember the case that motivated me to subscribe to this and to link it from #13525. I believe I had a case where I thought it would be nice if |
@gaearon my request is that React internally listen to |
Ahh, yes, @craigkovatch might actually be onto what was causing me trouble. |
This makes sense. The workaround is to check
All right, this is good to know. We've actually made that exact change on master so it should go out in 17. |
Yeah, that's why I brought it up in #13525 (comment) in the context of other breaking changes to align with how DOM events work. From that issue's description around changes to
|
In summary, I don't have a specific need and there are workarounds, but it could be nice to align with the DOM if that means fewer surprises. Of course, there's an argument to be made that the DOM events are surprising or unintuitive, but I'm not here to argue this either way. I'm all set! Glad to know |
If we are asking for things what I want is focusenter/focusleave like we have for mouse and pointer events. Given that they aren't stanardized tho it's probably not a good fit for react dom |
Yeah most of our recent work has been to align closer with the DOM so this is not a good fit at this stage. But there's been parallel work on better accessibility primitives so I think maybe it's in that scope. |
In React 17, It seems like this addresses the core of this issue (#6410 (comment)). It's fair that maybe it would be nice to eventually rename them to align with the browsers (and possibly, add non-bubbling versions). Please feel free to raise new issues for these topics. To try React 17, install Thanks to everyone for the very informative discussion! |
I made a small example demonstrating the current behavior based on #6410 (comment): https://codesandbox.io/s/strange-albattani-7tqr7?file=/src/App.js export default function App() {
return (
<div
tabIndex={1}
onFocus={(e) => {
console.log("focusin (self or child)");
if (e.currentTarget === e.target) {
console.log("focus (self)");
}
if (!e.currentTarget.contains(e.relatedTarget)) {
console.log("focusenter");
}
}}
onBlur={(e) => {
console.log("focusout (self or child)");
if (e.currentTarget === e.target) {
console.log("blur (self)");
}
if (!e.currentTarget.contains(e.relatedTarget)) {
console.log("focusleave");
}
}}
>
<input />
<input />
</div>
);
} Hopefully this helps as a quick reference when you need to decide on which check to use. I think there's definitely room for improvement here but if we're talking about API changes, a detailed RFC would be the best way to move this forward: http://github.com/reactjs/rfcs I'll lock the discussion so people don't lose this canonical answer when coming from Google, but feel free to file new issues if something is unclear or not working. |
Like mouse enter/leave, these are almost always what you want, not the
onFocus
andonBlur
events we currently expose. I run into this semi-frequently when actually doing product work. We should add them.The text was updated successfully, but these errors were encountered: