Skip to content

Fixes #12569, Feature detect for oldIE/Chrome event bubbling #1054

Closed
wants to merge 5 commits into from

5 participants

@markelog
jQuery Foundation member
markelog commented Dec 4, 2012

CSP error is not triggered here, div element has onsubmit and onchange properties, but does not have onfocusin, but it's fine, because you can't bind focusin/focusout events by defining onfocusin/onfocusout prop. Meaning that string in onfocusin will not be "evaled".

Also, in this patch, jQuery.support.focusinBubbles will be true for Chrome and Opera.

/cc @dmethvin

@rwaldron
jQuery Foundation member
rwaldron commented Dec 5, 2012

@orkel this looks great. Can you add a comment in the source that explains the removal of the IE inference condition—Thanks!

@dmethvin
jQuery Foundation member
dmethvin commented Dec 5, 2012

Ah! So Chrome has decided that they just won't support the magical onfocusin inline, so it won't be turned into a function!

@dmethvin dmethvin closed this Dec 5, 2012
@rwaldron
jQuery Foundation member
rwaldron commented Dec 5, 2012

Don't think you meant to close this ;)

@rwaldron rwaldron reopened this Dec 5, 2012
@gibson042 gibson042 commented on an outdated diff Dec 5, 2012
src/support.js
@@ -259,14 +250,23 @@ jQuery.support = (function() {
body.style.zoom = 1;
}
+ if ( !support.focusinBubbles && div.addEventListener ) {
+ div.addEventListener( "focusin", function() {
+ support.focusinBubbles = true;
+ }, false );
+
+ div.appendChild( input );
+ input.focus();
+ }
+
@gibson042
jQuery Foundation member
gibson042 added a note Dec 5, 2012

Thanks for this pull!

You can save six bytes by moving the new block to be the first test after defining div and using isSupported instead of focusinBubbles (since they're identical at that point). If you do so, though, please include a comment to that effect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markelog
jQuery Foundation member
markelog commented Dec 5, 2012
this looks great

Apparently i submitted this patch too soon.
I rewrite it, with a small refactoring of support module, now it's bigger then i want it to be.

@gibson042 gibson042 and 1 other commented on an outdated diff Dec 6, 2012
+ teardown: function() {
+ if ( --attaches === 0 ) {
+ document.removeEventListener( orig, handler, true );
+ }
+ }
+ };
+ } else {
+
+ // Correct focusin/focusout "bubbling"
+ jQuery.event.special[ orig ].preDispatch = function( event, handlers ) {
+
+ // If we have corresponded events, wait until they fire first
+ if ( handlers[ fix ] ) {
+ setTimeout(function( self ) {
+ jQuery.event.dispatch.call( self, event );
+ }, 0, this );
@gibson042
jQuery Foundation member
gibson042 added a note Dec 6, 2012

On the first point, I'm saying that the anonymous function passed to setTimeout should not have any parameters, but instead close over a var self = this; defined in the outer preDispatch function.

On the second, my intuition was wrong, but you can still save 27 bytes by replacing function fixFocus() {… with fixFocus = function() {…, 5 from moving the fixFocus() call to the very end, and 6 from rearranging jQuery.event.dispatch variables (a total of -38): gibson042@orkel:12569...gibson042:orkel_12569

@markelog
jQuery Foundation member
markelog added a note Dec 6, 2012
On the first point

Yeah, i get that, i'm asking why two arguments should be passed to setTimeout function.

You propose this –

setTimeout(function() {
    jQuery.event.dispatch.call( self, event );
}, 0 );

right?

Why not –

setTimeout(function() {
    jQuery.event.dispatch.call( self, event );
});

?

total of -38

Awesome! Can you open pull request to my branch?

@gibson042
jQuery Foundation member
gibson042 added a note Dec 6, 2012

Pass the 0 for consistency, compressibility, and maximum browser support. And I tried a pull request, but couldn't select your repo. Just cherry-pick 775f634.

@markelog
jQuery Foundation member
markelog added a note Dec 6, 2012
maximum browser support

All browsers are support that

compressibility

It saves additional 2 bytes

consistency

And 10 bytes if this style will be used in all instances of setTimeout called with second argument being 0 or 1 in the source.

@gibson042
jQuery Foundation member
gibson042 added a note Dec 6, 2012

Cool. Do it as , 0 ) here and open a new PR to standardize the other calls... you might even want to use #10417.

@markelog
jQuery Foundation member
markelog added a note Dec 6, 2012

i will start working on that, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markelog
jQuery Foundation member

I cherry-picked @gibson042 commit and simplified couple more things

@dmethvin
jQuery Foundation member

Ouch! This is now +58 against current master. Also, from painful recent experience with focus/blur I am worried this may change behavior yet again. Our only reason for wanting this is that we are currently not using native focusin for Chrome, is that right?, I'm not sure I want to fix it that badly

@markelog
jQuery Foundation member

@dmethvin

Our only reason for wanting this is that we are currently not using native focusin for Chrome, is that right?

All WebKit-based browsers and Opera.

from painful recent experience with focus/blur I am worried this may change behavior yet again.

Fair enough, although i'm sure this fix would be sufficent.

This PR accommodate two patches:
1) Improve feature detect for oldIE event bubbling (#12569)
2) Stop shimming focusin/focusout events

First one can actually save some bytes, second, not so much (+120 bytes or so).
I will try to work on first one in different pull, and forget the second one.

@markelog markelog closed this Dec 12, 2012
@Krinkle
jQuery Foundation member
Krinkle commented Dec 13, 2013

Followed up by pull request gh-1076.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.