Skip to content

Commit 416fa82

Browse files
authored
fix(aria/combobox): highlighting edge cases (#32136)
* refactor(aria/combobox): move some esc logic * fix(aria/combobox): highlighting edge cases * fixup! fix(aria/combobox): highlighting edge cases * fixup! fix(aria/combobox): highlighting edge cases * fix(aria/combobox): escape behavior * fixup! fix(aria/combobox): highlighting edge cases * fixup! fix(aria/combobox): highlighting edge cases * fix(aria/combobox): calc highlight state on open
1 parent 732a0d7 commit 416fa82

File tree

3 files changed

+108
-30
lines changed

3 files changed

+108
-30
lines changed

src/aria/combobox/combobox.spec.ts

Lines changed: 62 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -242,17 +242,19 @@ describe('Combobox', () => {
242242
expect(inputElement.getAttribute('aria-expanded')).toBe('true');
243243
});
244244

245-
it('should clear the completion string and not close on escape when a completion is present', () => {
245+
it('should close then clear the completion string', () => {
246246
fixture.componentInstance.filterMode.set('highlight');
247247
focus();
248-
input('A');
248+
input('Ala');
249249
expect(inputElement.value).toBe('Alabama');
250250
expect(inputElement.getAttribute('aria-expanded')).toBe('true');
251251
escape();
252-
expect(inputElement.value).toBe('A');
253-
expect(inputElement.getAttribute('aria-expanded')).toBe('true');
252+
expect(inputElement.value).toBe('Alabama');
253+
expect(inputElement.selectionEnd).toBe(7);
254+
expect(inputElement.selectionStart).toBe(3);
255+
expect(inputElement.getAttribute('aria-expanded')).toBe('false'); // close
254256
escape();
255-
expect(inputElement.value).toBe('A');
257+
expect(inputElement.value).toBe(''); // clear input
256258
expect(inputElement.getAttribute('aria-expanded')).toBe('false');
257259
});
258260

@@ -424,6 +426,28 @@ describe('Combobox', () => {
424426
expect(inputElement.selectionEnd).toBe(7);
425427
}));
426428

429+
it('should not insert a completion string on backspace', fakeAsync(() => {
430+
focus();
431+
input('New');
432+
tick();
433+
434+
expect(inputElement.value).toBe('New Hampshire');
435+
expect(inputElement.selectionStart).toBe(3);
436+
expect(inputElement.selectionEnd).toBe(13);
437+
}));
438+
439+
it('should insert a completion string even if the items are not changed', fakeAsync(() => {
440+
focus();
441+
input('New');
442+
tick();
443+
444+
input('New ');
445+
tick();
446+
expect(inputElement.value).toBe('New Hampshire');
447+
expect(inputElement.selectionStart).toBe(4);
448+
expect(inputElement.selectionEnd).toBe(13);
449+
}));
450+
427451
it('should commit the selected option on focusout', () => {
428452
focus();
429453
input('Cali');
@@ -438,15 +462,15 @@ describe('Combobox', () => {
438462
// TODO(wagnermaciel): Add unit tests for disabled options.
439463

440464
describe('Filtering', () => {
441-
beforeEach(() => setupCombobox());
442-
443465
it('should lazily render options', () => {
466+
setupCombobox();
444467
expect(getOptions().length).toBe(0);
445468
focus();
446469
expect(getOptions().length).toBe(50);
447470
});
448471

449472
it('should filter the options based on the input value', () => {
473+
setupCombobox();
450474
focus();
451475
input('New');
452476

@@ -459,20 +483,47 @@ describe('Combobox', () => {
459483
});
460484

461485
it('should show no options if nothing matches', () => {
486+
setupCombobox();
462487
focus();
463488
input('xyz');
464489
const options = getOptions();
465490
expect(options.length).toBe(0);
466491
});
467492

468493
it('should show all options when the input is cleared', () => {
494+
setupCombobox();
469495
focus();
470496
input('Alabama');
471497
expect(getOptions().length).toBe(1);
472498

473499
input('');
474500
expect(getOptions().length).toBe(50);
475501
});
502+
503+
it('should determine the highlighted state on open', () => {
504+
setupCombobox({filterMode: 'highlight'});
505+
focus();
506+
input('N');
507+
expect(inputElement.value).toBe('Nebraska');
508+
expect(inputElement.selectionEnd).toBe(8);
509+
expect(inputElement.selectionStart).toBe(1);
510+
expect(getOptions().length).toBe(8);
511+
512+
escape(); // close
513+
inputElement.selectionStart = 2; // Change highlighting
514+
down(); // open
515+
516+
expect(inputElement.value).toBe('Nebraska');
517+
expect(inputElement.selectionEnd).toBe(8);
518+
expect(inputElement.selectionStart).toBe(2);
519+
expect(getOptions().length).toBe(6);
520+
521+
escape(); // close
522+
inputElement.selectionStart = 3; // Change highlighting
523+
down(); // open
524+
525+
expect(getOptions().length).toBe(1);
526+
});
476527
});
477528

478529
// describe('with programmatic value changes', () => {
@@ -907,17 +958,17 @@ describe('Combobox', () => {
907958
expect(inputElement.getAttribute('aria-expanded')).toBe('true');
908959
});
909960

910-
it('should clear the completion string and not close on escape when a completion is present', () => {
961+
it('should close then clear the completion string', () => {
911962
fixture.componentInstance.filterMode.set('highlight');
912963
focus();
913964
input('Mar');
914965
expect(inputElement.value).toBe('March');
915966
expect(inputElement.getAttribute('aria-expanded')).toBe('true');
916967
escape();
917-
expect(inputElement.value).toBe('Mar');
918-
expect(inputElement.getAttribute('aria-expanded')).toBe('true');
968+
expect(inputElement.value).toBe('March');
969+
expect(inputElement.getAttribute('aria-expanded')).toBe('false'); // close
919970
escape();
920-
expect(inputElement.value).toBe('Mar');
971+
expect(inputElement.value).toBe(''); // clear input
921972
expect(inputElement.getAttribute('aria-expanded')).toBe('false');
922973
});
923974

src/aria/combobox/combobox.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,7 @@ export class ComboboxInput {
139139

140140
/** Focuses & selects the first item in the combobox if the user changes the input value. */
141141
afterRenderEffect(() => {
142+
this.value();
142143
this.combobox.popup()?.controls()?.items();
143144
untracked(() => this.combobox.pattern.onFilter());
144145
});

src/aria/ui-patterns/combobox/combobox.ts

Lines changed: 45 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,8 @@ export class ComboboxPattern<T extends ListItem<V>, V> {
152152
if (!this.expanded()) {
153153
return new KeyboardEventManager()
154154
.on('ArrowDown', () => this.open({first: true}))
155-
.on('ArrowUp', () => this.open({last: true}));
155+
.on('ArrowUp', () => this.open({last: true}))
156+
.on('Escape', () => this.close({reset: true}));
156157
}
157158

158159
const popupControls = this.inputs.popupControls();
@@ -166,21 +167,7 @@ export class ComboboxPattern<T extends ListItem<V>, V> {
166167
.on('ArrowUp', () => this.prev())
167168
.on('Home', () => this.first())
168169
.on('End', () => this.last())
169-
.on('Escape', () => {
170-
// TODO(wagnermaciel): We may want to fold this logic into the close() method.
171-
if (this.inputs.filterMode() === 'highlight' && popupControls.activeId()) {
172-
popupControls.unfocus();
173-
popupControls.clearSelection();
174-
175-
const inputEl = this.inputs.inputEl();
176-
if (inputEl) {
177-
inputEl.value = this.inputs.inputValue!();
178-
}
179-
} else {
180-
this.close();
181-
this.inputs.popupControls()?.clearSelection();
182-
}
183-
}) // TODO: When filter mode is 'highlight', escape should revert to the last committed value.
170+
.on('Escape', () => this.close({reset: true}))
184171
.on('Enter', () => this.select({commit: true, close: true}));
185172

186173
if (popupControls.role() === 'tree') {
@@ -253,6 +240,10 @@ export class ComboboxPattern<T extends ListItem<V>, V> {
253240
this.inputs.popupControls()?.clearSelection();
254241
}
255242
}
243+
244+
if (this.inputs.filterMode() === 'highlight' && !this.isDeleting) {
245+
this.highlight();
246+
}
256247
}
257248

258249
/** Handles focus in events for the combobox. */
@@ -367,15 +358,50 @@ export class ComboboxPattern<T extends ListItem<V>, V> {
367358
}
368359

369360
/** Closes the combobox. */
370-
close() {
371-
this.expanded.set(false);
372-
this.inputs.popupControls()?.unfocus();
361+
close(opts?: {reset: boolean}) {
362+
if (!opts?.reset) {
363+
this.expanded.set(false);
364+
this.inputs.popupControls()?.unfocus();
365+
return;
366+
}
367+
368+
const popupControls = this.inputs.popupControls();
369+
370+
if (!this.expanded()) {
371+
this.inputs.inputValue?.set('');
372+
popupControls?.clearSelection();
373+
374+
const inputEl = this.inputs.inputEl();
375+
376+
if (inputEl) {
377+
inputEl.value = '';
378+
}
379+
} else if (this.expanded()) {
380+
this.close();
381+
382+
const selectedItem = popupControls?.getSelectedItem();
383+
384+
if (selectedItem?.searchTerm() !== this.inputs.inputValue!()) {
385+
popupControls?.clearSelection();
386+
}
387+
}
373388
}
374389

375390
/** Opens the combobox. */
376391
open(nav?: {first?: boolean; last?: boolean}) {
377392
this.expanded.set(true);
378393

394+
const inputEl = this.inputs.inputEl();
395+
396+
if (inputEl) {
397+
const isHighlighting = inputEl.selectionStart !== inputEl.value.length;
398+
this.inputs.inputValue?.set(inputEl.value.slice(0, inputEl.selectionStart || 0));
399+
400+
if (!isHighlighting) {
401+
this.highlightedItem.set(undefined);
402+
}
403+
}
404+
379405
if (nav?.first) {
380406
this.first();
381407
}

0 commit comments

Comments
 (0)