-
Notifications
You must be signed in to change notification settings - Fork 20.6k
Event: Make focus re-triggering not focus the original element back #4813
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
Conversation
+58 bytes. @gibson042 any ideas for size improvements? |
If during a focus handler another focus event is triggered: ```js elem1.on( "focus", function() { elem2.trigger( "focus" ); } ); ``` due to their synchronous nature everywhere outside of IE the hack added in jquerygh-4279 to leverage native events causes the native `.focus()` method to be called last for the initial element, making it steal the focus back. To resolve this, we now skip calling the native method if focus was changed. A side effect of this change is that now `focusin` will only propagate to the document for the last focused element. This is a change in behavior but it also aligns us better with how this works with native methods. Fixes jquerygh-4382 Ref jquerygh-4279
8f29ce1
to
c835b62
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, I forgot about _default
. The special event plumbing really helps here. I think programmatic forcing of focus is an anti-pattern in general, but this doesn't seem to be too bad as far as code goes.
Yeah, it's only used for |
OK, I pushed another version of this change that just returns All the tests pass, focus is changed and the @dmethvin can you also re-review this? |
@@ -746,6 +746,12 @@ jQuery.each( { focus: "focusin", blur: "focusout" }, function( type, delegateTyp | |||
return true; | |||
}, | |||
|
|||
// Suppress native focus or blur as it's already being fired | |||
// in leverageNative. | |||
_default: function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I hadn't thought about that. The _default
action happens at the very very very end of this process so as long as leverageNative
always does the work for us earlier we don't need to do it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I trust @dmethvin here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love this solution.
If during a focus handler another focus event is triggered: ```js elem1.on( "focus", function() { elem2.trigger( "focus" ); } ); ``` due to their synchronous nature everywhere outside of IE the hack added in gh-4279 to leverage native events causes the native `.focus()` method to be called last for the initial element, making it steal the focus back. Since the native method is already being called in `leverageNative`, we can skip that final call. This aligns with changes to the `_default` method for the `click` event that were added when `leverageNative` was introduced there. A side effect of this change is that now `focusin` will only propagate to the document for the last focused element. This is a change in behavior but it also aligns us better with how this works with native methods. Fixes gh-4382 Closes gh-4813 Ref gh-4279 (cherry picked from commit dbcffb3)
The `_default` function in the special event settings for focus/blur has always returned `true` since jquerygh-4813 as the event was already being fired from `leverageNative`. However, that only works if there's an active handler on that element; this made a quick consecutive call: ```js elem.on( "focus", function() {} ).off( "focus" ); ``` make subsequent `.trigger( "focus" )` calls to not do any triggering. The solution, already used in a similar `_default` method for the `click` event, is to check for the `dataPriv` entry on the element for the focus event (similarly for blur). Fixes jquerygh-4867
The `_default` function in the special event settings for focus/blur has always returned `true` since gh-4813 as the event was already being fired from `leverageNative`. However, that only works if there's an active handler on that element; this made a quick consecutive call: ```js elem.on( "focus", function() {} ).off( "focus" ); ``` make subsequent `.trigger( "focus" )` calls to not do any triggering. The solution, already used in a similar `_default` method for the `click` event, is to check for the `dataPriv` entry on the element for the focus event (similarly for blur). Fixes gh-4867 Closes gh-4885
The `_default` function in the special event settings for focus/blur has always returned `true` since jquerygh-4813 as the event was already being fired from `leverageNative`. However, that only works if there's an active handler on that element; this made a quick consecutive call: ```js elem.on( "focus", function() {} ).off( "focus" ); ``` make subsequent `.trigger( "focus" )` calls to not do any triggering. The solution, already used in a similar `_default` method for the `click` event, is to check for the `dataPriv` entry on the element for the focus event (similarly for blur). Fixes jquerygh-4867 Closes jquerygh-4885 (cherry picked from commit e539bac)
Summary
If during a focus handler another focus event is triggered:
due to their synchronous nature everywhere outside of IE the hack added in
gh-4279 to leverage native events causes the native
.focus()
method to becalled last for the initial element, making it steal the focus back. Since
the native method is already being called in
leverageNative
, we can skip thatfinal call.
This aligns with changes to the
_default
method for theclick
event thatwere added when
leverageNative
was introduced there.A side effect of this change is that now
focusin
will only propagate to thedocument for the last focused element. This is a change in behavior but it also
aligns us better with how this works with native methods.
Fixes gh-4382
Ref gh-4279
Checklist
If needed, a docs issue/PR was created at https://github.com/jquery/api.jquery.com