Skip to content

split button focus behavior #2137

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

Closed
wants to merge 5 commits into from
Closed

split button focus behavior #2137

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
16e2fd1
updated splitbutton for focusbehavior
varun-srinivasa Sep 15, 2025
ba036bb
Added test cases for splitbutton focusbehavior
varun-srinivasa Sep 15, 2025
76f1ea7
updated testcase and removed only
varun-srinivasa Sep 15, 2025
4b4b9d3
Merge branch 'main' into ix/splitButton-focusBehavior
nuke-ellington Oct 2, 2025
b487a16
updated splitbutton and focus
varun-srinivasa Oct 13, 2025
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
68 changes: 68 additions & 0 deletions packages/core/src/components/split-button/split-button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { AlignedPlacement } from '../dropdown/placement';
import { iconContextMenu } from '@siemens/ix-icons/icons';
import { CloseBehavior } from '../dropdown/dropdown-controller';
import type { SplitButtonVariant } from './split-button.types';
import { ArrowFocusController } from '../utils/focus';

@Component({
tag: 'ix-split-button',
Expand Down Expand Up @@ -88,13 +89,79 @@ export class SplitButton {

private triggerElement?: HTMLElement;
private dropdownElement?: HTMLIxDropdownElement;
private arrowFocusController: ArrowFocusController | undefined;

private linkTriggerRef() {
if (this.triggerElement && this.dropdownElement) {
this.dropdownElement.trigger = this.triggerElement;
}
}

private get dropdownItems(): HTMLElement[] {
return Array.from(this.hostElement.querySelectorAll('ix-dropdown-item'));
}
private onDropdownShowChanged(event: CustomEvent<boolean>) {
if (event.detail) {
this.arrowFocusController = new ArrowFocusController(
this.dropdownItems,
this.dropdownElement!,
(index: number) => this.focusDropdownItem(index)
);
Comment on lines +118 to +122
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Oct 1, 2025

Choose a reason for hiding this comment

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

critical

The ArrowFocusController will not work as expected here. Its getActiveIndex method compares document.activeElement directly with the items in the list. However, your focusDropdownItem function focuses an inner <button> element inside the ix-dropdown-item's shadow DOM. This means document.activeElement will be this inner button, not the ix-dropdown-item element itself. As a result, getActiveIndex will always return -1, breaking the arrow key navigation.

To fix this, ArrowFocusController needs to be updated to correctly find the active item, for example by checking if an item contains(document.activeElement). Since utils/focus.ts is not part of this PR, you might need to include it or find a workaround.

The new tests in split-button-keyboard.ct.ts are also likely to fail because they expect the ix-dropdown-item host to be focused (ix-dropdown-item:focus), which is not what happens.

this.arrowFocusController.wrap = true;
requestAnimationFrame(() => this.focusDropdownItem(0));
this.dropdownElement?.addEventListener('keydown', this.handleKeyDown);
} else {
this.arrowFocusController?.disconnect();
this.arrowFocusController = undefined;
this.dropdownElement?.removeEventListener('keydown', this.handleKeyDown);
}
}

private handleKeyDown = (event: KeyboardEvent) => {
if (event.key !== 'Tab') {
return;
}
this.dropdownItems.forEach((item) => item.setAttribute('tabindex', '-1'));
Copy link
Collaborator

@nuke-ellington nuke-ellington Oct 1, 2025

Choose a reason for hiding this comment

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

Why do we need to set the tabIndex?

Copy link
Contributor Author

@varun-srinivasa varun-srinivasa Oct 7, 2025

Choose a reason for hiding this comment

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

When Shift+Tab is pressed, the browser's default behavior is to move focus to the previous focusable element, which in this case is the split-button's dropdown trigger (the anchorButton).
However, the desired behavior is to close the dropdown and move focus to the element before the entire split-button component.
To achieve this, we temporarily set tabindex="-1" on the anchorButton, making it non-focusable. This allows the browser's default Shift+Tab action to skip the anchorButton and move focus to the correct preceding element.
The tabindex is immediately restored within a requestAnimationFrame callback so that the anchorButton is focusable again for future interactions.

this.dropdownElement!.show = false;
Copy link
Collaborator

@nuke-ellington nuke-ellington Oct 1, 2025

Choose a reason for hiding this comment

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

Why is this necessary?

if (!event.shiftKey) {
return;
}
const actionButton = this.hostElement.shadowRoot?.querySelector(
'ix-button, ix-icon-button:not(.anchor)'
) as HTMLElement | null;

const anchorButton = this.hostElement.shadowRoot?.querySelector(
Copy link
Collaborator

@nuke-ellington nuke-ellington Oct 1, 2025

Choose a reason for hiding this comment

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

Getter would be better IMO.

'ix-icon-button.anchor'
) as HTMLElement | null;

const isDisabled = actionButton?.classList.contains('disabled');

if (actionButton && !isDisabled) {
event.preventDefault();
requestAnimationFrame(() => {
const shadowBtn = actionButton.shadowRoot?.querySelector('button');
(shadowBtn ?? actionButton).focus();
});
} else if (anchorButton) {
anchorButton.setAttribute('tabindex', '-1');
requestAnimationFrame(() => {
anchorButton.removeAttribute('tabindex');
});
}
};

private focusDropdownItem(index: number) {
if (index < 0) return;
const items = this.dropdownItems;
items.forEach((item, i) =>
item.setAttribute('tabindex', i === index ? '0' : '-1')
Copy link
Collaborator

@nuke-ellington nuke-ellington Oct 1, 2025

Choose a reason for hiding this comment

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

Why do we need to change the tabIndex?

Copy link
Contributor Author

@varun-srinivasa varun-srinivasa Oct 7, 2025

Choose a reason for hiding this comment

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

When the dropdown is closed by pressing the Tab key, we set the tabindex of all dropdown items to -1. This removes them from the natural tab order of the page. It ensures that a user cannot accidentally Tab into the items of a closed dropdown. Focus should move to the next focusable element outside of the split-button component, which is the standard and expected keyboard navigation behavior.

);
const item = items[index];
requestAnimationFrame(() => {
item.shadowRoot?.querySelector('button')?.focus();
});
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Oct 1, 2025

Choose a reason for hiding this comment

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

high

The code attempts to access item.shadowRoot without checking if item is defined. If the split button has no dropdown items, this.dropdownItems will be empty, and items[index] will be undefined. This will cause a runtime error on item.shadowRoot. Please add a guard to ensure item exists before using it.

    const item = items[index];
    if (item) {
      requestAnimationFrame(() => {
        item.shadowRoot?.querySelector('button')?.focus();
      });
    }

}

componentDidLoad() {
this.linkTriggerRef();
}
Expand Down Expand Up @@ -139,6 +206,7 @@ export class SplitButton {
<ix-dropdown
ref={(r) => (this.dropdownElement = r)}
closeBehavior={this.closeBehavior}
onShowChanged={(e) => this.onDropdownShowChanged(e)}
>
<slot></slot>
</ix-dropdown>
Expand Down
133 changes: 133 additions & 0 deletions packages/core/src/components/split-button/test/split-button-keyboard.ct.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
/*
* SPDX-FileCopyrightText: 2025 Siemens AG
*
* SPDX-License-Identifier: MIT
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

import { expect, Locator } from '@playwright/test';
import { regressionTest } from '@utils/test';

function splitButtonController(splitBtn: Locator) {
const mainButton = splitBtn.locator('ix-button').first();
const chevronButton = splitBtn.locator('ix-icon-button.anchor');
const dropdown = splitBtn.locator('ix-dropdown');

const dropdownVisible = async () => {
const element = await dropdown.elementHandle();
if (!element) {
throw new Error('Dropdown has no open handle');
}
await element.waitForElementState('stable');
await expect(dropdown).toBeVisible();
};

return {
async clickMainButton() {
await mainButton.click();
},
async clickChevron() {
await chevronButton.click();
await dropdownVisible();
},
async arrowDown(skipDropdownCheck = false) {
if (!skipDropdownCheck) {
await dropdownVisible();
}
await splitBtn.page().keyboard.press('ArrowDown', { delay: 50 });
},
async arrowUp(skipDropdownCheck = false) {
if (!skipDropdownCheck) {
await dropdownVisible();
}
await splitBtn.page().keyboard.press('ArrowUp', { delay: 50 });
},
async pressEnter() {
await splitBtn.page().keyboard.press('Enter');
},
async getDropdownItemsLocator() {
await dropdownVisible();
return splitBtn.locator('ix-dropdown-item').all();
},
async getFocusedItemLocator() {
await dropdownVisible();
return splitBtn.locator('ix-dropdown-item:focus');
},
};
}

regressionTest(
'ArrowDown cycles through dropdown items',
async ({ mount, page }) => {
await mount(`
<ix-split-button>
Action
<ix-dropdown-item>Item 1</ix-dropdown-item>
<ix-dropdown-item>Item 2</ix-dropdown-item>
<ix-dropdown-item>Item 3</ix-dropdown-item>
</ix-split-button>
`);

const splitBtn = page.locator('ix-split-button');
const ctrl = splitButtonController(splitBtn);

await ctrl.clickChevron();
await ctrl.arrowDown();
await ctrl.arrowDown();

const items = await ctrl.getDropdownItemsLocator();
const focused = await ctrl.getFocusedItemLocator();

expect(items).toHaveLength(3);
await expect(focused).toHaveText('Item 3');
}
);

regressionTest('ArrowUp cycles backwards', async ({ mount, page }) => {
await mount(`
<ix-split-button>
Action
<ix-dropdown-item>Item A</ix-dropdown-item>
<ix-dropdown-item>Item B</ix-dropdown-item>
</ix-split-button>
`);

const splitBtn = page.locator('ix-split-button');
const ctrl = splitButtonController(splitBtn);

await ctrl.clickChevron();
await ctrl.arrowDown();
await ctrl.arrowUp();

const focused = await ctrl.getFocusedItemLocator();
await expect(focused).toHaveText('Item A');
});

regressionTest(
'Tab from dropdown item moves focus to next component',
async ({ mount, page }) => {
await mount(`
<ix-split-button>
Action
<ix-dropdown-item>Item 1</ix-dropdown-item>
<ix-dropdown-item>Item 2</ix-dropdown-item>
<ix-dropdown-item>Item 3</ix-dropdown-item>
</ix-split-button>
<ix-button id="next">Next Component</ix-button>
`);

const splitBtn = page.locator('ix-split-button');
const ctrl = splitButtonController(splitBtn);

await ctrl.clickChevron();
await ctrl.arrowDown();
await ctrl.arrowDown();

await page.keyboard.press('Tab');

const nextButton = page.locator('#next');
await expect(nextButton).toBeFocused();
}
);
Loading