Jlb/fix combobox bugs (#3854)

* Account for null values when comparing "by" property

* More fixes

* Fix aria-selected attribute

* Fix home/end/page up/page down + shift keydown handling

* Add some additional tests

* Fix more aria-selected tests

* wip

---------

Co-authored-by: Jason Beggs <jason@roasted.dev>

packages/ui/src/combobox.js

@@ -246,7 +246,15 @@ function handleRoot(el, Alpine) {
 
                     if (typeof by === 'string') {
                         let property = by
-                        by = (a, b) => a[property] === b[property]
+                        by = (a, b) => {
+                            // Handle null values
+                            if ((! a || typeof a !== 'object') || (! b || typeof b !== 'object')) {
+                                return Alpine.raw(a) === Alpine.raw(b)
+                            }
+
+
+                            return a[property] === b[property];
+                        }
                     }
 
                     return by(a, b)
@@ -432,9 +440,9 @@ function handleOption(el, Alpine) {
 
         // Only the active element should have aria-selected="true"...
         'x-effect'() {
-            this.$comboboxOption.isActive
+            this.$comboboxOption.isSelected
                 ? el.setAttribute('aria-selected', true)
-                : el.removeAttribute('aria-selected')
+                : el.setAttribute('aria-selected', false)
         },
 
         ':aria-disabled'() { return this.$comboboxOption.isDisabled },

packages/ui/src/list-context.js

@@ -292,6 +292,8 @@ export function generateContext(Alpine, multiple, orientation, activateSelectedO
                     break;
                 case 'Home':
                 case 'PageUp':
+                    if (e.key == 'Home' && e.shiftKey) return;
+
                     e.preventDefault(); e.stopPropagation()
                     setIsTyping(false)
                     this.reorderKeys(); hasActive = this.hasActive()
@@ -300,6 +302,8 @@ export function generateContext(Alpine, multiple, orientation, activateSelectedO
 
                 case 'End':
                 case 'PageDown':
+                    if (e.key == 'End' && e.shiftKey) return;
+
                     e.preventDefault(); e.stopPropagation()
                     setIsTyping(false)
                     this.reorderKeys(); hasActive = this.hasActive()

tests/cypress/integration/plugins/ui/combobox.spec.js

@@ -708,6 +708,155 @@ test('"multiple" and "name" props together',
     },
 );
 
+test('"by" prop with string value',
+    [html`
+        <div
+            x-data="{
+                people: [
+                    { id: 1, name: 'Wade Cooper' },
+                    { id: 2, name: 'Arlene Mccoy' },
+                    { id: 3, name: 'Devon Webb' },
+                    { id: 4, name: 'Tom Cook' },
+                    { id: 5, name: 'Tanya Fox', disabled: true },
+                    { id: 6, name: 'Hellen Schmidt' },
+                    { id: 7, name: 'Caroline Schultz' },
+                    { id: 8, name: 'Mason Heaney' },
+                    { id: 9, name: 'Claudie Smitham' },
+                    { id: 10, name: 'Emil Schaefer' },
+                ]
+            }"
+            x-combobox
+            by="id"
+        >
+            <label x-combobox:label>Assigned to</label>
+
+            <input x-combobox:input :display-value="(person) => person" type="text">
+            <button x-combobox:button x-text="$combobox.value ? $combobox.value : 'Select People'"></button>
+
+            <ul x-combobox:options>
+                <template x-for="person in people" :key="person.id">
+                    <li
+                        :option="person.id"
+                        x-combobox:option
+                        :value="person.id"
+                        :disabled="person.disabled"
+                        :class="{
+                            'selected': $comboboxOption.isSelected,
+                            'active': $comboboxOption.isActive,
+                        }"
+                    >
+                        <span x-text="person.name"></span>
+                    </li>
+                </template>
+            </ul>
+        </div>
+    `],
+    ({ get }) => {
+        get('ul').should(notBeVisible())
+        get('button').click()
+        get('ul').should(beVisible())
+        get('button').click()
+        get('ul').should(notBeVisible())
+        get('button').click()
+        get('[option="2"]').click()
+        get('ul').should(notBeVisible())
+        get('input').should(haveValue('2'))
+        get('button').should(haveText('2'))
+        get('button').click()
+        get('ul').should(contain('Wade Cooper'))
+            .should(contain('Arlene Mccoy'))
+            .should(contain('Devon Webb'))
+        get('[option="3"]').click()
+        get('ul').should(notBeVisible())
+        get('input').should(haveValue('3'))
+        get('button').should(haveText('3'))
+        get('button').click()
+        get('ul').should(contain('Wade Cooper'))
+            .should(contain('Arlene Mccoy'))
+            .should(contain('Devon Webb'))
+        get('[option="1"]').click()
+        get('ul').should(notBeVisible())
+        get('input').should(haveValue('1'))
+        get('button').should(haveText('1'))
+    },
+);
+
+test('"by" prop with string value and "nullable"',
+    [html`
+        <div
+            x-data="{
+                people: [
+                    { id: 1, name: 'Wade Cooper' },
+                    { id: 2, name: 'Arlene Mccoy' },
+                    { id: 3, name: 'Devon Webb' },
+                    { id: 4, name: 'Tom Cook' },
+                    { id: 5, name: 'Tanya Fox', disabled: true },
+                    { id: 6, name: 'Hellen Schmidt' },
+                    { id: 7, name: 'Caroline Schultz' },
+                    { id: 8, name: 'Mason Heaney' },
+                    { id: 9, name: 'Claudie Smitham' },
+                    { id: 10, name: 'Emil Schaefer' },
+                ]
+            }"
+            x-combobox
+            by="id"
+            default-value="5"
+            nullable
+        >
+            <label x-combobox:label>Assigned to</label>
+
+            <input x-combobox:input :display-value="(person) => person?.name" type="text">
+            <button x-combobox:button x-text="$combobox.value ? $combobox.value.name : 'Select People'"></button>
+
+            <ul x-combobox:options>
+                <template x-for="person in people" :key="person.id">
+                    <li
+                        :option="person.id"
+                        x-combobox:option
+                        :value="person"
+                        :disabled="person.disabled"
+                        :class="{
+                            'selected': $comboboxOption.isSelected,
+                            'active': $comboboxOption.isActive,
+                        }"
+                    >
+                        <span x-text="person.name"></span>
+                    </li>
+                </template>
+            </ul>
+        </div>
+    `],
+    ({ get }) => {
+        get('ul').should(notBeVisible())
+        get('button').click()
+        get('ul').should(beVisible())
+        get('button').click()
+        get('ul').should(notBeVisible())
+        get('button').click()
+        get('[option="2"]').click()
+        get('ul').should(notBeVisible())
+        get('input').should(haveValue('Arlene Mccoy'))
+        get('button').should(haveText('Arlene Mccoy'))
+        get('button').click()
+        get('ul').should(contain('Wade Cooper'))
+            .should(contain('Arlene Mccoy'))
+            .should(contain('Devon Webb'))
+        get('[option="3"]').click()
+        get('ul').should(notBeVisible())
+        get('input').should(haveValue('Devon Webb'))
+        get('button').should(haveText('Devon Webb'))
+        get('button').click()
+        get('ul').should(contain('Wade Cooper'))
+            .should(contain('Arlene Mccoy'))
+            .should(contain('Devon Webb'))
+        get('[option="1"]').click()
+        get('ul').should(notBeVisible())
+        get('input').should(haveValue('Wade Cooper'))
+        get('button').should(haveText('Wade Cooper'))
+    },
+);
+
+
 test('keyboard controls',
     [html`
         <div
@@ -913,13 +1062,13 @@ test('has accessibility attributes',
             .should(haveAttribute('role', 'option'))
             .should(haveAttribute('id', 'alpine-combobox-option-1'))
             .should(haveAttribute('tabindex', '-1'))
-            .should(haveAttribute('aria-selected', 'true'))
+            .should(haveAttribute('aria-selected', 'false'))
 
         get('[option="2"]')
             .should(haveAttribute('role', 'option'))
             .should(haveAttribute('id', 'alpine-combobox-option-2'))
             .should(haveAttribute('tabindex', '-1'))
-            .should(notHaveAttribute('aria-selected'))
+            .should(haveAttribute('aria-selected', 'false'))
 
         get('input')
             .should(haveAttribute('role', 'combobox'))
@@ -931,9 +1080,13 @@ test('has accessibility attributes',
             .should(haveAttribute('aria-activedescendant', 'alpine-combobox-option-1'))
             .type('{downarrow}')
             .should(haveAttribute('aria-activedescendant', 'alpine-combobox-option-2'))
+            .type('{enter}')
 
         get('[option="2"]')
             .should(haveAttribute('aria-selected', 'true'))
+
+        get('[option="1"]')
+            .should(haveAttribute('aria-selected', 'false'))
     },
 )
 
@@ -1271,6 +1424,7 @@ test('active element logic when opening a combobox',
                                     :option="person.id"
                                     :value="person"
                                     :disabled="person.disabled"
+                                    :class="$comboboxOption.isActive ? 'active' : ''"
                                     x-text="person.name"
                                 >
                                 </li>
@@ -1287,19 +1441,120 @@ test('active element logic when opening a combobox',
         get('button').click()
         // First option is selected on opening if no preselection
         get('ul').should(beVisible())
-        get('[option="1"]').should(haveAttribute('aria-selected', 'true'))
+        get('[option="1"]').should(haveAttribute('aria-selected', 'false'))
+        get('[option="1"]').should(haveClasses(['active']))
         // First match is selected while typing
-        get('[option="4"]').should(notHaveAttribute('aria-selected'))
+        get('[option="4"]').should(haveAttribute('aria-selected', 'false'))
+        get('[option="4"]').should(notHaveClasses(['active']))
         get('input').type('T')
         get('input').trigger('change')
-        get('[option="4"]').should(haveAttribute('aria-selected', 'true'))
+        get('[option="4"]').should(haveAttribute('aria-selected', 'false'))
+        get('[option="4"]').should(haveClasses(['active']))
         // Reset state and select option 3
         get('button').click()
         get('button').click()
         get('[option="3"]').click()
         // Previous selection is selected
         get('button').click()
-        get('[option="4"]').should(notHaveAttribute('aria-selected'))
+        get('[option="4"]').should(haveAttribute('aria-selected', 'false'))
         get('[option="3"]').should(haveAttribute('aria-selected', 'true'))
     }
 )
+
+test('can remove an option without other options getting removed',
+    [html`<div
+        x-data="{
+            query: '',
+            selected: [],
+            frameworks: [
+                {
+                    id: 1,
+                    name: 'Laravel',
+                    disabled: false,
+                },
+                {
+                    id: 2,
+                    name: 'Ruby on Rails',
+                    disabled: false,
+                },
+                {
+                    id: 3,
+                    name: 'Django',
+                    disabled: false,
+                },
+            ],
+            get filteredFrameworks() {
+                return this.query === ''
+                    ? this.frameworks
+                    : this.frameworks.filter((framework) => {
+                        return framework.name.toLowerCase().includes(this.query.toLowerCase())
+                    })
+            },
+            remove(framework) {
+                this.selected = this.selected.filter((i) => i !== framework)
+            }
+        }"
+    >
+        <div x-combobox x-model="selected" by="id" multiple>
+            <div x-show="selected.length">
+                <template x-for="selectedFramework in selected" :key="selectedFramework.id">
+                    <button x-on:click.prevent="remove(selectedFramework)" :remove-option="selectedFramework.id">
+                        <span x-text="selectedFramework.name"></span>
+                    </button>
+                </template>
+            </div>
+
+            <div>
+                <div>
+                    <input
+                        x-combobox:input
+                        @change="query = $event.target.value;"
+                        placeholder="Search..."
+                    />
+                    <button x-combobox:button>
+                        Show options
+                    </button>
+                </div>
+
+                <div x-combobox:options x-cloak x-transition.out.opacity>
+                    <ul>
+                        <template
+                            x-for="framework in filteredFrameworks"
+                            :key="framework.id"
+                            hidden
+                        >
+                            <li
+                                x-combobox:option
+                                :option="framework.id"
+                                :value="framework"
+                                :disabled="framework.disabled"
+                            >
+                                <span x-text="framework.name"></span>
+
+                                <span x-show="$comboboxOption.isSelected" :check="framework.id">&check;</span>
+                            </li>
+                        </template>
+                    </ul>
+
+                    <p x-show="filteredFrameworks.length == 0">No frameworks match your query.</p>
+                </div>
+            </div>
+        </div>
+    </div>
+    `],
+    ({ get }) => {
+        get('input').type('a').trigger('input')
+        cy.wait(100)
+        get('[option="1"]').click()
+        get('[option="2"]').click()
+        get('[option="3"]').click()
+        get('[remove-option="3"]').click()
+        get('[option="1"]').should(haveAttribute('aria-selected', 'true'))
+        get('[option="2"]').should(haveAttribute('aria-selected', 'true'))
+        get('[option="3"]').should(haveAttribute('aria-selected', 'false'))
+        get('input').type('a').trigger('input')
+        get('[check="1"]').should(beVisible())
+        get('[check="2"]').should(beVisible())
+        get('[check="3"]').should(notBeVisible())
+    },
+);