-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Cdk listbox: multipleselect support and active descendant #19929
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.
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
Changes from 13 commits
1f14151
378ca05
d5a4b19
e11ce60
bf9dadb
a2e8339
f248a82
f1b291b
057c921
5e67e9a
637f1b5
c7fb815
0678a25
e27aaa9
4efbd05
571d5eb
15fdf79
c417d73
18c9faa
59205ef
c5ac3d5
6b51456
5790b14
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -308,8 +308,246 @@ describe('CdkOption', () => { | |||||
expect(listboxInstance._listKeyManager.activeItem).toEqual(optionInstances[2]); | ||||||
expect(listboxInstance._listKeyManager.activeItemIndex).toBe(2); | ||||||
}); | ||||||
|
||||||
it('should select all options using the select all method', () => { | ||||||
let selectedOptions = optionInstances.filter(option => option.selected); | ||||||
|
||||||
expect(selectedOptions.length).toBe(0); | ||||||
expect(optionElements[0].hasAttribute('aria-selected')).toBeFalse(); | ||||||
expect(optionInstances[0].selected).toBeFalse(); | ||||||
expect(fixture.componentInstance.changedOption).toBeUndefined(); | ||||||
|
||||||
listboxInstance.setAllSelected(true); | ||||||
fixture.detectChanges(); | ||||||
|
||||||
selectedOptions = optionInstances.filter(option => option.selected); | ||||||
expect(selectedOptions.length).toBe(4); | ||||||
|
||||||
for (const option of optionElements) { | ||||||
expect(option.getAttribute('aria-selected')).toBe('true'); | ||||||
} | ||||||
|
||||||
expect(fixture.componentInstance.changedOption).toBeDefined(); | ||||||
expect(fixture.componentInstance.changedOption.id).toBe(optionInstances[3].id); | ||||||
}); | ||||||
|
||||||
it('should update selected option on click event', () => { | ||||||
let selectedOptions = optionInstances.filter(option => option.selected); | ||||||
|
||||||
expect(selectedOptions.length).toBe(0); | ||||||
expect(optionElements[0].hasAttribute('aria-selected')).toBeFalse(); | ||||||
expect(optionInstances[0].selected).toBeFalse(); | ||||||
expect(fixture.componentInstance.changedOption).toBeUndefined(); | ||||||
|
||||||
dispatchMouseEvent(optionElements[0], 'click'); | ||||||
fixture.detectChanges(); | ||||||
|
||||||
selectedOptions = optionInstances.filter(option => option.selected); | ||||||
expect(selectedOptions.length).toBe(1); | ||||||
expect(optionElements[0].getAttribute('aria-selected')).toBe('true'); | ||||||
expect(optionInstances[0].selected).toBeTrue(); | ||||||
expect(fixture.componentInstance.changedOption).toBeDefined(); | ||||||
expect(fixture.componentInstance.changedOption.id).toBe(optionInstances[0].id); | ||||||
}); | ||||||
}); | ||||||
|
||||||
describe('multiselectable tests', () => { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Prefer naming
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, noted. |
||||||
let fixture: ComponentFixture<ListboxMultiselect>; | ||||||
|
||||||
let testComponent: ListboxMultiselect; | ||||||
|
||||||
let options: DebugElement[]; | ||||||
let optionInstances: CdkOption[]; | ||||||
let optionElements: HTMLElement[]; | ||||||
|
||||||
beforeEach(async(() => { | ||||||
TestBed.configureTestingModule({ | ||||||
imports: [CdkListboxModule], | ||||||
declarations: [ListboxMultiselect], | ||||||
}).compileComponents(); | ||||||
})); | ||||||
|
||||||
beforeEach(async(() => { | ||||||
fixture = TestBed.createComponent(ListboxMultiselect); | ||||||
fixture.detectChanges(); | ||||||
|
||||||
testComponent = fixture.debugElement.componentInstance; | ||||||
|
||||||
options = fixture.debugElement.queryAll(By.directive(CdkOption)); | ||||||
optionInstances = options.map(o => o.injector.get<CdkOption>(CdkOption)); | ||||||
optionElements = options.map(o => o.nativeElement); | ||||||
})); | ||||||
|
||||||
it('should deselect previously selected when multiple is false', () => { | ||||||
let selectedOptions = optionInstances.filter(option => option.selected); | ||||||
|
||||||
expect(selectedOptions.length).toBe(0); | ||||||
expect(optionElements[0].hasAttribute('aria-selected')).toBeFalse(); | ||||||
expect(optionInstances[0].selected).toBeFalse(); | ||||||
expect(fixture.componentInstance.changedOption).toBeUndefined(); | ||||||
|
||||||
dispatchMouseEvent(optionElements[0], 'click'); | ||||||
fixture.detectChanges(); | ||||||
|
||||||
selectedOptions = optionInstances.filter(option => option.selected); | ||||||
expect(selectedOptions.length).toBe(1); | ||||||
expect(optionElements[0].getAttribute('aria-selected')).toBe('true'); | ||||||
expect(optionInstances[0].selected).toBeTrue(); | ||||||
expect(fixture.componentInstance.changedOption.id).toBe(optionInstances[0].id); | ||||||
|
||||||
dispatchMouseEvent(optionElements[2], 'click'); | ||||||
fixture.detectChanges(); | ||||||
|
||||||
selectedOptions = optionInstances.filter(option => option.selected); | ||||||
expect(selectedOptions.length).toBe(1); | ||||||
expect(optionElements[0].hasAttribute('aria-selected')).toBeFalse(); | ||||||
expect(optionInstances[0].selected).toBeFalse(); | ||||||
expect(optionElements[2].getAttribute('aria-selected')).toBe('true'); | ||||||
expect(optionInstances[2].selected).toBeTrue(); | ||||||
expect(fixture.componentInstance.changedOption.id).toBe(optionInstances[2].id); | ||||||
}); | ||||||
|
||||||
it('should allow multiple selection when multiple is false', () => { | ||||||
let selectedOptions = optionInstances.filter(option => option.selected); | ||||||
testComponent.isMultiselectable = true; | ||||||
|
||||||
expect(selectedOptions.length).toBe(0); | ||||||
expect(fixture.componentInstance.changedOption).toBeUndefined(); | ||||||
|
||||||
dispatchMouseEvent(optionElements[0], 'click'); | ||||||
fixture.detectChanges(); | ||||||
|
||||||
selectedOptions = optionInstances.filter(option => option.selected); | ||||||
expect(selectedOptions.length).toBe(1); | ||||||
expect(optionElements[0].getAttribute('aria-selected')).toBe('true'); | ||||||
expect(optionInstances[0].selected).toBeTrue(); | ||||||
expect(fixture.componentInstance.changedOption.id).toBe(optionInstances[0].id); | ||||||
|
||||||
dispatchMouseEvent(optionElements[2], 'click'); | ||||||
fixture.detectChanges(); | ||||||
|
||||||
selectedOptions = optionInstances.filter(option => option.selected); | ||||||
expect(selectedOptions.length).toBe(2); | ||||||
expect(optionElements[0].getAttribute('aria-selected')).toBe('true'); | ||||||
expect(optionInstances[0].selected).toBeTrue(); | ||||||
expect(optionElements[2].getAttribute('aria-selected')).toBe('true'); | ||||||
expect(optionInstances[2].selected).toBeTrue(); | ||||||
expect(fixture.componentInstance.changedOption.id).toBe(optionInstances[2].id); | ||||||
}); | ||||||
|
||||||
it('should deselect all options when multiple switches to false', () => { | ||||||
let selectedOptions = optionInstances.filter(option => option.selected); | ||||||
testComponent.isMultiselectable = true; | ||||||
|
||||||
expect(selectedOptions.length).toBe(0); | ||||||
expect(fixture.componentInstance.changedOption).toBeUndefined(); | ||||||
|
||||||
dispatchMouseEvent(optionElements[0], 'click'); | ||||||
fixture.detectChanges(); | ||||||
|
||||||
selectedOptions = optionInstances.filter(option => option.selected); | ||||||
expect(selectedOptions.length).toBe(1); | ||||||
expect(optionElements[0].getAttribute('aria-selected')).toBe('true'); | ||||||
expect(optionInstances[0].selected).toBeTrue(); | ||||||
expect(fixture.componentInstance.changedOption.id).toBe(optionInstances[0].id); | ||||||
|
||||||
testComponent.isMultiselectable = false; | ||||||
fixture.detectChanges(); | ||||||
|
||||||
selectedOptions = optionInstances.filter(option => option.selected); | ||||||
expect(selectedOptions.length).toBe(0); | ||||||
expect(optionElements[0].hasAttribute('aria-selected')).toBeFalse(); | ||||||
expect(optionInstances[0].selected).toBeFalse(); | ||||||
expect(fixture.componentInstance.changedOption.id).toBe(optionInstances[0].id); | ||||||
}); | ||||||
}); | ||||||
|
||||||
describe('aria active descendant tests', () => { | ||||||
let fixture: ComponentFixture<ListboxActiveDescendant>; | ||||||
|
||||||
let testComponent: ListboxActiveDescendant; | ||||||
|
||||||
let listbox: DebugElement; | ||||||
let listboxInstance: CdkListbox; | ||||||
let listboxElement: HTMLElement; | ||||||
|
||||||
let options: DebugElement[]; | ||||||
let optionInstances: CdkOption[]; | ||||||
let optionElements: HTMLElement[]; | ||||||
|
||||||
beforeEach(async(() => { | ||||||
TestBed.configureTestingModule({ | ||||||
imports: [CdkListboxModule], | ||||||
declarations: [ListboxActiveDescendant], | ||||||
}).compileComponents(); | ||||||
})); | ||||||
|
||||||
beforeEach(async(() => { | ||||||
fixture = TestBed.createComponent(ListboxActiveDescendant); | ||||||
fixture.detectChanges(); | ||||||
|
||||||
testComponent = fixture.debugElement.componentInstance; | ||||||
|
||||||
listbox = fixture.debugElement.query(By.directive(CdkListbox)); | ||||||
listboxInstance = listbox.injector.get<CdkListbox>(CdkListbox); | ||||||
listboxElement = listbox.nativeElement; | ||||||
|
||||||
options = fixture.debugElement.queryAll(By.directive(CdkOption)); | ||||||
optionInstances = options.map(o => o.injector.get<CdkOption>(CdkOption)); | ||||||
optionElements = options.map(o => o.nativeElement); | ||||||
})); | ||||||
|
||||||
it('should update aria active descendant', () => { | ||||||
expect(listboxElement.hasAttribute('aria-activedescendant')).toBeFalse(); | ||||||
|
||||||
listboxInstance.setActiveOption(optionInstances[0]); | ||||||
fixture.detectChanges(); | ||||||
|
||||||
expect(listboxElement.hasAttribute('aria-activedescendant')).toBeTrue(); | ||||||
expect(listboxElement.getAttribute('aria-activedescendant')).toBe(optionElements[0].id); | ||||||
|
||||||
listboxInstance.setActiveOption(optionInstances[2]); | ||||||
fixture.detectChanges(); | ||||||
|
||||||
expect(listboxElement.hasAttribute('aria-activedescendant')).toBeTrue(); | ||||||
expect(listboxElement.getAttribute('aria-activedescendant')).toBe(optionElements[2].id); | ||||||
}); | ||||||
|
||||||
it('should update aria active descendant via arrow keys', () => { | ||||||
expect(listboxElement.hasAttribute('aria-activedescendant')).toBeFalse(); | ||||||
|
||||||
dispatchKeyboardEvent(listboxElement, 'keydown', DOWN_ARROW); | ||||||
fixture.detectChanges(); | ||||||
|
||||||
expect(listboxElement.hasAttribute('aria-activedescendant')).toBeTrue(); | ||||||
expect(listboxElement.getAttribute('aria-activedescendant')).toBe(optionElements[0].id); | ||||||
|
||||||
dispatchKeyboardEvent(listboxElement, 'keydown', DOWN_ARROW); | ||||||
fixture.detectChanges(); | ||||||
|
||||||
expect(listboxElement.hasAttribute('aria-activedescendant')).toBeTrue(); | ||||||
expect(listboxElement.getAttribute('aria-activedescendant')).toBe(optionElements[1].id); | ||||||
}); | ||||||
|
||||||
it('should place focus on options and not set active descendant', () => { | ||||||
testComponent.isActiveDescendant = false; | ||||||
fixture.detectChanges(); | ||||||
|
||||||
expect(listboxElement.hasAttribute('aria-activedescendant')).toBeFalse(); | ||||||
|
||||||
dispatchKeyboardEvent(listboxElement, 'keydown', DOWN_ARROW); | ||||||
fixture.detectChanges(); | ||||||
|
||||||
expect(listboxElement.hasAttribute('aria-activedescendant')).toBeFalse(); | ||||||
expect(document.activeElement).toEqual(optionElements[0]); | ||||||
dispatchKeyboardEvent(listboxElement, 'keydown', DOWN_ARROW); | ||||||
fixture.detectChanges(); | ||||||
|
||||||
expect(listboxElement.hasAttribute('aria-activedescendant')).toBeFalse(); | ||||||
expect(document.activeElement).toEqual(optionElements[1]); | ||||||
|
||||||
}); | ||||||
}); | ||||||
}); | ||||||
|
||||||
@Component({ | ||||||
|
@@ -337,3 +575,48 @@ class ListboxWithOptions { | |||||
this.changedOption = event.option; | ||||||
} | ||||||
} | ||||||
|
||||||
@Component({ | ||||||
template: ` | ||||||
<div cdkListbox | ||||||
[multiple]="isMultiselectable" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's an extra space here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually I think the |
||||||
(selectionChange)="onSelectionChange($event)"> | ||||||
<div cdkOption>Purple</div> | ||||||
<div cdkOption>Solar</div> | ||||||
<div cdkOption>Arc</div> | ||||||
<div cdkOption>Stasis</div> | ||||||
</div>` | ||||||
}) | ||||||
class ListboxMultiselect { | ||||||
changedOption: CdkOption; | ||||||
isMultiselectable: boolean = false; | ||||||
|
||||||
onSelectionChange(event: ListboxSelectionChangeEvent) { | ||||||
this.changedOption = event.option; | ||||||
} | ||||||
} | ||||||
|
||||||
@Component({ | ||||||
template: ` | ||||||
<div cdkListbox | ||||||
[useActiveDescendant]="isActiveDescendant" | ||||||
> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure which is the standard, but looks like this closing tag is on the next line, but in the previous template it's on the previous line There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I think it should be on the previous line. |
||||||
<div cdkOption>Purple</div> | ||||||
<div cdkOption>Solar</div> | ||||||
<div cdkOption>Arc</div> | ||||||
<div cdkOption>Stasis</div> | ||||||
</div>` | ||||||
}) | ||||||
class ListboxActiveDescendant { | ||||||
changedOption: CdkOption; | ||||||
isActiveDescendant: boolean = true; | ||||||
focusedOption: string; | ||||||
|
||||||
onSelectionChange(event: ListboxSelectionChangeEvent) { | ||||||
this.changedOption = event.option; | ||||||
} | ||||||
|
||||||
onFocus(option: string) { | ||||||
this.focusedOption = option; | ||||||
} | ||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I typically avoid toBeDefined since it returns true for "null", which can be a little confusing. ".not.toBe(undefined);" is less mistakable. May not matter for this usage, up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I am checking
changedOption.id
directly after, do you think it is even necessary to check thatchangedOption
is defined?