Skip to content
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

Focusout event on $(window) & $(document) : events call twice #4652

Closed
dutrieux opened this issue Apr 4, 2020 · 6 comments
Closed

Focusout event on $(window) & $(document) : events call twice #4652

dutrieux opened this issue Apr 4, 2020 · 6 comments

Comments

@dutrieux
Copy link

@dutrieux dutrieux commented Apr 4, 2020

Description

If I attach focusout event on the $(window) and $(document), each events are called twice.

I use jQuery v3.4.1 and I have the same problem with Firefox (v74.0.1) and Chrome (v80.0.3987.163).
If I use native Js, each events are called once.

Link to test case

https://jsbin.com/rilamarodi/1/edit?html,css,js,output
open console, focus input field and go out (remove focus) to see on console that each events are called twice.

focusout

@mgol
Copy link
Member

@mgol mgol commented Apr 5, 2020

Thanks for the report. To add to that, on Firefox if I focus the input & then either focus another application or Firefox DevTools, I see 4 occurrences of the document focusout & 5 of window focusout.

I initially thought it's related to the big refactor of focus event handling in #4279 but the issue is even in 3.0.0.

@dutrieux
Copy link
Author

@dutrieux dutrieux commented Apr 5, 2020

but the issue is even in 3.0.0.

in 2.2.4 too !

@mgol mgol added 3.x-only Discuss in Meeting labels Apr 6, 2020
@mgol
Copy link
Member

@mgol mgol commented Apr 6, 2020

OK, so this issue first occurred in jQuery 1.11.0/2.1.0. It was working fine in 2.0.3. It was introduced in 1cecf64. That commit broke focusout handling on non-element targets, though, a fix was added in c2aca17 so that's the first commit where you can see this issue.

The issue doesn't exist on master which is future jQuery 4.0.0 because we removed the focusin/focusout shim there in PR #4362. We didn't backport the removal to the 3.x line as it changes the relative order between focusin/focusout & focus/blur events.

Taking into account that:

  • the issue should be relatively easy to work around
  • this is the first report I see about it
  • this change landed 9 years ago
    it might be best to leave it as-is for the 3.x branch.

If you find a simple patch fixing the current 3.x implementation, we'd review the PR, though.

Marking for discussion at today's team meeting.

@mgol
Copy link
Member

@mgol mgol commented Apr 6, 2020

Oh, actually this is pretty simple: the doc variable in https://github.com/jquery/jquery/blob/3.4.1/src/event/focusin.js#L30 will match document for document & window for window and we'd like them to be grouped by document. A fix incoming, it will be quite simple.

That also explains why this was unreported so long - the issue happens only if you attach the focusout (or focusin) handler on both window & document, or on window & another regular node.

@mgol mgol removed the Discuss in Meeting label Apr 6, 2020
@mgol
Copy link
Member

@mgol mgol commented Apr 6, 2020

PR: #4656

@mgol mgol added this to the 3.5.0 milestone Apr 6, 2020
mgol added a commit that referenced this issue Apr 6, 2020
The `doc` variable in:
https://github.com/jquery/jquery/blob/3.4.1/src/event/focusin.js#L30
matched `document` for `document` & `window` for `window`, creating two
separate wrapper event handlers & calling handlers twice if at least one
`focusout` or `focusin` handler was attached on *both* `window` & `document`,
or on `window` & another regular node.

Also, fix the "focusin from an iframe" test to actually verify the behavior
from commit 1cecf64 - the commit that
introduced the regression - to make sure we don't regress on either front.

Fixes gh-4652
Closes gh-4656
@mgol
Copy link
Member

@mgol mgol commented Apr 6, 2020

Fixed via #4656, landed in 9e15d6b.

@mgol mgol closed this as completed Apr 6, 2020
mgol added a commit to mgol/jquery that referenced this issue Apr 6, 2020
Backport tests from a jQuery 3.x fix that's not needed on `master`.

Also, fix the "focusin from an iframe" test to actually verify the behavior
from commit 1cecf64 - the commit that
introduced the regression - to make sure we don't regress on either front.

The main part of the modified test was checking that focusin handling in an
iframe works and that's still checked. The test was also checking that it
doesn't propagate to the parent document, though, and, apparently, in IE it
does. This one test is now blacklisted in IE.

(cherry picked from 9e15d6b)
(cherry picked from 1a4f10d)

Ref jquerygh-4652
Ref jquerygh-4656
mgol added a commit that referenced this issue Apr 27, 2020
Backport tests from a jQuery 3.x fix that's not needed on `master`.

Also, fix the "focusin from an iframe" test to actually verify the behavior
from commit 1cecf64 - the commit that
introduced the regression - to make sure we don't regress on either front.

The main part of the modified test was checking that focusin handling in an
iframe works and that's still checked. The test was also checking that it
doesn't propagate to the parent document, though, and, apparently, in IE it
does. This one test is now blacklisted in IE.

(cherry picked from 9e15d6b)
(cherry picked from 1a4f10d)

Ref gh-4652
Ref gh-4656
Closes gh-4657
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants