Skip to content

Commit

Permalink
Event: Make focus re-triggering not focus the original element back
Browse files Browse the repository at this point in the history
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)
  • Loading branch information
mgol committed Dec 7, 2020
1 parent 07829a1 commit 2fadbc0
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 0 deletions.
6 changes: 6 additions & 0 deletions src/event.js
Original file line number Diff line number Diff line change
Expand Up @@ -778,6 +778,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() {
return true;
},

delegateType: delegateType
};
} );
Expand Down
34 changes: 34 additions & 0 deletions test/unit/event.js
Original file line number Diff line number Diff line change
Expand Up @@ -3263,6 +3263,40 @@ QUnit.test( "native-backed events preserve trigger data (gh-1741, gh-4139)", fun
}, 50 );
} );

QUnit.test( "focus change during a focus handler (gh-4382)", function( assert ) {
assert.expect( 2 );

var done = assert.async(),
select = jQuery( "<select><option selected='selected'>A</option></select>" ),
button = jQuery( "<button>Focus target</button>" );

jQuery( "#qunit-fixture" )
.append( select )
.append( button );

select.on( "focus", function() {
button.trigger( "focus" );
} );

jQuery( document ).on( "focusin.focusTests", function( ev ) {
// Support: IE 11+
// In IE focus is async so focusin on document is fired multiple times,
// for each of the elements. In other browsers it's fired just once, for
// the last one.
if ( ev.target === button[ 0 ] ) {
assert.ok( true, "focusin propagated to document from the button" );

This comment has been minimized.

Copy link
@IgnatovDan

IgnatovDan Mar 23, 2021

IMHO it's an incorrect approach to check event args:

  • test will be passed if 'focusin' event will not occurs
  • test will be passed if 'focusin' event will not occurs with the required 'ev.target' value

I would prefer to move such checks out of the event handler and use a variable so 'assert' will be executed always.
Something like this:

var focusInEventResult = false;
jQuery( document ).on( "focusin.focusTests", function( ev ) {
	if ( ev.target === button[ 0 ] ) {
		focusInEventResult = true;
..
assert.ok( focusInEventResult, "focusin propagated to document from the button" );
done();

This comment has been minimized.

Copy link
@mgol

mgol Mar 24, 2021

Author Member

Tests will not pass if this assertion doesn't fire since the number of expected assertions is declared up front; that's one of the cool features of QUnit that many other test frameworks are missing.

}
} );

select.trigger( "focus" );

setTimeout( function() {
assert.strictEqual( document.activeElement, button[ 0 ], "Focus redirect worked" );
jQuery( document ).off( ".focusTests" );
done();
} );
} );

// TODO replace with an adaptation of
// https://github.com/jquery/jquery/pull/1367/files#diff-a215316abbaabdf71857809e8673ea28R2464
( function() {
Expand Down

0 comments on commit 2fadbc0

Please sign in to comment.