fix(ButtonGroup): keep arrow keys inside a member's open menu, plus the ButtonGroup audit

.changeset/button-group-layer-arrow-keys.md

@@ -0,0 +1,7 @@
+---
+'@astryxdesign/core': patch
+---
+
+[fix] ButtonGroup: arrow keys pressed inside a member's open menu stay with that menu. A DropdownMenu renders its menu inline inside the group, so ArrowLeft and ArrowRight used to bubble to the group and move focus onto a sibling button while the menu was still open. The group's `elevation` is also reflected as `data-elevation` now, so a theme can target it.
+
+@cixzhang

apps/storybook/rtl-audit/targets.json

@@ -1,4 +1,15 @@
 [
+  {
+    "component": "ButtonGroup",
+    "storyId": "core-buttongroup--horizontal",
+    "dims": [
+      "D2"
+    ],
+    "selectors": {
+      "prev": "[role=\"group\"] button:first-child",
+      "next": "[role=\"group\"] button:last-child"
+    }
+  },
   {
     "component": "Calendar",
     "storyId": "core-calendar--default",

apps/storybook/stories/ButtonGroup.stories.tsx

@@ -184,3 +184,55 @@ export const WithDropdownMenu: Story = {
     </ButtonGroup>
   ),
 };
+
+/**
+ * Disabled members: the whole group via `isDisabled`, and a single member
+ * disabling itself inside an enabled group.
+ */
+export const Disabled: Story = {
+  render: () => (
+    <div style={{display: 'flex', gap: 16, alignItems: 'center'}}>
+      <ButtonGroup label="Clipboard actions" isDisabled>
+        <Button label="Copy" />
+        <Button label="Cut" />
+        <Button label="Paste" />
+      </ButtonGroup>
+      <ButtonGroup label="History">
+        <Button label="Undo" />
+        <Button label="Redo" isDisabled />
+      </ButtonGroup>
+    </div>
+  ),
+};
+
+/**
+ * The connected buttons share one surface, so the shadow sits on the group and
+ * lifts them together.
+ */
+export const Elevation: Story = {
+  render: () => (
+    <div style={{display: 'flex', gap: 24, alignItems: 'center'}}>
+      {(['none', 'low', 'med', 'high'] as const).map(elevation => (
+        <ButtonGroup key={elevation} label={elevation} elevation={elevation}>
+          <Button label="Copy" />
+          <Button label="Paste" />
+        </ButtonGroup>
+      ))}
+    </div>
+  ),
+};
+
+/**
+ * Long labels in a narrow container. Members neither wrap nor truncate, so a
+ * group whose labels are wider than its container overflows it.
+ */
+export const LongLabels: Story = {
+  render: () => (
+    <div style={{width: 280}}>
+      <ButtonGroup label="Review actions">
+        <Button label="Approve and merge immediately" />
+        <Button label="Request changes" />
+      </ButtonGroup>
+    </div>
+  ),
+};

packages/core/src/ButtonGroup/ButtonGroup.doc.mjs

@@ -15,7 +15,7 @@ export const docs = {
   },
   theming: {
     targets: [
-      {className: 'astryx-button-group', visualProps: ['size', 'orientation']},
+      {className: 'astryx-button-group', visualProps: ['size', 'orientation', 'elevation']},
     ],
   },
   components: [
@@ -46,6 +46,8 @@ export const docs = {
       {guidance: false, description: "Don't mix wildly different actions. A Save button next to a Delete button in the same group is confusing."},
       {guidance: false, description: "Don't use ButtonGroup for navigation. Use SegmentedControl or TabList for switching between views."},
       {guidance: false, description: "Don't nest ButtonGroups. If you need multiple groups, place them side by side with a gap."},
+      {guidance: true, description: 'Name the group for what its buttons act on. The label is the group\u2019s accessible name and a screen reader reads it before each member.'},
+      {guidance: false, description: "Don't disable the group to show that an action is in flight. A disabled member drops focus, so a keyboard user loses their place; leave the group enabled and show progress on the button that started the work."},
     ],
     anatomy: [
       {name: 'Button', required: true, description: 'One or more Button or IconButton children that form the connected group.'},
@@ -84,6 +86,8 @@ export const docsZh = {
       {guidance: false, description: '不要混合差异很大的操作，将保存按钮和删除按钮放在同一组中会令人困惑。'},
       {guidance: false, description: '不要使用 ButtonGroup 进行导航，使用 SegmentedControl 或 TabList 切换视图。'},
       {guidance: false, description: '不要嵌套 ButtonGroup。如需多个组，请并排放置并留有间隔。'},
+      {guidance: true, description: '按钮组的标签应说明这些按钮作用于什么。该标签是按钮组的无障碍名称，屏幕阅读器会在每个成员之前朗读它。'},
+      {guidance: false, description: '不要用禁用整个按钮组来表示操作进行中。禁用的成员会失去焦点，键盘用户会丢失位置；请保持按钮组可用，并在发起操作的按钮上显示进度。'},
     ],
     anatomy: [
       {name: '按钮', required: true, description: '一个或多个 Button 或 IconButton 子元素，形成连接的组。'},
@@ -105,6 +109,8 @@ export const docsDense = {
       {guidance: false, description: "Don't mix unrelated actions in one group."},
       {guidance: false, description: "Don't use for navigation. Use SegmentedControl or TabList."},
       {guidance: false, description: "Don't nest ButtonGroups."},
+      {guidance: true, description: 'Label the group for what its buttons act on; it is the accessible name.'},
+      {guidance: false, description: "Don't disable the group for an in-flight action; a disabled member drops focus."},
     ],
   },
   components: [

packages/core/src/ButtonGroup/ButtonGroup.test.tsx

@@ -12,7 +12,8 @@
  */
 
 import {describe, it, expect} from 'vitest';
-import {render, screen} from '@testing-library/react';
+import {render, screen, fireEvent} from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
 import {transformSync} from '@babel/core';
 import stylexBabelPlugin from '@stylexjs/babel-plugin';
 import {readFileSync} from 'node:fs';
@@ -611,5 +612,188 @@ describe('ButtonGroup', () => {
         none.querySelector('[role="group"]')!.className,
       );
     });
+
+    it('exposes the elevation to a theme as a data attribute', () => {
+      render(
+        <ButtonGroup label="Actions" elevation="high">
+          <Button label="One" />
+        </ButtonGroup>,
+      );
+
+      expect(screen.getByRole('group')).toHaveAttribute(
+        'data-elevation',
+        'high',
+      );
+    });
+  });
+
+  // ===========================================================================
+  // Keyboard navigation
+  //
+  // The group wires useListFocus, so arrow keys move focus between members.
+  // The last two tests cover the boundary: a member can own a layer, whose
+  // keys belong to that layer, and the group itself can sit inside a layer,
+  // whose keys are still the group's own. Both must hold at once.
+  // ===========================================================================
+  describe('keyboard navigation', () => {
+    const clipboard = (
+      <ButtonGroup label="Actions">
+        <Button label="Copy" />
+        <Button label="Cut" />
+        <Button label="Paste" />
+      </ButtonGroup>
+    );
+
+    it('moves focus between members with the arrow keys, wrapping at the end', async () => {
+      const user = userEvent.setup();
+      render(clipboard);
+
+      screen.getByRole('button', {name: 'Copy'}).focus();
+
+      await user.keyboard('{ArrowRight}');
+      expect(screen.getByRole('button', {name: 'Cut'})).toHaveFocus();
+
+      await user.keyboard('{ArrowRight}{ArrowRight}');
+      expect(screen.getByRole('button', {name: 'Copy'})).toHaveFocus();
+
+      await user.keyboard('{ArrowLeft}');
+      expect(screen.getByRole('button', {name: 'Paste'})).toHaveFocus();
+    });
+
+    it('jumps to the first and last member with Home and End', async () => {
+      const user = userEvent.setup();
+      render(clipboard);
+
+      screen.getByRole('button', {name: 'Cut'}).focus();
+
+      await user.keyboard('{End}');
+      expect(screen.getByRole('button', {name: 'Paste'})).toHaveFocus();
+
+      await user.keyboard('{Home}');
+      expect(screen.getByRole('button', {name: 'Copy'})).toHaveFocus();
+    });
+
+    it('uses the vertical arrows when the group is vertical', async () => {
+      const user = userEvent.setup();
+      render(
+        <ButtonGroup label="Actions" orientation="vertical">
+          <Button label="Copy" />
+          <Button label="Cut" />
+        </ButtonGroup>,
+      );
+
+      screen.getByRole('button', {name: 'Copy'}).focus();
+
+      await user.keyboard('{ArrowDown}');
+      expect(screen.getByRole('button', {name: 'Cut'})).toHaveFocus();
+
+      await user.keyboard('{ArrowUp}');
+      expect(screen.getByRole('button', {name: 'Copy'})).toHaveFocus();
+    });
+
+    it('follows visual direction in RTL', async () => {
+      const user = userEvent.setup();
+      // dir sits on the group because jsdom resolves computed direction from
+      // the element's own attribute, not from an ancestor.
+      render(
+        <ButtonGroup label="Actions" dir="rtl">
+          <Button label="Copy" />
+          <Button label="Cut" />
+          <Button label="Paste" />
+        </ButtonGroup>,
+      );
+      screen.getByRole('button', {name: 'Copy'}).focus();
+
+      await user.keyboard('{ArrowLeft}');
+      expect(screen.getByRole('button', {name: 'Cut'})).toHaveFocus();
+
+      await user.keyboard('{ArrowRight}');
+      expect(screen.getByRole('button', {name: 'Copy'})).toHaveFocus();
+    });
+
+    it('skips a disabled member', async () => {
+      const user = userEvent.setup();
+      render(
+        <ButtonGroup label="Actions">
+          <Button label="Copy" />
+          <Button label="Cut" isDisabled />
+          <Button label="Paste" />
+        </ButtonGroup>,
+      );
+
+      screen.getByRole('button', {name: 'Copy'}).focus();
+
+      await user.keyboard('{ArrowRight}');
+      expect(screen.getByRole('button', {name: 'Paste'})).toHaveFocus();
+    });
+
+    it('leaves arrow keys to a member\u2019s open menu', async () => {
+      const user = userEvent.setup();
+      render(
+        <ButtonGroup label="Approve action">
+          <Button label="Allow once" />
+          <DropdownMenu
+            hasChevron={false}
+            button={{label: 'Allow options', isIconOnly: true}}
+            items={[{label: 'Allow for 30 minutes'}, {label: 'Always allow'}]}
+          />
+        </ButtonGroup>,
+      );
+
+      await user.click(screen.getByRole('button', {name: /Allow options/}));
+      const item = screen.getByRole('menuitem', {
+        name: 'Allow for 30 minutes',
+        hidden: true,
+      });
+      item.focus();
+
+      // Assert positively: the menu item keeps focus. `.not.toHaveFocus()` on
+      // the group's buttons would also pass if the group had thrown focus
+      // somewhere else entirely.
+      fireEvent.keyDown(item, {key: 'ArrowRight'});
+      expect(item).toHaveFocus();
+
+      fireEvent.keyDown(item, {key: 'ArrowLeft'});
+      expect(item).toHaveFocus();
+
+      // End belongs to the menu's own list focus, so it lands on the last menu
+      // item rather than the group's last button.
+      fireEvent.keyDown(item, {key: 'End'});
+      expect(
+        screen.getByRole('menuitem', {name: 'Always allow', hidden: true}),
+      ).toHaveFocus();
+    });
+
+    it('still moves between its own members when the group sits inside a popover', async () => {
+      const user = userEvent.setup();
+      // A group rendered inside a Popover, ContextMenu, HoverCard or Toast has
+      // a `[popover]` ancestor, because that is how useLayer mounts a layer.
+      // A bare `popover` div is the same DOM fact without the layer harness.
+      render(
+        <div popover="manual">
+          <ButtonGroup label="Actions">
+            <Button label="Copy" />
+            <Button label="Cut" />
+            <Button label="Paste" />
+          </ButtonGroup>
+        </div>,
+      );
+
+      // A closed popover is display:none in jsdom, so its contents read as
+      // inaccessible; focus and key handling still work on them.
+      const button = (name: string) =>
+        screen.getByRole('button', {name, hidden: true});
+
+      button('Copy').focus();
+
+      await user.keyboard('{ArrowRight}');
+      expect(button('Cut')).toHaveFocus();
+
+      await user.keyboard('{End}');
+      expect(button('Paste')).toHaveFocus();
+
+      await user.keyboard('{Home}');
+      expect(button('Copy')).toHaveFocus();
+    });
   });
 });

packages/core/src/ButtonGroup/ButtonGroup.tsx

@@ -172,6 +172,13 @@ export function ButtonGroup({
 
   const {listRef, handleKeyDown} = useListFocus<HTMLDivElement>({
     itemSelector: 'button, [tabindex="0"]',
+    // A member's layer renders inline inside the group (useLayer does not
+    // portal it), so a key pressed in an open DropdownMenu bubbles here. The
+    // boundary is the group's own root or any layer, whichever is nearer to
+    // the target. Naming the group's own root is what keeps a group rendered
+    // inside a Popover or Toast working: bailing on any [popover] ancestor
+    // instead would swallow every arrow key it owns.
+    boundarySelector: '[role="group"], [popover]',
     orientation,
   });
 
@@ -187,7 +194,7 @@ export function ButtonGroup({
           ref={mergeRefs(ref, listRef)}
           {...props}
           {...mergeProps(
-            themeProps('button-group', {size, orientation}),
+            themeProps('button-group', {size, orientation, elevation}),
             stylex.props(
               styles.group,
               orientation === 'vertical' && styles.vertical,