-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Exception when selecting Quick Open result by clicking #1384
Comments
Surprisingly, it looks like this was caused by 0159dc2. Removing a DOM node with jQuery's remove(), instead of the generic DOM API removeChild(), has the side effect of stripping off the node's stored jQuery data bag. When we close the dialog this way, the smart-autocomplete itemSelected handler runs a moment later and crashes on the missing data. (Or in the Go to Line case, it's the resultsReady handler that runs asynchronously a moment later still). It seems like we can't backtrack to the old removal, though, because jQuery basically requires all removals to use its own APIs -- it actually leaks memory (everything you stored on the node(s) via |
Reviewed - Peter has a solution. |
I am seeing 2 exceptions:
|
Both are a side effect of 0159dc2 -- the smart-auto-complete plugin isn't expecting its metadata to be cleared before it detaches all its listeners, which I think is just a special case of it not expecting the popup to be closed by anyone other than itself. It relies on global focus & click events to decide when to close the popup, which is problematic for us because we hide the popup in other cases too, like key events. We weren't seeing these bugs before 0159dc2 only because because we were never clearing the metadata (leaking memory). My original fix was to adjust the timing of when the metadata is removed until after smart-autocomplete received the focus loss event that makes it decide to agree with us on closing the popup. But this doesn't fix Randy's case #1 since EditorManager.focusEditor() is a no-op when there's no editor to focus (and jQuery apparently doesn't dispatch focus loss events when removing an item from the DOM removes its focus -- which seems like a bug to me). Anyway, I think the real right fix is to explicitly tell smart-autocomplete to close the popup itself before removing the text field from the DOM. I don't think it has an official API for this, but I think I see a way to trick it. I'll take a look tomorrow. |
Any Smart Autocomplete code that runs after $.remove() will fail because remove() strips off metadata that it expects to find on the text input element. The fix is (a) to forcibly close Smart Autocomplete every time we're closing the search bar, and (b) only fully clean up the search bar after a timeout, to let Smart Autocomplete's event handlers finish running. Also, rename handleDocumentClick() since it actually handles mouseDown, and clarify its comment.
Fix bug #1384 (Exception when selecting Quick Open result by clicking)
Fix has been merged -- closing |
Result: exception in jquery.smart-autocomplete.js
Expected: no exception -- it worked fine a few sprints ago
Alternative repro steps:
The text was updated successfully, but these errors were encountered: