Event: Leverage native events for focus and blur #4279
Conversation
mgol
left a comment
mgol
left a comment
There was a problem hiding this comment.
I've finally went through this PR, nice work! There's some clever code here, it's not that easy to follow! I think I understand most of it now, though. I left comments where I have doubts.
P.S. How do you plan to land it, considering the author of the first commit is different?
| // synthetic events by interrupting progress until reinvoked in response to | ||
| // *native* events that it fires directly, ensuring that state changes have | ||
| // already occurred before other listeners are invoked. | ||
| function leverageNative( el, type, forceAdd, allowAsync ) { |
There was a problem hiding this comment.
This will hurt readability but since forceAdd may be only false or returnTrue, we could squash it with allowAsync, changing the signatures to:
leverageNative( el, type, allowAsync ); // old `leverageNative( el, type, false, allowAsync )`
leverageNative( el, type, false ); // old `leverageNative( el, type, returnTrue )`That may save some size.
There was a problem hiding this comment.
So, something like this?
function leverageNative( el, type, allowAsync ) {
// Missing allowAsync indicates a trigger call, which must force setup through jQuery.event.add
if ( !allowAsync ) {
jQuery.event.add( el, type, returnTrue );
return;
}
// Register the controller as a special namespace-universal handler
dataPriv.set( el, type, false );
jQuery.event.add( el, type, {…} );
}I think I already tried that, but will specifically look into it.
There was a problem hiding this comment.
Yes, I think I meant something like that. If this doesn't help size then consider the request moot.
| ns.remove(); | ||
| } ); | ||
|
|
||
| QUnit.test( "Check order of focusin/focusout events", function( assert ) { |
There was a problem hiding this comment.
Has the order changed? Why is this being removed?
There was a problem hiding this comment.
Yes, I think it now follows native behavior, further cementing my good feelings about #4300 (comment) .
There was a problem hiding this comment.
I guess it's fine as we never guaranteed the order and it was different in IE anyway but I'd like to make it clear we're changing it.
@jquery/core what do you think?
There was a problem hiding this comment.
If the order changes, this has the potential to break existing code. That kinda smells of a major version to me.
There was a problem hiding this comment.
I don't think we've ever documented a guaranteed order, and moreover I don't think we're consistent across supported browsers right now. But I can try restoring this test to see what happens.
There was a problem hiding this comment.
Technically speaking it's not a breaking change as we haven't documented the order. That said, if something is consistent between browsers, people tend to start depending on that and changing the behavior may break them. @gibson042 is right the order is different between browsers right now but it's only different between IE & the rest. If a site doesn't support IE (there are more & more of such sites), for their purposes the current order is consistent between browsers. So there's risk involved here.
There was a problem hiding this comment.
To add to the above, while the current order is inconsistent between IE & others, what's consistent (& verified by the removed test) is that focusin fires before focus & focusout before blur.
mgol
left a comment
mgol
left a comment
There was a problem hiding this comment.
I added a few comments & resolved the discussions where I'm OK with the outcome.
| ns.remove(); | ||
| } ); | ||
|
|
||
| QUnit.test( "Check order of focusin/focusout events", function( assert ) { |
There was a problem hiding this comment.
I guess it's fine as we never guaranteed the order and it was different in IE anyway but I'd like to make it clear we're changing it.
@jquery/core what do you think?
|
@gibson042 One more thing, a comment that I added in my original PR and I don't see the reply:
|
To clarify - do you want to land it as two commits: a re-land of the original one plus your fixes or just squash it to one commit authored by you since the original changes are credited via Git history anyway? |
|
@gibson042 @timmywil I restored the removed order tests locally and checked in Chrome & Firefox. The order of events is as follows:
When comparing to the orders from #4300, remember #4300 presents
while the blurring one is:
Therefore, the order is not native; it's closer to the existing jQuery one but it has one additional second |
|
The second An additional complication are the |
(cherry picked from commit b442aba) Ref jquerygh-3494 Fixes jquerygh-3423
mgol
force-pushed
the
3.4-native-focus-blur-events
branch
from
9de7bba to
2846a6b
Compare
mgol
changed the title
|
I rebased the PR & restored the |
Accept that focusin handlers may fire more than once for now.
mgol
force-pushed
the
3.4-native-focus-blur-events
branch
from
2846a6b to
897f0b0
Compare
|
The current version of the PR passes tests when run via Karma or by opening It happens in Chrome & Safari, it doesn't happen in Firefox/Edge/IE. It also succeeds in old mobile Chrome/Safari, see my local table (ignore the AJAX failures in IE, that's a BrowserStack issue): I'm trying to debug that now... |
|
OK, it might be a local caching issue, let me try to land it and see on our real TestSwarm instance... |
5 participants
Summary
Builds upon #4277 (diff). This is currently super sloppy, but it works!
Fixes gh-1741
Size reduction ideas:
triggermethodssafeActiveElementwith something for direct use inleverageNative, e.g. accepting (element, eventType) and returning maybeAsyncleverageNativeparameter as wellthiswithelin theleverageNativehandler (but only if that persuades minification to duplicate more character sequences)leverageNativehandler for more gzip-friendly minification outputChecklist
If needed, a docs issue/PR was created at https://github.com/jquery/api.jquery.com