bug #1502 [Autocomplete] Fix 2 bugs where TomSelect would reset when …

…not necessary (weaverryan)

This PR was merged into the 2.x branch.

Discussion
----------

[Autocomplete] Fix 2 bugs where TomSelect would reset when not necessary

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| Issues        | Fix #1499
| License       | MIT

Hi!

The Autocomplete component "resets" the TomSelect instance under certain situations:

* A) The underlying `<option>` elements have changed
* B) The `<select>` changed its `multiple` attribute

Both of these situations had a bug:

* A) If there was no empty `<option value"">` element, then each time a value was selected, it incorrectly looked like the options had changed, triggering a reset.
* B) If the `<select multiple>` attribute was used without the `multiple="multiple"`, it would incorrectly look like the multiple value was changing, when in fact it was just varying between these 2 valid formats.

In both cases, when combined with LiveComponents, an infinite loop was triggered. That's because, as part of the reset process, when we recreate the TomSelect instance, we "select" the original value. This triggers a `change` event, causing LiveComponents to re-render. On its own , that's fine. But the re-render would trigger one of the bugs above, which would trigger another reset and another re-render.

Cheers!

Commits
-------

7a801fa [Autocomplete] Fix 2 bugs where TomSelect would reset when not necessary

src/Autocomplete/assets/dist/controller.js

@@ -116,6 +116,7 @@ class default_1 extends Controller {
     }
     resetTomSelect() {
         if (this.tomSelect) {
+            this.dispatchEvent('before-reset', { tomSelect: this.tomSelect });
             this.stopMutationObserver();
             const currentHtml = this.element.innerHTML;
             const currentValue = this.tomSelect.getValue();
@@ -143,6 +144,7 @@ class default_1 extends Controller {
                 subtree: true,
                 attributes: true,
                 characterData: true,
+                attributeOldValue: true,
             });
             this.isObserving = true;
         }
@@ -164,7 +166,11 @@ class default_1 extends Controller {
                         break;
                     }
                     if (mutation.target === this.element && mutation.attributeName === 'multiple') {
-                        requireReset = true;
+                        const isNowMultiple = this.element.hasAttribute('multiple');
+                        const wasMultiple = mutation.oldValue === 'multiple';
+                        if (isNowMultiple !== wasMultiple) {
+                            requireReset = true;
+                        }
                         break;
                     }
                     break;
@@ -191,12 +197,14 @@ class default_1 extends Controller {
         });
     }
     areOptionsEquivalent(newOptions) {
-        if (this.originalOptions.length !== newOptions.length) {
+        const filteredOriginalOptions = this.originalOptions.filter((option) => option.value !== '');
+        const filteredNewOptions = newOptions.filter((option) => option.value !== '');
+        if (filteredOriginalOptions.length !== filteredNewOptions.length) {
             return false;
         }
         const normalizeOption = (option) => `${option.value}-${option.text}-${option.group}`;
-        const originalOptionsSet = new Set(this.originalOptions.map(normalizeOption));
-        const newOptionsSet = new Set(newOptions.map(normalizeOption));
+        const originalOptionsSet = new Set(filteredOriginalOptions.map(normalizeOption));
+        const newOptionsSet = new Set(filteredNewOptions.map(normalizeOption));
         return (originalOptionsSet.size === newOptionsSet.size &&
             [...originalOptionsSet].every((option) => newOptionsSet.has(option)));
     }

src/Autocomplete/assets/src/controller.ts

@@ -325,6 +325,7 @@ export default class extends Controller {
 
     private resetTomSelect(): void {
         if (this.tomSelect) {
+            this.dispatchEvent('before-reset', { tomSelect: this.tomSelect });
             this.stopMutationObserver();
 
             // Grab the current HTML then restore it after destroying TomSelect
@@ -358,6 +359,7 @@ export default class extends Controller {
                 subtree: true,
                 attributes: true,
                 characterData: true,
+                attributeOldValue: true,
             });
             this.isObserving = true;
         }
@@ -384,7 +386,11 @@ export default class extends Controller {
                     }
 
                     if (mutation.target === this.element && mutation.attributeName === 'multiple') {
-                        requireReset = true;
+                        const isNowMultiple = this.element.hasAttribute('multiple');
+                        const wasMultiple = mutation.oldValue === 'multiple';
+                        if (isNowMultiple !== wasMultiple) {
+                            requireReset = true;
+                        }
 
                         break;
                     }
@@ -419,14 +425,18 @@ export default class extends Controller {
     }
 
     private areOptionsEquivalent(newOptions: Array<{ value: string; text: string; group: string | null }>): boolean {
-        if (this.originalOptions.length !== newOptions.length) {
+        // remove the empty option, which is added by TomSelect so may be missing from new options
+        const filteredOriginalOptions = this.originalOptions.filter((option) => option.value !== '');
+        const filteredNewOptions = newOptions.filter((option) => option.value !== '');
+
+        if (filteredOriginalOptions.length !== filteredNewOptions.length) {
             return false;
         }
 
         const normalizeOption = (option: { value: string; text: string; group: string | null }) =>
             `${option.value}-${option.text}-${option.group}`;
-        const originalOptionsSet = new Set(this.originalOptions.map(normalizeOption));
-        const newOptionsSet = new Set(newOptions.map(normalizeOption));
+        const originalOptionsSet = new Set(filteredOriginalOptions.map(normalizeOption));
+        const newOptionsSet = new Set(filteredNewOptions.map(normalizeOption));
 
         return (
             originalOptionsSet.size === newOptionsSet.size &&

src/Autocomplete/assets/test/controller.test.ts

@@ -815,4 +815,46 @@ describe('AutocompleteController', () => {
         });
         expect(getSelectedValues()).toEqual(['2', '3']);
     });
+
+    it('does not trigger a reset when the style of "multiple" attribute changes', async () => {
+        const { container } = await startAutocompleteTest(`
+            <select multiple data-testid='main-element' data-controller='autocomplete'>
+                <option value=''>Select dogs</option>
+                <option value='1'>dog1</option>
+                <option value='2'>dog2</option>
+                <option value='3'>dog3</option>
+            </select>
+        `);
+
+        let wasReset = false;
+        container.addEventListener('autocomplete:before-reset', () => {
+            wasReset = true;
+        });
+
+        const selectElement = getByTestId(container, 'main-element') as HTMLSelectElement;
+        selectElement.setAttribute('multiple', 'multiple');
+        // wait for the mutation observe
+        await shortDelay(10);
+        expect(wasReset).toBe(false);
+    });
+
+    it('does not trigger a reset based on the extra, empty select', async () => {
+        const { container, tomSelect } = await startAutocompleteTest(`
+            <select data-testid='main-element' data-controller='autocomplete'>
+                <option value='1'>dog1</option>
+                <option value='2'>dog2</option>
+                <option value='3'>dog3</option>
+            </select>
+        `);
+
+        let wasReset = false;
+        container.addEventListener('autocomplete:before-reset', () => {
+            wasReset = true;
+        });
+
+        tomSelect.addItem('2');
+        // wait for the mutation observe
+        await shortDelay(10);
+        expect(wasReset).toBe(false);
+    });
 });