Improve handling of aria-controls

.yarn/versions/794debf7.yml

@@ -0,0 +1,15 @@
+releases:
+  "@radix-ui/react-accordion": minor
+  "@radix-ui/react-alert-dialog": minor
+  "@radix-ui/react-collapsible": minor
+  "@radix-ui/react-context-menu": minor
+  "@radix-ui/react-dialog": minor
+  "@radix-ui/react-dropdown-menu": minor
+  "@radix-ui/react-menu": minor
+  "@radix-ui/react-menubar": minor
+  "@radix-ui/react-navigation-menu": minor
+  "@radix-ui/react-popover": minor
+  "@radix-ui/react-select": minor
+
+declined:
+  - primitives

packages/react/collapsible/src/Collapsible.test.tsx

@@ -48,6 +48,10 @@ describe('given a default Collapsible', () => {
       });
     });
   });
+
+  it('should not have an aria-controls attribute', () => {
+    expect(trigger.getAttribute('aria-controls')).toBeNull();
+  });
 });
 
 describe('given an open uncontrolled Collapsible', () => {
@@ -100,4 +104,11 @@ describe('given an open controlled Collapsible', () => {
       expect(content).toBeVisible();
     });
   });
+
+  it('should have a valid aria-controls attribute', () => {
+    const trigger = rendered.getByText(TRIGGER_TEXT);
+    const ariaControls = trigger.getAttribute('aria-controls');
+    expect(ariaControls).not.toBeNull();
+    expect(ariaControls).toBe(content.id);
+  });
 });

packages/react/collapsible/src/Collapsible.tsx

@@ -93,7 +93,7 @@ const CollapsibleTrigger = React.forwardRef<CollapsibleTriggerElement, Collapsib
     return (
       <Primitive.button
         type="button"
-        aria-controls={context.contentId}
+        aria-controls={context.open ? context.contentId : undefined}
         aria-expanded={context.open || false}
         data-state={getState(context.open)}
         data-disabled={context.disabled ? '' : undefined}

packages/react/dialog/src/Dialog.test.tsx

@@ -1,4 +1,4 @@
-import React from 'react';
+import React, { act } from 'react';
 import { axe } from 'jest-axe';
 import { render, fireEvent, RenderResult, cleanup } from '@testing-library/react';
 import * as Dialog from '@radix-ui/react-dialog';
@@ -134,4 +134,21 @@ describe('given a default Dialog', () => {
       });
     });
   });
+
+  describe("it's aria-controls", () => {
+    it('should not be defined when the dialog is closed', async () => {
+      expect(rendered.queryByText(TITLE_TEXT)).not.toBeInTheDocument();
+      expect(trigger.getAttribute('aria-controls')).toBeNull();
+    });
+
+    it('should be valid when the dialog is open', () => {
+      act(() => trigger.click());
+      const ariaControls = trigger.getAttribute('aria-controls');
+      expect(ariaControls).not.toBeNull();
+
+      const dialog = rendered.getByRole('dialog');
+      expect(dialog).toBeInTheDocument();
+      expect(dialog.id).toBe(ariaControls);
+    });
+  });
 });

packages/react/dialog/src/Dialog.tsx

@@ -104,7 +104,7 @@ const DialogTrigger = React.forwardRef<DialogTriggerElement, DialogTriggerProps>
         type="button"
         aria-haspopup="dialog"
         aria-expanded={context.open}
-        aria-controls={context.contentId}
+        aria-controls={context.open ? context.contentId : undefined}
         data-state={getState(context.open)}
         {...triggerProps}
         ref={composedTriggerRef}

packages/react/menu/src/Menu.tsx

@@ -1047,7 +1047,7 @@ const MenuSubTrigger = React.forwardRef<MenuSubTriggerElement, MenuSubTriggerPro
           id={subContext.triggerId}
           aria-haspopup="menu"
           aria-expanded={context.open}
-          aria-controls={subContext.contentId}
+          aria-controls={context.open ? subContext.contentId : undefined}
           data-state={getOpenState(context.open)}
           {...props}
           ref={composeRefs(forwardedRef, subContext.onTriggerChange)}

packages/react/navigation-menu/src/NavigationMenu.test.tsx

@@ -0,0 +1,45 @@
+import { act, render } from '@testing-library/react';
+import * as NavigationMenu from '.';
+
+const TRIGGER = 'Trigger';
+const CONTENT = 'Content';
+
+function NavigationMenuTest(): React.ReactElement {
+  return (
+    <NavigationMenu.Root>
+      <NavigationMenu.List>
+        <NavigationMenu.Item>
+          <NavigationMenu.Trigger>{TRIGGER}</NavigationMenu.Trigger>
+          <NavigationMenu.Content>{CONTENT}</NavigationMenu.Content>
+        </NavigationMenu.Item>
+      </NavigationMenu.List>
+    </NavigationMenu.Root>
+  );
+}
+
+describe('NavigationMenu', () => {
+  describe('aria-controls', () => {
+    it('should not be defined when the menu is closed', async () => {
+      const rendered = render(<NavigationMenuTest />);
+
+      expect(rendered.queryByText(CONTENT)).not.toBeInTheDocument();
+      const trigger = rendered.getByText(TRIGGER);
+      expect(trigger.getAttribute('aria-controls')).toBeNull();
+    });
+
+    it('should be valid when the menu is open', () => {
+      const rendered = render(<NavigationMenuTest />);
+
+      const trigger = rendered.getByText(TRIGGER);
+      act(() => trigger.click());
+      const content = rendered.queryByText(CONTENT);
+      expect(content).toBeVisible();
+
+      const ariaControls = trigger.getAttribute('aria-controls');
+      expect(ariaControls).not.toBeNull();
+      if (content !== null) {
+        expect(content.id).toBe(ariaControls);
+      }
+    });
+  });
+});

packages/react/navigation-menu/src/NavigationMenu.tsx

@@ -498,7 +498,7 @@ const NavigationMenuTrigger = React.forwardRef<
             data-disabled={disabled ? '' : undefined}
             data-state={getOpenState(open)}
             aria-expanded={open}
-            aria-controls={contentId}
+            aria-controls={open ? contentId : undefined}
             {...triggerProps}
             ref={composedRefs}
             onPointerEnter={composeEventHandlers(props.onPointerEnter, () => {

packages/react/popover/src/Popover.test.tsx

@@ -0,0 +1,39 @@
+import { act, render } from '@testing-library/react';
+import * as Popover from '.';
+
+const OPEN_TEXT = 'Open';
+const CONTENT_TEXT = 'Content';
+
+function PopoverTest(): React.ReactElement {
+  return (
+    <Popover.Root>
+      <Popover.Trigger>{OPEN_TEXT}</Popover.Trigger>
+      <Popover.Content>{CONTENT_TEXT}</Popover.Content>
+    </Popover.Root>
+  );
+}
+
+describe('Popover', () => {
+  describe('aria-controls', () => {
+    it('should not be defined when the popover is closed', async () => {
+      const rendered = render(<PopoverTest />);
+
+      expect(rendered.queryByText(CONTENT_TEXT)).not.toBeInTheDocument();
+      const trigger = rendered.getByText(OPEN_TEXT);
+      expect(trigger.getAttribute('aria-controls')).toBeNull();
+    });
+
+    it('should be valid when the popover is open', () => {
+      const rendered = render(<PopoverTest />);
+
+      const trigger = rendered.getByText(OPEN_TEXT);
+      act(() => trigger.click());
+      const ariaControls = trigger.getAttribute('aria-controls');
+      expect(ariaControls).not.toBeNull();
+
+      const dialog = rendered.getByRole('dialog');
+      expect(dialog).toBeInTheDocument();
+      expect(dialog.id).toBe(ariaControls);
+    });
+  });
+});

packages/react/popover/src/Popover.tsx

@@ -143,7 +143,7 @@ const PopoverTrigger = React.forwardRef<PopoverTriggerElement, PopoverTriggerPro
         type="button"
         aria-haspopup="dialog"
         aria-expanded={context.open}
-        aria-controls={context.contentId}
+        aria-controls={context.open ? context.contentId : undefined}
         data-state={getState(context.open)}
         {...triggerProps}
         ref={composedTriggerRef}

packages/react/select/src/Select.test.tsx

@@ -0,0 +1,48 @@
+import { act, render } from '@testing-library/react';
+import * as Select from '.';
+
+const VALUE_1 = 'Value 1';
+const VALUE_2 = 'Value 2';
+
+function SelectTest(): React.ReactElement {
+  return (
+    <Select.Root>
+      <Select.Trigger>
+        <Select.Value placeholder="Open the select" />
+        <Select.Portal>
+          <Select.Content>
+            <Select.Item value="value1">{VALUE_1}</Select.Item>
+            <Select.Item value="value2">{VALUE_2}</Select.Item>
+          </Select.Content>
+        </Select.Portal>
+      </Select.Trigger>
+    </Select.Root>
+  );
+}
+
+describe('Select', () => {
+  describe('aria-controls', () => {
+    it('should not be defined when the select is closed', async () => {
+      const rendered = render(<SelectTest />);
+
+      expect(rendered.queryByText(VALUE_1)).not.toBeInTheDocument();
+      const trigger = rendered.getByRole('combobox');
+      expect(trigger.getAttribute('aria-controls')).toBeNull();
+    });
+
+    it('should be valid when the select is open', () => {
+      const rendered = render(<SelectTest />);
+
+      const trigger = rendered.getByRole('combobox');
+      act(() => trigger.click());
+      expect(rendered.queryByText(VALUE_1)).toBeVisible();
+
+      const ariaControls = trigger.getAttribute('aria-controls');
+      expect(ariaControls).not.toBeNull();
+
+      const listbox = rendered.getByRole('listbox');
+      expect(listbox).toBeInTheDocument();
+      expect(listbox.id).toBe(ariaControls);
+    });
+  });
+});

packages/react/select/src/Select.tsx

@@ -253,7 +253,7 @@ const SelectTrigger = React.forwardRef<SelectTriggerElement, SelectTriggerProps>
         <Primitive.button
           type="button"
           role="combobox"
-          aria-controls={context.contentId}
+          aria-controls={context.open ? context.contentId : undefined}
           aria-expanded={context.open}
           aria-required={context.required}
           aria-autocomplete="none"