Skip to content

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

Merged
merged 2 commits into from
Dec 7, 2020

Conversation

mgol
Copy link
Member

@mgol mgol commented Dec 1, 2020

Summary

If during a focus handler another focus event is triggered:

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
Ref gh-4279

Checklist

@mgol mgol added Event Needs review Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. labels Dec 1, 2020
@mgol mgol added this to the 3.6.0 milestone Dec 1, 2020
@mgol mgol self-assigned this Dec 1, 2020
@mgol
Copy link
Member Author

mgol commented Dec 1, 2020

+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
@mgol mgol force-pushed the focus-retrigger-4382 branch from 8f29ce1 to c835b62 Compare December 1, 2020 16:26
Copy link
Member

@dmethvin dmethvin left a 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.

@mgol
Copy link
Member Author

mgol commented Dec 2, 2020

Wow, I forgot about _default.

Yeah, it's only used for click at the moment, I'm adding a second entry here. 🙂

@mgol
Copy link
Member Author

mgol commented Dec 2, 2020

OK, I pushed another version of this change that just returns true in the focus & blur _default unconditionally, it's only +6 bytes. This works because we already call the native method in leverageNative and we don't need to call it again. For click, _default already returns true in situations where leverageNative is used.

All the tests pass, focus is changed and the focusin event propagates to document properly. Am I missing anything? If @gibson042 was able to look at it, that'd be great.

@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() {
Copy link
Member

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.

Copy link
Member

@timmywil timmywil left a 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

Copy link
Member

@gibson042 gibson042 left a 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.

@mgol mgol removed Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. Needs review labels Dec 7, 2020
@mgol mgol merged commit dbcffb3 into jquery:master Dec 7, 2020
@mgol mgol deleted the focus-retrigger-4382 branch December 7, 2020 19:28
mgol added a commit that referenced this pull request Dec 7, 2020
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)
@mgol
Copy link
Member Author

mgol commented Dec 7, 2020

Landed on master in dbcffb3 & on 3.x-stable in 2fadbc0.

mgol added a commit to mgol/jquery that referenced this pull request May 7, 2021
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
mgol added a commit that referenced this pull request May 10, 2021
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
mgol added a commit to mgol/jquery that referenced this pull request May 10, 2021
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

bug with focus() method in a specific case in 3.4.0
4 participants