Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jlb/fix combobox bugs #3854

Merged
merged 8 commits into from
Nov 6, 2023
Merged

Jlb/fix combobox bugs #3854

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
aa9c207
Account for null values when comparing "by" property
jasonlbeggs Sep 21, 2023
e179f7b
More fixes
jasonlbeggs Sep 21, 2023
11e3945
Fix aria-selected attribute
jasonlbeggs Nov 5, 2023
2b4399f
Fix home/end/page up/page down + shift keydown handling
jasonlbeggs Nov 5, 2023
a5e7192
Add some additional tests
jasonlbeggs Nov 5, 2023
46fd4c6
Merge branch 'main' into jlb/fix-combobox-bugs
jasonlbeggs Nov 5, 2023
63a5ffd
Fix more aria-selected tests
jasonlbeggs Nov 5, 2023
5d39a07
wip
jasonlbeggs Nov 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions packages/ui/src/combobox.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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 },
Expand Down
4 changes: 4 additions & 0 deletions packages/ui/src/list-context.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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()
Expand Down
267 changes: 261 additions & 6 deletions tests/cypress/integration/plugins/ui/combobox.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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'))
Expand All @@ -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'))
},
)

Expand Down Expand Up @@ -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>
Expand All @@ -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())
},
);