v.3.4.1 focus() add focus listener to element

[jucliu]

Description

Hey,

In 3.4.1, when moving focus, the focused item is added 'focus' listener but it doesn't happen on jquery 3.3.0.
In below example, move focus to the first button and third button, extra focus listeners are added to them. Please take a look. Thanks

Before:

After:

Link to test case

jquery 3.4.1: https://jsfiddle.net/ljccccc/cfjygwx2/8/

[timmywil]

Thanks for opening an issue. I tried this in Chrome but I don't see any event listeners getting added. For support, go to stackoverflow.com.

[jucliu]

Thanks @timmywil

It happens on chrome 76. Could you please refer to the attached gif and take a look at this issue?

Dev tools won't refresh newly added listener. Please click another element in Dev tool -> 'Elements' before checking the focused element listeners.

[mgol]

I confirm it, both in Chrome & Firefox. To reproduce, click on the first button & then press Tab which will focus the third button. Then you can blur or do anything you'd like and the two focus listeners on the third button stay. Firefox even shows 3 handlers. Reopening.

[mgol]

This is most likely caused by #4279 (cc @gibson042).

[timmywil]

I see it now. It does seem odd there would be more than one listener.

[mgol]

One listener would be a bug as well; there should be none. -- Michał Gołębiowski-Owczarek

[timmywil]

@mgol Not necessarily. We are claiming the listener in setup whenever focus is used to fix other issues. It should be harmless as long as it doesn't leak more listeners.

[gibson042]

It is expected that a listener is added by .trigger, because leverageNative is necessary to propagate rest arguments to handlers. It is also expected for there to be two listeners when .trigger is used before .on, because the .trigger-based path forces setup by adding a dummy returnTrue listener:

jquery/src/event.js

Line 525 in cf84696

// Missing expectSync indicates a trigger call, which must force setup through jQuery.event.add

We could try cleaning these up with a teardown hook, but I would rather not go down that path unless this is actively causing problems.

[jucliu]

hey @gibson042 thank you for looking at this issue.
adding extra focus listeners will cause memory leak. Please fix it. Thanks!!

[mgol]

@jucliu Memory leaks are an issue in a scenario where a running app takes more & more memory while not getting continuously larger by itself. I don't see such an issue here; you may get up to two focus listeners attached when the event gets triggered on an element. It happens only on those elements and will get cleaned up when the element is removed. Do you have a scenario where this causes a significant memory overhead?

[timmywil]

Closing as there will likely be no changes needed here. The listeners are expected and this is not a memory leak.

[jucliu]

My scenario is that all elements already have 2 focus listeners on a single page application. The .focus() is used to implemented accessibility 'Tab' keyboard traversal functions for this SPA. So many elements on this SPA will be added extra focus listeners when 'Tab' traversal is called and these newly added listeners exist on the SPA until the application is closed. Please help me clean up the unnecessary listeners. Thanks

[mgol]

Are you sure those extra listeners are a real issue, though? Did memory usage increase significantly compared to the previous jQuery version? These are important questions.

[mgol]

This issue is causing failures in the jQuery UI test suite, BTW, as that suite checks whether instantiating various widgets followed by their destruction leaves the DOM in its initial state; one thing that is checked is attached events and there's one remaining focus & blur event even after destruction.

See e.g. http://swarm.jquery.org/result/3615149 (or https://web.archive.org/web/20200624124754/http://swarm.jquery.org/result/3615149 in case the Swarm result gets removed).

Perhaps we could remove that event during teardown? I'll reopen & unlock to discuss the issue further.

[mgol]

To make it precise, what's causing a failure is those handlers being present after jQuery handlers are removed so fixing that issue wouldn't necessarily do everything the OP requested but it should be close enough for people doing cleanup tests.

[timmywil]

We discussed this in the meeting. Core will stay as-is and we will adjust the UI tests. See jquery/jquery-ui#1930