Datepicker: Hide the UI on destroy

[porterclev]

Issue: https://github.com/jquery/jquery-ui/issues/2178 - date picker remains after being destroyed

We confirmed through git bisect that this commit, https://github.com/jquery/jquery-ui/commit/817ce38, introduced a bug while fixing a memory leak with the date picker. This commit sets the current instance to null, https://github.com/jquery/jquery-ui/blob/5665215a8560832193735d9507a90d10e03b90c8/ui/widgets/datepicker.js#L440, to remove the date picker logic but fails to remove the UI widget.

Our proposed fix is to hide the date picker when the destroy function using $.datepicker._hideDatepicker(); so the user can no longer hover over a "ghost" date picker that can no longer be interacted with.

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: porterclev / name: Porter Clevidence (5e4f1b86296c5a977abc7efaff671ea9004d4d37, d19d80e5e646ece9b18b1bf97cbd6df86b5147b9, ae27ec2fd91f0839293332e9aeee2f0aa3046adf, 340941831cdb91d3f7f859c4fd45fce8a75c3605)
• :white_check_mark: login: mgol / name: Michał Gołębiowski-Owczarek (7be414808e1bb7c0653b0b7ad6a4599885264a22)

[porterclev]

@fnagel

[DawnSovereign]

The following text is the understanding we have of what happened. The date picker was getting destroyed, but a lingering reference to it was kept alive (curInst). This meant it was a memory leak. The PR that fixed the memory leak did not handle the case where the datepicker was already open. It clears curInst but it does not actually destroy or remove the DOM element

To answer your question about setting this.curInst to null; curInst is being set to null, but the datepicker still thinks it is open, so other parts of the code still access _curInst which is why we set this.curInst to null.

In order to directly address how we could get rid of the daterpicker UI that's still open, we get rid of the div instance by destroying it from the DOM and clearing dp div by setting it to null.

Here are the screenshots with our fix for before and after the div instance is destroyed:

Here is a screenshot of the stack trace:

[OmarShehata]

Here is a screenshot of the stack trace:

@DawnSovereign are you able to get this stack trace with the local, unminified code? or click on it and see what function it's in, and find the corresponding function in the source code?

from the screenshot, it looks like it's this function, right?

https://github.com/jquery/jquery-ui/blob/5e4f1b86296c5a977abc7efaff671ea9004d4d37/ui/widgets/datepicker.js#L2166-L2177

[mgol]

You can also see that existing tests are not passing, please make sure they do.

[porterclev]

@mgol Thanks for taking a look. We've pushed a unit test that reproduces the issue. It fails in main and passes in this branch with the fix.

We've reverted the fix back to hiding the datepicker instead of destroying the div. This is because several tests expect to re-use the element after it is destroyed. If we want to properly remove the element when the datepicker is destroyed, it looks like this may be a breaking change? I can open a ticket for that separately with more details.

The underlying issue is that destroying the datepicker by itself does not hide it. The current tests in main don't show this problem because they always hide the datepicker first before destroying it. So it is possible for a user to work around this problem at the moment by hiding before destroying.

The long term solution is to remove the element when the datepicker is destroyed, and update the tests, and mark this as a (potential?) breaking change.

[fnagel]

At first I thought that just hiding the elements instead of destroying sounds not ideal, but then I've seen the comment about "This is because several tests expect to re-use the element after it is destroyed." so the change makes sense to me.

It's not ideal that the destroy method does not really destroy everything (elements, events, ...) like other widgets do, but the Datepicker does not use the widget factory and always was a little special.

So to me, this change improves the current state and fixes a bug introduces with 1.13.x. Only the test comment is a little confusing (see my review).

@mgol We might want to squash the commits when merging this.

[mgol]

@fnagel Was the comment your only concern to address before merging? I get your comment about destroying; I had a look at the code and it seemed not that easy to retrofit it into the existing design. _hideDatepicker may use an animation and we'd have to destroy only when it's done, but it doesn't provide any hook to do so. We'd have to add one, then add tests, also fix existing tests to not depend on it existing... Maybe let's leave it as it is for now.

Please approve if you have no further concerns to address.

[fnagel]

@mgol I'm fine with merging it as it is. Datepicker was always special (as mentioned before, no widget factory in use) and SHOULD have been replaced with the new calendar and datepicker widget but never has.

Not sure what little surprises will come up when we start changing how the structure and design currently looks ;-)

[mgol]

Just to make sure, I checked myself what was the cause of the original issue. Datepicker attaches a global mousedown on document: https://github.com/jquery/jquery-ui/blob/cd41c45d917d18839ab800816070db09a359d5bf/ui/widgets/datepicker.js#L2201 It never removes it. This handler hides the datepicker in certain conditions - but the first thing it checks is whether $.datepicker._curInst is truthy: https://github.com/jquery/jquery-ui/blob/cd41c45d917d18839ab800816070db09a359d5bf/ui/widgets/datepicker.js#L1005-L1022 Since #1852 made _curInst cleared, this method stopped doing anything.

Some parts of the description from #2178 are incorrect, though - in 1.12.1, the datepicker UI was not hidden on destroy either! It was just hidden after the first mousedown. If you just hovered over the datepicker or clicked on any of its elements, you'd get lots of console errors even in 1.12.1.

Therefore, this PR actually changes the behavior compared to 1.12.1 which was already partially broken. I think it's a reasonable change - the datepicker UI should hide when destroying the widget - but let's call it out.