Skip to content

Commit

Permalink
Fix Select arrow down and up behaviors with hidden selected option (#…
Browse files Browse the repository at this point in the history
…2314)

# Pull Request

## 🤨 Rationale

While doing the integration work of updating the SLE user selector to
use dynamic options, a non-ideal behavior became apparent in the case
where a user would provide some filter text, filtering out the current
selected user (which as our guidance states, requires the option to be
present but hidden), and then press `ArrowDown`, and nothing would get
selected. This would happen when the selected option was _below_ the
other options in DOM order.

The two solutions for creating the expected behavior were 1) update the
integration code to always put the hidden option at the top of the DOM
order, or 2) change the Nimble Select behavior to provide the expected
behavior in this case. I opted with 2 because it feels like a gap that
any user of the Nimble Select could possibly hit.

## 👩‍💻 Implementation

Now handling the `ArrowDown` and `ArrowUp` cases directly in the key
handling code of the `Select`.

## 🧪 Testing

Added some unit tests. Manually checked behavior in Angular example app
with dynamic options.

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [ ] I have updated the project documentation to reflect my changes or
determined no changes are needed.

---------

Co-authored-by: Milan Raj <[email protected]>
  • Loading branch information
atmgrifter00 and rajsite authored Aug 1, 2024
1 parent bbede1f commit c3e3478
Show file tree
Hide file tree
Showing 3 changed files with 126 additions and 1 deletion.
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "Update Select arrow down behavior to select first option in dropdown when selected option is visually hidden.",
"packageName": "@ni/nimble-components",
"email": "[email protected]",
"dependentChangeType": "patch"
}
45 changes: 44 additions & 1 deletion packages/nimble-components/src/select/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -567,15 +567,58 @@ export class Select
*/
public override keydownHandler(e: KeyboardEvent): BooleanOrVoid {
const initialSelectedIndex = this.selectedIndex;
super.keydownHandler(e);
const key = e.key;
if (e.ctrlKey || e.shiftKey) {
return true;
}

if (key !== keyArrowDown && key !== keyArrowUp) {
super.keydownHandler(e);
}

let currentActiveIndex = this.openActiveIndex ?? this.selectedIndex;
let commitValueThenClose = false;
switch (key) {
case keyArrowDown: {
const selectedOption = this.options[this.selectedIndex];
if (
this.open
&& isListOption(selectedOption)
&& !isOptionOrGroupVisible(selectedOption)
) {
if (this.openActiveIndex === this.selectedIndex) {
this.selectFirstOption();
} else {
this.selectNextOption();
}
} else {
super.keydownHandler(e);
}

// update currentActiveIndex again as dependent state may have changed
currentActiveIndex = this.openActiveIndex ?? this.selectedIndex;
break;
}
case keyArrowUp: {
const selectedOption = this.options[this.selectedIndex];
if (
this.open
&& isListOption(selectedOption)
&& !isOptionOrGroupVisible(selectedOption)
) {
if (this.openActiveIndex === this.selectedIndex) {
this.selectLastOption();
} else {
this.selectPreviousOption();
}
} else {
super.keydownHandler(e);
}

// update currentActiveIndex again as dependent state may have changed
currentActiveIndex = this.openActiveIndex ?? this.selectedIndex;
break;
}
case keySpace: {
// when dropdown is open allow user to enter a space for filter text
if (this.open && this.filterMode !== FilterMode.none) {
Expand Down
75 changes: 75 additions & 0 deletions packages/nimble-components/src/select/tests/select.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ type OptionInitialState =
| 'disabled hidden'
| 'disabled selected hidden'
| 'hidden'
| 'hidden selected'
| 'visually-hidden';

async function setup(
Expand Down Expand Up @@ -585,6 +586,80 @@ describe('Select', () => {
await disconnect();
});

it('when second option is selected and hidden, pressing arrow down while dropdown is open selects first option', async () => {
const { element, connect, disconnect } = await setup(
undefined,
false,
undefined,
'hidden selected'
);
const pageObject = new SelectPageObject(element);
await connect();
await waitForUpdatesAsync();
pageObject.clickSelect();
pageObject.pressArrowDownKey();

expect(pageObject.getActiveOption()?.value).toBe('one');

await disconnect();
});

it('when second option is selected and hidden, pressing arrow down twice while dropdown is open selects third option', async () => {
const { element, connect, disconnect } = await setup(
undefined,
false,
undefined,
'hidden selected'
);
const pageObject = new SelectPageObject(element);
await connect();
await waitForUpdatesAsync();
pageObject.clickSelect();
pageObject.pressArrowDownKey();
pageObject.pressArrowDownKey();

expect(pageObject.getActiveOption()?.value).toBe('three');

await disconnect();
});

it('when second option is selected and hidden, pressing arrow up while dropdown is open selects last option', async () => {
const { element, connect, disconnect } = await setup(
undefined,
false,
undefined,
'hidden selected'
);
const pageObject = new SelectPageObject(element);
await connect();
await waitForUpdatesAsync();
pageObject.clickSelect();
pageObject.pressArrowUpKey();

expect(pageObject.getActiveOption()?.value).toBe('has space');

await disconnect();
});

it('when second option is selected and hidden, pressing arrow up twice while dropdown is open selects second to last option', async () => {
const { element, connect, disconnect } = await setup(
undefined,
false,
undefined,
'hidden selected'
);
const pageObject = new SelectPageObject(element);
await connect();
await waitForUpdatesAsync();
pageObject.clickSelect();
pageObject.pressArrowUpKey();
pageObject.pressArrowUpKey();

expect(pageObject.getActiveOption()?.value).toBe('zürich');

await disconnect();
});

describe('with all options disabled', () => {
async function setupAllDisabled(): Promise<Fixture<Select>> {
const viewTemplate = html`
Expand Down

0 comments on commit c3e3478

Please sign in to comment.