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

Jump to bottom

support listbox with multiple selection #171

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

support listbox with multiple selection #171

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e9baedf
support listbox with multiple selection
miguelcobain Nov 23, 2022
9a8f325
fix tabindex and aria-selected="false". Correct test unselected optio…
miguelcobain Nov 23, 2022
1e5b261
fix focus tests
miguelcobain Nov 23, 2022
dad2bbc
fix weird registerOptionElement side-effects
miguelcobain Nov 23, 2022
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
5 changes: 3 additions & 2 deletions ember-headlessui/addon/components/combobox/-option.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@
<Tag
role='option'
id={{this.guid}}
tabindex='-1'
tabindex={{unless @disabled '-1'}}
{{this.registerOption}}
{{on 'focus' (fn @setActiveOption this)}}
{{on 'mousedown' this.handleMouseDown}}
{{on 'mouseover' (fn @setActiveOption this)}}
{{on 'mouseout' this.handleMouseOut}}
{{on 'click' this.handleOptionClick}}
aria-selected={{if this.isSelectedOption 'true'}}
aria-selected={{if this.isSelectedOption 'true' 'false'}}
aria-disabled={{if @disabled 'true' 'false'}}
data-is-active={{if this.isActiveOption 'true'}}
disabled={{if @disabled true false}}
...attributes
Expand Down
3 changes: 2 additions & 1 deletion ember-headlessui/addon/components/listbox.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
unregisterOptionsElement=this.unregisterOptionsElement
hasLabelElement=this.labelElement
activeOptionGuid=this.activeOptionGuid
selectedOptionGuid=this.selectedOptionGuid
selectedOptionGuids=this.selectedOptionGuids
setActiveOption=this.setActiveOption
unsetActiveOption=this.unsetActiveOption
setSelectedOption=this.setSelectedOption
Expand All @@ -23,6 +23,7 @@
openListbox=this.openListbox
closeListbox=this.closeListbox
handleClickOutside=this.handleClickOutside
multiple=@multiple
)
Button=(component
'listbox/-button'
Expand Down
60 changes: 45 additions & 15 deletions ember-headlessui/addon/components/listbox.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ import Component from '@glimmer/component';
import { tracked } from '@glimmer/tracking';
import { action } from '@ember/object';
import { guidFor } from '@ember/object/internals';
import { debounce } from '@ember/runloop';
import { debounce, scheduleOnce } from '@ember/runloop';

import { TrackedSet } from 'tracked-maps-and-sets';

const ACTIVATE_NONE = 0;
const ACTIVATE_FIRST = 1;
Expand Down Expand Up @@ -30,7 +32,7 @@ export default class ListboxComponent extends Component {
optionElements = [];
optionValues = {};
search = '';
@tracked selectedOptionIndex;
@tracked selectedOptionIndexes = new TrackedSet();

get activeOptionGuid() {
return this.optionElements[this.activeOptionIndex]?.id;
Expand All @@ -40,8 +42,10 @@ export default class ListboxComponent extends Component {
return !!this.args.disabled;
}

get selectedOptionGuid() {
return this.optionElements[this.selectedOptionIndex]?.id;
get selectedOptionGuids() {
return Array.from(this.selectedOptionIndexes).map(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

way make an array that ends up being a separate array each access?
does this need to be a getter?
can we access the optionElements directly?

(i) => this.optionElements[i]?.id
);
}

get isOpen() {
Expand All @@ -51,7 +55,7 @@ export default class ListboxComponent extends Component {
set isOpen(isOpen) {
if (isOpen) {
this.activeOptionIndex = undefined;
this.selectedOptionIndex = undefined;
this.selectedOptionIndexes.clear();
this.optionElements = [];
this.optionValues = {};
this._isOpen = true;
Expand Down Expand Up @@ -132,7 +136,9 @@ export default class ListboxComponent extends Component {
this.activateBehaviour = ACTIVATE_FIRST;
if (this.isOpen) {
this.setSelectedOption(event.target, event);
this.isOpen = false;
if (!this.args.multiple) {
this.isOpen = false;
}
} else {
this.isOpen = true;
}
Expand Down Expand Up @@ -181,15 +187,27 @@ export default class ListboxComponent extends Component {
optionElement.setAttribute('data-index', this.optionElements.length - 1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any chance you'd be willing to do a PR that removes all this "registerElement" stuff? it's very side-effect heavy and causes double (or more!) renders


if (this.args.value) {
if (this.args.value === optionComponent.args.value) {
this.selectedOptionIndex = this.activeOptionIndex =
this.optionElements.length - 1;
let isSelected;

if (this.args.multiple) {
isSelected = this.args.value.includes(optionComponent.args.value);
} else {
isSelected = this.args.value === optionComponent.args.value;
}

if (isSelected) {
this.selectedOptionIndexes.add(this.optionElements.length - 1);
this.activeOptionIndex = this.optionElements.length - 1;

this.scrollIntoView(optionElement);
}
}

if (!this.selectedOptionIndex) {
scheduleOnce('afterRender', this, this.setDefaultActiveOption);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we avoid hacking runloop usage?

runloop is kinda anti-patterny and causes performance problems

}

setDefaultActiveOption() {
if (this.selectedOptionIndexes.size === 0) {
switch (this.activateBehaviour) {
case ACTIVATE_FIRST:
this.setFirstOptionActive();
Expand All @@ -198,6 +216,8 @@ export default class ListboxComponent extends Component {
this.setLastOptionActive();
break;
}
} else {
this.activeOptionIndex = Math.min(...this.selectedOptionIndexes);
}
}

Expand Down Expand Up @@ -239,13 +259,23 @@ export default class ListboxComponent extends Component {
}

if (!this.optionElements[optionIndex].hasAttribute('disabled')) {
this.selectedOptionIndex = optionIndex;

if (this.args.onChange) {
this.args.onChange(optionValue);
if (this.args.multiple) {
let value = this.args.value ?? [];

if (this.selectedOptionIndexes.has(optionIndex)) {
optionValue = value.filter((i) => i !== optionValue);
this.selectedOptionIndexes.delete(optionIndex);
} else {
optionValue = [...value, optionValue];
this.selectedOptionIndexes.add(optionIndex);
}
} else {
this.selectedOptionIndexes.add(optionIndex);
}

if (e.type === 'click') {
this.args.onChange?.(optionValue);

if (e.type === 'click' && !this.args.multiple) {
this.isOpen = false;
}
} else {
Expand Down
5 changes: 3 additions & 2 deletions ember-headlessui/addon/components/listbox/-option.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,14 @@
<Tag
role='option'
id={{this.guid}}
tabindex='-1'
tabindex={{unless @disabled '-1'}}
{{this.registerOption}}
{{on 'focus' (fn @setActiveOption this)}}
{{on 'mouseover' (fn @setActiveOption this)}}
{{on 'mouseout' @unsetActiveOption}}
{{on 'click' this.handleClick}}
aria-selected={{if this.isSelectedOption 'true'}}
aria-selected={{if this.isSelectedOption 'true' 'false'}}
aria-disabled={{if @disabled 'true' 'false'}}
disabled={{if @disabled true false}}
...attributes
>
Expand Down
2 changes: 1 addition & 1 deletion ember-headlessui/addon/components/listbox/-option.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,6 @@ export default class ListboxOptionComponent extends Component {
}

get isSelectedOption() {
return this.args.selectedOptionGuid == this.guid;
return this.args.selectedOptionGuids.includes(this.guid);
}
}
3 changes: 2 additions & 1 deletion ember-headlessui/addon/components/listbox/-options.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
}}
aria-activedescendant={{@activeOptionGuid}}
aria-orientation='vertical'
aria-multiselectable={{if @multiple 'true'}}
{{this.registerOptions}}
{{on 'keypress' @handleKeyPress}}
{{on 'keydown' @handleKeyDown}}
Expand All @@ -28,7 +29,7 @@
'listbox/-option'
registerOptionElement=@registerOptionElement
activeOptionGuid=@activeOptionGuid
selectedOptionGuid=@selectedOptionGuid
selectedOptionGuids=@selectedOptionGuids
setActiveOption=@setActiveOption
unsetActiveOption=@unsetActiveOption
setSelectedOption=@setSelectedOption
Expand Down
26 changes: 20 additions & 6 deletions test-app/tests/accessibility-assertions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,13 @@ const ListboxState = {
InvisibleUnmounted: 'InvisibleUnmounted',
};

const ListboxMode = {
/** The listbox is in the `single` mode. */
Single: 'Single',
/** The listbox is in the `multiple` mode. */
Multiple: 'Multiple',
};

const ComboboxState = {
Visible: 'Visible',
InvisibleHidden: 'InvisibleHidden',
Expand Down Expand Up @@ -808,7 +815,7 @@ function assertNoActiveListboxOption(listbox = getListbox()) {
function assertNoSelectedListboxOption(items = getListboxOptions()) {
try {
for (let item of items)
Qunit.assert.dom(item).doesNotHaveAttribute('aria-selected');
Qunit.assert.dom(item).hasAttribute('aria-selected', 'false');
} catch (err) {
Error.captureStackTrace(err, assertNoSelectedListboxOption);
throw err;
Expand Down Expand Up @@ -877,7 +884,7 @@ export function assertListboxOption({ tag, attributes, selected }, item) {
Qunit.assert.dom(item).hasAttribute('tabindex', '-1');

// Ensure listbox button has the following attributes
if (!tag && !attributes && !selected) return;
if (!tag && !attributes && selected === undefined) return;

for (let attributeName in attributes) {
Qunit.assert
Expand All @@ -895,7 +902,7 @@ export function assertListboxOption({ tag, attributes, selected }, item) {
return Qunit.assert.dom(item).hasAttribute('aria-selected', 'true');

case false:
return Qunit.assert.dom(item).doesNotHaveAttribute('aria-selected');
return Qunit.assert.dom(item).hasAttribute('aria-selected', 'false');

default:
Qunit.assert.ok();
Expand All @@ -908,7 +915,7 @@ export function assertListboxOption({ tag, attributes, selected }, item) {
}

function assertListbox(
{ state, attributes, textContent },
{ state, attributes, textContent, mode },
listbox = getListbox(),
orientation = 'vertical'
) {
Expand Down Expand Up @@ -944,6 +951,12 @@ function assertListbox(
Qunit.assert.dom(listbox).hasAttribute('aria-orientation', orientation);
Qunit.assert.dom(listbox).hasAttribute('role', 'listbox');

if (mode && mode === ListboxMode.Multiple) {
Qunit.assert
.dom(listbox)
.hasAttribute('aria-multiselectable', 'true');
}

if (textContent) Qunit.assert.dom(listbox).hasText(textContent);

for (let attributeName in attributes) {
Expand Down Expand Up @@ -994,7 +1007,7 @@ function assertComboboxOption({ tag, attributes, selected }, item) {
return Qunit.assert.dom(item).hasAttribute('aria-selected', 'true');

case false:
return Qunit.assert.dom(item).doesNotHaveAttribute('aria-selected');
return Qunit.assert.dom(item).hasAttribute('aria-selected', 'false');

default:
Qunit.assert.ok();
Expand All @@ -1009,7 +1022,7 @@ function assertComboboxOption({ tag, attributes, selected }, item) {
function assertNoSelectedComboboxOption(items = getComboboxOptions()) {
try {
for (let item of items)
Qunit.assert.dom(item).doesNotHaveAttribute('aria-selected');
Qunit.assert.dom(item).hasAttribute('aria-selected', 'false');
} catch (err) {
Error.captureStackTrace(err, assertNoSelectedComboboxOption);
throw err;
Expand Down Expand Up @@ -1363,5 +1376,6 @@ export {
getListboxLabel,
getListboxOptions,
getRadioGroupOptions,
ListboxMode,
ListboxState,
};
Loading