fix(Dropdown): updated structure for dropdown menu list item by ssherigar-c-eightfold · Pull Request #1113 · EightfoldAI/octuple

ssherigar-c-eightfold · 2026-03-31T13:48:26Z

SUMMARY:

MenuItemButton

Moved interactivity from button → li (with role, tabIndex, keyboard handlers)
Added full keyboard support (Arrow keys, Enter/Space)
Simplified nested items (use span instead of button)

Dropdown

Removed default referenceRole='button'
Removed incorrect role='listbox' from overlay
Fixed aria-haspopup usage
- The fix removes the default value 'true' from ariaHaspopupValue:
  Before : ariaHaspopupValue defaulted to 'true', so aria-haspopup="true" was always present on the dropdown button, even when no value was passed.
  After : ariaHaspopupValue has no default. When nothing is passed (or false is explicitly passed), aria-haspopup is omitted entirely. The attribute only appears when a string value is explicitly provided by the parent component.
Role only applied when explicitly passed

List

Added ariaLabelledBy → maps to aria-labelledby, to allow lists to be programmatically associated with visible labels
Only render the role attribute for ul and li tag when explicitly provided by the parent. Remove default/implicit role assignment to avoid unnecessary roles in the DOM.

GITHUB ISSUE (Open Source Contributors)

NA

JIRA TASK (Eightfold Employees Only):

https://eightfoldai.atlassian.net/browse/ENG-167674
https://eightfoldai.atlassian.net/browse/ENG-167852
https://eightfoldai.atlassian.net/browse/ENG-167512

CHANGE TYPE:

Bugfix Pull Request
Feature Pull Request

TEST COVERAGE:

Tests for this change already exist
I have added unittests for this change

TEST PLAN:

Check dropdown list items for accessibility compliance
Dropdown

Verify click and keyboard (Enter/Space) interactions work correctly
Ensure arrow keys support proper navigation between items
Confirm button is removed and replaced with a non-interactive element (e.g., span), with interaction handled at the parent level
Validate that appropriate roles are applied
Ensure correct use of aria-haspopup, aria-expanded, and aria-controls
Confirm existing ARIA attributes are preserved
Ensure no incorrect roles are applied to the overlay

codesandbox-ci · 2026-03-31T13:51:29Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

ychhabra-eightfold

Regression Risk Analysis

This PR restructures Dropdown menu list items for a11y — moving interactivity from inner <button> to <li>, fixing ARIA roles, and adding keyboard navigation. The a11y intent is solid, but there are several areas with regression risk that need attention.

Overall assessment: The structural change from <li role="presentation"> > <button role="menuitem"> to <li role="menuitem"> is directionally correct for ARIA compliance, but the scope of changes across Dropdown, Menu, Select, and multiple snapshot-dependent components creates a wide blast radius. See inline comments for specific risks.

ychhabra-eightfold · 2026-04-06T11:16:02Z

            {iconProps && alignIcon === MenuItemIconAlign.Right && getIcon()}
-          </button>
+          </span>
          <span className={styles.menuInnerButton}>


Regression Risk: Disabled items are still focusable and clickable

aria-disabled does not prevent focus or click the way disabled on a native <button> does. With the old structure, <button disabled> natively prevented both. Now:

Focus: Disabled <li> elements still receive tabIndex={active ? 0 : tabIndex} (default 0), so users can Tab/Arrow into disabled items.

Click: handleOnClick checks if (disabled) and calls preventDefault(), but this only prevents the onClick callback — it does not prevent other event handlers that consumers may have added via ...rest.

Consider setting tabIndex={disabled ? -1 : (active ? 0 : tabIndex)} to fully replicate the old native button behavior.

Fixed, tabIndex={disabled ? -1 : active ? 0 : tabIndex}
Disabled items now get tabIndex={-1}, removing them from the tab sequence and arrow key navigation, matching the old native behavior

ychhabra-eightfold · 2026-04-06T11:16:11Z

              <span className={styles.itemText}>
                <span className={styles.label}>{text}</span>
              </span>
            </span>


Regression Risk: ...rest spread moved from <button> to <li>

Previously, ...rest was spread onto the inner <button>. Now it's on the <li>. This is a breaking change for consumers who pass HTML button-specific attributes (e.g., type, form, formAction) or event handlers expecting HTMLButtonElement targets.

The type was also changed from OcBaseProps<HTMLButtonElement> to OcBaseProps<HTMLLIElement>, which is correct for TypeScript consumers, but any JavaScript consumers passing button-specific props will get silent runtime bugs.

Worth calling out as a breaking change in release notes.

The type was updated from OcBaseProps to OcBaseProps, so TypeScript consumers will get a compile-time error if they pass button-specific props like type, form, or formAction.

ychhabra-eightfold · 2026-04-06T11:16:20Z

+      if (isNested) {
+        return (
+          <span className={styles.menuItemButton}>{menuButtonContent()}</span>
+        );


Regression Risk: Duplicate keyboard navigation

handleLiKeyDown implements ArrowUp/Down/Home/End navigation by querying [role="option"], [role="menuitem"] from the parent. However, the List component already has its own keyboard navigation logic (disableArrowKeys, focus management). This creates two competing keyboard handlers on the same DOM tree.

If a <Menu> is rendered inside a <List> (which it typically is), both handlers will fire on the same keypress, potentially moving focus twice or conflicting.

Also, the selector only matches option and menuitem roles — if any consumer uses a custom role (e.g., treeitem), those items will be invisible to the arrow key navigation.

Resolved. Arrow key handling was removed since the List component already handles it. handleLiKeyDown now only handles Enter/Space to trigger click on li tag, which isn’t native like button

ychhabra-eightfold · 2026-04-06T11:16:31Z

      {
        ariaRef,
-        ariaHaspopupValue = 'true',
+        ariaHaspopupValue,


Regression Risk: Removing ariaHaspopupValue default silently breaks all Dropdown triggers

Previously, every Dropdown trigger automatically got aria-haspopup="true". Now it's omitted unless explicitly passed. This is a wide-impact behavioral change — every existing Dropdown in the app that doesn't pass ariaHaspopupValue will lose its aria-haspopup attribute.

Per ARIA spec, aria-haspopup is required on elements that trigger popups. Removing it will cause accessibility audit failures (e.g., Axe, Lighthouse) for all Dropdown usages that don't explicitly set it.

The combobox special-casing to add listbox is correct, but non-combobox Dropdown triggers (the majority of usages) now have no aria-haspopup at all. Consider defaulting to 'menu' instead of removing the default entirely, as that's the most common Dropdown use case.

This is intentional, aria-haspopup is applied only when a value is explicitly provided from the parent via ariaHaspopupValue; otherwise, the attribute is not added to the element

ychhabra-eightfold · 2026-04-06T11:16:40Z

            ariaRef.current.getAttribute('aria-haspopup');
-          if (currentRole !== 'combobox' && !currentAriaHaspopup) {
-            ariaRef.current.setAttribute('aria-haspopup', ariaHaspopupValue);
+          if (


Regression Risk: Removing role="listbox" from dropdown overlay

The role prop with default 'listbox' is removed entirely from the overlay <div>. This means the dropdown overlay container now has no ARIA role unless passed via overlayProps.

While the inner <Menu> typically has role="menu", the overlay wrapper is the element referenced by aria-controls on the trigger. Some screen readers use aria-controls + the target's role to announce what will open. Without a role on the overlay, the association may not be announced correctly.

Consumers relying on role prop (which was a public prop on DropdownProps) will also find it no longer does anything since it's been destructured out.

Removing role="listbox" from the overlay is correct.
The overlay is just a wrapper, not the interactive element. The actual role is already applied inside (menu/listbox), and aria-haspopup on the trigger is enough for screen readers.
Adding a role to the wrapper can cause incorrect semantics

ychhabra-eightfold · 2026-04-06T11:16:47Z

-            type={htmlType}
-            {...rest}
-            onClick={handleOnClick}
-            ref={ref}


Code quality: as any cast hides type mismatch

onKeyDown?.(event as any) in the default case bypasses TypeScript safety. The onKeyDown prop was originally typed for HTMLButtonElement keyboard events, but the event is now from HTMLLIElement. Rather than casting to any, the prop type should be updated to accept React.KeyboardEvent<HTMLLIElement>, or the type should be widened to React.KeyboardEvent<HTMLElement>.

ychhabra-eightfold · 2026-04-06T11:16:55Z

  const getHeader = (): JSX.Element =>
    header && (
      <div className={headerClassNames}>
-        <div className={styles.heading}>{header}</div>


Regression Risk: Hardcoded <h2> may break heading hierarchy

Changing from <div> to <h2> is good for a11y (gives the menu a semantic heading), but hardcoding <h2> may break the document heading hierarchy depending on where the Menu is rendered. If a Menu appears inside a section that's already under an <h2>, this creates an incorrect heading level.

Consider making the heading level configurable (e.g., headerLevel prop defaulting to h2), or use role="heading" aria-level={n} for flexibility.

This is a requirement , we need to use a semantic h2, tag for the heading

ychhabra-eightfold · 2026-04-06T11:17:07Z

-    expect(screen.getByTestId('option1-test-id').matches(':focus')).toBe(true);
+    expect(select).toHaveFocus();
  });



Regression Risk: Focus tests weakened rather than fixed

Multiple tests previously asserted that opening the dropdown focused the first option (option1-test-id.matches(':focus')). These have been replaced with expect(select).toHaveFocus(), meaning the select input retains focus instead of moving to the options.

This is a significant UX/a11y behavior change:

Before: Opening a Select with keyboard moved focus into the listbox options, allowing immediate arrow key navigation of the options list.

After: Focus stays on the input, meaning users must use a different mechanism to navigate options.

If this is intentional (virtual focus via aria-activedescendant), that pattern requires aria-activedescendant to be set during arrow key navigation — but the updated filterable test only checks aria-activedescendant after a click, not during keyboard nav. This may leave keyboard-only users without proper option announcement.

ychhabra-eightfold · 2026-04-06T11:17:16Z

-        disabled={disabled}
-        tabIndex={tabIndex}
-        type={htmlType}
-        {...rest}


Regression Risk: secondaryButton lost its click handler and ref

The inner <button> in secondaryButton() was replaced with a <span>, but the onClick={handleOnClick} and ref={ref} that were on that button are now gone. The <li> has onClick={!dropdownMenuItems ? handleOnClick : undefined}, but secondaryButton items may or may not have dropdownMenuItems.

If a menu item uses secondaryButtonProps, the primary text area (menuOuterButton span) is no longer independently clickable — clicks only work on the <li>. This might be fine, but verify it doesn't break split-button menu items where the primary and secondary actions are distinct.

ref - Not lost, It moved from the inner button to the li, which is the correct element to reference.
onClick regression , Clicking the secondary button was bubbling and triggering the primary action. Fixed by adding e.stopPropagation(), so only the intended handler runs

ychhabra-eightfold

Review of accessibility refactor. Strong direction overall — moving interactivity onto <li> to drop redundant button wrappers is the right ARIA pattern for menuitem. Flagging a few regressions and concerns below.

High-impact regressions

List silently drops aria-label on the rendered <ul>/<ol>. The destructured prop 'aria-label': ariaLabel is no longer applied — every existing consumer relying on aria-label loses its accessible name with no migration path. aria-labelledby should be additive, not a replacement.
Menu removed the default role='menu', so the <ul> rendered by List no longer has a container role, while its children still claim role='menuitem'. Per WAI-ARIA, menuitem requires a menu/menubar ancestor — visible in the new snapshots (<ul class="list-container vertical"> with <li role="menuitem"> inside). This is a spec violation that screen readers will pick up on.
MenuItemButton sets tabIndex={disabled ? -1 : active ? 0 : tabIndex} where tabIndex defaults to 0. That means every non-disabled menu item is tabindex=0, breaking the roving-tabindex pattern that menus require — Tab will now stop on every item. Confirmed in the new snapshots (all non-disabled items show tabindex="0").

Other concerns

MenuItemCustom and Menu overlay (role='listbox') defaults were removed too — same shape of problem as #2 for any consumer that was relying on the implicit role.
MenuItemButton spreads {...rest} on the <li> after aria-disabled/tabIndex but before onClick/onKeyDown — accidental aria-disabled/tabIndex keys in rest will silently override the deliberate values.
The dropdown-trigger menu item (dropdownMenuItems branch) loses keyboard activation: handleLiKeyDown skips Enter/Space when dropdownMenuItems is set, and there's no <button> underneath anymore. Worth verifying the wrapping Dropdown reference handles this.
Menu header was hard-coded to <h2>. That forces a heading level on every consumer regardless of where the menu lives in the document outline — should be configurable (e.g. headerLevel prop) and default to a non-heading element to preserve current behavior.
Dropdown now eagerly sets aria-haspopup="listbox" on combobox elements (the test was updated from toBeNull() to 'listbox'). This is correct per ARIA 1.2 but is a behavior change — confirm no consumer was deliberately leaving it off (e.g. for combobox-with-menu popup).
MenuItem.types.ts changed NativeMenuButtonProps from OcBaseProps<HTMLButtonElement> to OcBaseProps<HTMLLIElement>. Public type change — breaks any consumer that destructured a button-typed ref/event from MenuItemButtonProps.

ychhabra-eightfold · 2026-04-13T06:22:40Z

-          role={role}
-          aria-label={ariaLabel}
+          {...(role !== undefined && { role })}
+          aria-labelledby={ariaLabelledBy}


Regression: the existing aria-label prop is destructured (line 39 'aria-label': ariaLabel) but no longer rendered. Replacing it with aria-labelledby silently breaks every consumer that passes aria-label for the list's accessible name — and they get no warning or migration path.

Keep both: render aria-label={ariaLabel} alongside aria-labelledby={ariaLabelledBy}. They're not mutually exclusive, and aria-labelledby should be additive.

ychhabra-eightfold · 2026-04-13T06:22:40Z

-          role={role}
-          aria-label={ariaLabel}
+          {...(role !== undefined && { role })}
+          aria-labelledby={ariaLabelledBy}


Same as the <ul> branch: dropping aria-label here is a silent breaking change for <ol> consumers. Render both aria-label and aria-labelledby.

ychhabra-eightfold · 2026-04-13T06:22:40Z

  onChange,
  onOk,
-  role = 'menu',
+  role,


Removing the 'menu' default cascades into a real accessibility regression: Menu now passes role={undefined} to List, which (combined with the List-side role change) renders <ul> with no role at all — while its <li> children still carry role='menuitem'. Per WAI-ARIA, menuitem must have a menu/menubar ancestor. Visible in the updated Menu.test.tsx.snap: the ul has no role but every li is role="menuitem".

Keep role = 'menu' as the default (or pass it explicitly to List here) to preserve the parent/child role contract.

default role is removed from both ul and li tag, role will render only when explicitly provided by the parent

ychhabra-eightfold · 2026-04-13T06:22:40Z

    header && (
      <div className={headerClassNames}>
-        <div className={styles.heading}>{header}</div>
+        <h2 className={styles.heading}>{header}</h2>


Hard-coding <h2> forces a fixed heading level on every Menu instance regardless of the surrounding document outline. A menu inside an <h3> section, or one with no heading parent at all, will produce an invalid heading hierarchy.

Make the heading level configurable (e.g. a headerAs/headerLevel prop) and default to the previous <div> to keep existing renders backward-compatible. If the goal is just to make headers focusable/labellable, consider role='heading' aria-level={n} driven by a prop instead.

This is a requirement , we need to use a semantic h2, tag for the heading
styling is updated, as per previous design

ychhabra-eightfold · 2026-04-13T06:22:40Z

-      <li className={menuItemClassNames} role={listItemRole ?? 'presentation'}>
+      <li
+        className={menuItemClassNames}
+        aria-disabled={disabled ? true : undefined}


This breaks the roving-tabindex pattern that menu/menubar requires. tabIndex prop defaults to 0 (line 33), so every non-disabled item evaluates to tabIndex=0 — meaning Tab stops on every menu item instead of just the active one. Confirmed in the new snapshots: non-disabled items all render tabindex="0".

The correct shape is: tabIndex = disabled ? -1 : (active ? 0 : -1) with the Menu/List parent driving which item is currently the tab stop. Otherwise focus management inside the menu is broken for keyboard users.

Tab cycling through all menu items is intentional — this component does not use the WAI-ARIA roving tabindex pattern. Each item remains a Tab stop to support both Tab and arrow key navigation, and shouldCloseOnTab provides a clear exit path. tabIndex=0 is intentional, so no change is needed

ychhabra-eightfold · 2026-04-13T06:22:40Z

+        className={menuItemClassNames}
+        aria-disabled={disabled ? true : undefined}
+        tabIndex={disabled ? -1 : active ? 0 : tabIndex}
+        {...rest}


When dropdownMenuItems is set, click is intentionally disabled here, and handleLiKeyDown (line 152) also skips Enter/Space in that branch. The nested content is now a <span> (no native interactivity). That leaves the dropdown-trigger menu item with no keyboard activation path on the <li> itself — it relies entirely on the wrapping Dropdown reference handlers being attached to this same node.

Please confirm that the menuRenderer-wrapped Dropdown does in fact bind its trigger handlers to this <li> (and not to the inner <span>). If not, Enter/Space will no longer open nested dropdowns.

Enter/Space for nested dropdowns still works via getItemProps()’s merged onKeyDown. handleLiKeyDown only triggers currentTarget.click() when !dropdownMenuItems; otherwise, the event falls through and Floating UI handles submenu activation. Verified in practice: arrow key navigation and Enter/Space both work correctly, so no change is needed.

ychhabra-eightfold · 2026-04-13T06:22:40Z

 }

-type NativeMenuButtonProps = Omit<OcBaseProps<HTMLButtonElement>, 'children'>;
+type NativeMenuButtonProps = Omit<OcBaseProps<HTMLLIElement>, 'children'>;


Public-API type change: NativeMenuButtonProps switched from HTMLButtonElement to HTMLLIElement. Any consumer that typed an event handler or ref against MenuItemButtonProps (e.g. React.MouseEvent<HTMLButtonElement>) will get a TS error. Worth calling out in the changelog as a breaking type change, even if the runtime behavior intent is the same.

ychhabra-eightfold · 2026-04-13T06:22:40Z

+              ariaRef.current.setAttribute('aria-haspopup', 'listbox');
+            } else if (ariaHaspopupValue !== false) {
+              ariaRef.current.setAttribute('aria-haspopup', ariaHaspopupValue);
+            }


Behavior change worth flagging in the description: previously combobox refs were left without aria-haspopup (the test asserted .toBeNull()); now they're force-set to 'listbox'. ARIA 1.2 allows 'listbox' | 'tree' | 'grid' | 'dialog' | 'menu' for combobox popups — 'listbox' is the right default for Select, but a combobox whose popup is a menu or dialog will now get the wrong value with no way to opt out.

Consider honoring ariaHaspopupValue for combobox too (fall through to it when provided), so callers can override the implicit 'listbox'.

ychhabra-eightfold · 2026-04-13T06:22:41Z

              onKeyDown={handleFloatingKeyDown}
              id={dropdownId}
-              role={role}
              {...overlayProps}


Removing the default role='listbox' on the overlay is reasonable, but combine it with the referenceRole default also being removed and you get a regression for any consumer that was relying on either default — no warning, just silently missing roles. Consider keeping the role prop pass-through ({...(role !== undefined && { role })}) so callers that explicitly pass one still see it applied.

Removed the redundant role from the wrapper div, since the same role is already correctly applied to the ul.

ychhabra-eightfold · 2026-04-13T06:22:41Z


    return (
-      <li className={menuItemClassNames} role={role}>
+      <li className={menuItemClassNames} {...(role !== undefined && { role })}>


Same theme as the other default-removals: dropping the role='presentation' default leaves custom items with no role. Inside a role='menu' ul, a roleless <li> with arbitrary content will be exposed as a generic list item to AT, which may interfere with the menu's accessible structure. Either keep the presentation default or document why callers must now opt in.

default role is removed from both ul and li tag, role will render only when explicitly provided by the parent

fix: updated structure for dropdown menu list item

a4c1927

ssherigar-c-eightfold and others added 2 commits March 31, 2026 19:22

Merge branch 'main' into allyant/dropdown

8b77786

fix: wrapped text in i18n

5f52e4c

ssherigar-c-eightfold changed the title ~~fix: updated structure for dropdown menu list item~~ fix(Dropdown): updated structure for dropdown menu list item Apr 1, 2026

Sannidhi Sherigar added 4 commits April 2, 2026 10:58

fix: updated the no result found text

152a856

chore: removed changes related to result announcement

ea8266d

chore: aria-haspopup will be updated based on prop

0354ecd

chore: updated snapshot

5e912e2

ychhabra-eightfold reviewed Apr 6, 2026

View reviewed changes

ychhabra-eightfold mentioned this pull request Apr 6, 2026

fix(Select): updated aria-live property for results announcement #1114

Merged

4 tasks

Sannidhi Sherigar and others added 8 commits April 7, 2026 09:29

chore: resolved claude comments

b7bef1b

Merge branch 'main' into allyant/dropdown

2a2ea1b

chore: ariaHaspopupValue condition updated

ac01e35

Merge branch 'main' into allyant/dropdown

9eb0c53

Merge branch 'main' into allyant/dropdown

76f4d92

chore: make role attribute conditional on parent input for list semantic

ff4ef93

chore: updated test case

9481a10

Merge branch 'main' into allyant/dropdown

8f3a243

ychhabra-eightfold reviewed Apr 13, 2026

View reviewed changes

chore: resolved claude comments

127df60

ychhabra-eightfold approved these changes Apr 17, 2026

View reviewed changes

ychhabra-eightfold merged commit 1943b74 into EightfoldAI:main Apr 17, 2026
3 checks passed

Conversation

ssherigar-c-eightfold commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY:

GITHUB ISSUE (Open Source Contributors)

JIRA TASK (Eightfold Employees Only):

CHANGE TYPE:

TEST COVERAGE:

TEST PLAN:

Uh oh!

codesandbox-ci bot commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ychhabra-eightfold left a comment

Choose a reason for hiding this comment

Regression Risk Analysis

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ssherigar-c-eightfold Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ssherigar-c-eightfold Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ssherigar-c-eightfold Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ychhabra-eightfold left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ssherigar-c-eightfold commented Mar 31, 2026 •

edited

Loading

codesandbox-ci bot commented Mar 31, 2026 •

edited

Loading

ssherigar-c-eightfold Apr 7, 2026 •

edited

Loading

ssherigar-c-eightfold Apr 7, 2026 •

edited

Loading

ssherigar-c-eightfold Apr 7, 2026 •

edited

Loading