fix(ui): fix event listener leak on dropdown re-open
This commit is contained in:
@@ -45,6 +45,7 @@ export class UIManager {
|
||||
elements: InfoPanelElements;
|
||||
nodeSelector: NodeSelector;
|
||||
currentDropdown: HTMLDivElement | null = null;
|
||||
private currentDropdownCleanup: (() => void) | null = null;
|
||||
onNodeSelected: ((nodeId: number) => void) | null = null;
|
||||
// Track active widget editors for cleanup
|
||||
private activeEditors: Map<string, WidgetEditorInstance> = new Map();
|
||||
@@ -1464,7 +1465,7 @@ export class UIManager {
|
||||
item.addEventListener('click', (e) => {
|
||||
e.stopPropagation();
|
||||
this.hideDropdown();
|
||||
cleanup();
|
||||
// cleanup() is called by hideDropdown
|
||||
|
||||
// Set selected node in state
|
||||
this.stateManager.setSelectedNode(node.id);
|
||||
@@ -1491,13 +1492,20 @@ export class UIManager {
|
||||
if (this.elements.panel) {
|
||||
this.elements.panel.removeEventListener('mousedown', panelCloseHandler, true);
|
||||
}
|
||||
// Clear the global reference
|
||||
if (this.currentDropdownCleanup === cleanup) {
|
||||
this.currentDropdownCleanup = null;
|
||||
}
|
||||
};
|
||||
|
||||
// Store cleanup globally so hideDropdown can call it
|
||||
this.currentDropdownCleanup = cleanup;
|
||||
|
||||
// Close on click outside - use mousedown with capture to ensure it fires before other handlers
|
||||
const closeHandler = (e: MouseEvent) => {
|
||||
if (!dropdown.contains(e.target as Node) && !anchorElement.contains(e.target as Node)) {
|
||||
this.hideDropdown();
|
||||
cleanup();
|
||||
// Cleanup is now handled by hideDropdown calling this.currentDropdownCleanup
|
||||
}
|
||||
};
|
||||
|
||||
@@ -1505,7 +1513,7 @@ export class UIManager {
|
||||
const panelCloseHandler = (e: MouseEvent) => {
|
||||
if (!dropdown.contains(e.target as Node) && !anchorElement.contains(e.target as Node)) {
|
||||
this.hideDropdown();
|
||||
cleanup();
|
||||
// Cleanup is now handled by hideDropdown calling this.currentDropdownCleanup
|
||||
}
|
||||
};
|
||||
|
||||
@@ -1544,7 +1552,7 @@ export class UIManager {
|
||||
e.preventDefault();
|
||||
e.stopPropagation();
|
||||
this.hideDropdown();
|
||||
cleanup();
|
||||
// Cleanup is now handled by hideDropdown
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -1591,6 +1599,12 @@ export class UIManager {
|
||||
* Hide the current dropdown.
|
||||
*/
|
||||
hideDropdown(): void {
|
||||
// Run cleanup if it exists
|
||||
if (this.currentDropdownCleanup) {
|
||||
this.currentDropdownCleanup();
|
||||
this.currentDropdownCleanup = null;
|
||||
}
|
||||
|
||||
if (this.currentDropdown && this.currentDropdown.parentNode) {
|
||||
this.currentDropdown.parentNode.removeChild(this.currentDropdown);
|
||||
this.currentDropdown = null;
|
||||
|
||||
@@ -126,17 +126,41 @@ describe('UIManager Dropdown Accessibility', () => {
|
||||
expect(document.activeElement).toBe(otherInput);
|
||||
|
||||
// Arrow Down - should NOT change selection if focus is elsewhere
|
||||
// We need to spy on preventDefault to verify it wasn't called, ensuring event propagates
|
||||
const event = new KeyboardEvent('keydown', { key: 'ArrowDown', bubbles: true, cancelable: true });
|
||||
const preventDefaultSpy = vi.spyOn(event, 'preventDefault');
|
||||
|
||||
document.dispatchEvent(event);
|
||||
|
||||
// If the fix is implemented, preventDefault should NOT be called
|
||||
// And the selection should NOT change
|
||||
expect(preventDefaultSpy).not.toHaveBeenCalled();
|
||||
expect(items[1].getAttribute('aria-selected')).toBe('false'); // Should stay on first item
|
||||
|
||||
vi.useRealTimers();
|
||||
});
|
||||
|
||||
it('should cleanup previous event listeners when opening a new dropdown via anchor click', () => {
|
||||
vi.useFakeTimers();
|
||||
const removeEventListenerSpy = vi.spyOn(document, 'removeEventListener');
|
||||
|
||||
const nodes = [{ id: 1, title: 'Node 1', type: 'Type A' }];
|
||||
const anchor = document.createElement('div');
|
||||
document.body.appendChild(anchor);
|
||||
|
||||
// Open first time
|
||||
(uiManager as any).createDropdown(nodes, anchor, 'title');
|
||||
vi.advanceTimersByTime(100);
|
||||
|
||||
// Reset spy to track calls for the next action
|
||||
removeEventListenerSpy.mockClear();
|
||||
|
||||
// Simulate re-opening (which calls hideDropdown then createDropdown)
|
||||
// This simulates clicking the anchor again, or clicking another anchor which triggers hideDropdown first
|
||||
(uiManager as any).showTitleDropdown(anchor);
|
||||
|
||||
// Verify that cleanup for the FIRST dropdown occurred
|
||||
// We expect removeEventListener to be called for the listeners attached by the first call
|
||||
expect(removeEventListenerSpy).toHaveBeenCalledWith('keydown', expect.any(Function));
|
||||
expect(removeEventListenerSpy).toHaveBeenCalledWith('mousedown', expect.any(Function), true);
|
||||
|
||||
vi.useRealTimers();
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user