Enhancement: Port layout components to React - #18
Merged
ViewFromTheBox merged 10 commits intoMar 28, 2026
Merged
Conversation
Port the complete layout component category to React with TypeScript and DaisyUI/Tailwind CSS: Card, Modal, Tabs, Accordion, Collapse, Drawer, Dropdown, Divider, Stack, Grid, and Popover. Each component supports ref forwarding, full TypeScript prop types, controlled/uncontrolled state, DaisyUI color and size variants, and accessibility (ARIA attributes, keyboard navigation, focus management). Tabs supports vertical/vertical-right orientation with color and custom class props. Modal uses native <dialog> with focus restoration. Dropdown and Popover use DaisyUI CSS-based show/hide for reliability. Includes 131 unit tests across 11 test files (280 total passing). Closes #5
📝 WalkthroughWalkthroughAdds 11 new React layout components (Accordion, Card, Collapse, Divider, Drawer, Dropdown, Grid, Modal, Popover, Stack, Tabs) with TypeScript props, ref-forwarding, internal/controlled APIs, comprehensive Vitest+RTL tests, and an index that re-exports components and types. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
ViewFromTheBox
marked this pull request as ready for review
March 28, 2026 20:06
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react/src/__tests__/layout/Grid.test.tsx`:
- Around line 50-54: Add two tests to the existing Grid test suite that exercise
the single-axis fallback branches in the Grid component: one that renders <Grid
gapX={2}> and asserts the rendered element has class 'gap-x-2' and does NOT have
a 'gap-y-*' class, and another that renders <Grid gapY={4}> and asserts the
element has class 'gap-y-4' and does NOT have a 'gap-x-*' class; place these
near the existing 'applies directional gaps' test so the Grid component's gapX
and gapY-only logic (the branches referenced in the Grid component) are covered
and cannot regress.
In `@packages/react/src/components/layout/Accordion/Accordion.tsx`:
- Around line 73-89: The identity check child.type !== Collapse is fragile for
wrapped or HOC/styled variants; create a small type-guard helper (e.g.,
isCollapseElement) that returns true if child.type === Collapse OR (child.type
as any)?.displayName === Collapse.displayName OR a static marker like
(child.type as any)?.__isCollapse; then replace the condition inside items.map
(the isValidElement && child.type check) with isValidElement(child) &&
isCollapseElement(child) and cast to ReactElement<CollapseProps> as before
(collapseChild) so cloneElement still receives the same props handling.
In `@packages/react/src/components/layout/Card/Card.tsx`:
- Around line 4-29: The Card currently types props with CardProps extends
HTMLAttributes<HTMLDivElement> and always uses an HTMLDivElement ref, but when
the link prop is set the component renders an <a> so you must replace CardProps
with a discriminated union: define NonLinkCardProps (extends
HTMLAttributes<HTMLDivElement> with link?: undefined) and LinkCardProps (extends
AnchorHTMLAttributes<HTMLAnchorElement> with link: string) and export type
CardProps = NonLinkCardProps | LinkCardProps; update the component forwardRef
generic/ref typing to accept HTMLDivElement | HTMLAnchorElement (or use
overloaded forwards via the discriminant) so consumers get the correct ref and
anchor-specific props (target, rel, download) when link is provided; remove the
unsafe cast currently used when rendering the anchor and ensure prop spreading
uses the appropriate attribute type based on the presence of link.
In `@packages/react/src/components/layout/Collapse/Collapse.tsx`:
- Around line 84-102: The aria-expanded attribute is on the title div but must
be on the interactive control; update both input branches (the radio and the
checkbox) to include aria-expanded={isOpen} (and keep
aria-controls={`collapse-content-${autoId}`}, checked={isOpen},
onChange={handleToggle}, disabled={disabled}) and remove aria-expanded from the
div with className "collapse-title" (which should only render the title text).
This ensures the inputs (not the title div) expose the expanded state to
assistive technologies.
In `@packages/react/src/components/layout/Divider/Divider.tsx`:
- Around line 16-25: The colorMap is currently typed as Record<string,string>
which loses type safety against the component's DaisyColor prop; change
colorMap's type to Record<DaisyColor,string> (or a mapped type using the
DaisyColor union) and update any imports so DaisyColor is referenced, then
ensure all DaisyColor variants are present in colorMap (or use a partial with a
default fallback in the Divider component where colorMap is read) so the
compiler enforces coverage between the color prop and colorMap; update the
Divider component to import DaisyColor and use the stronger type for colorMap to
catch mismatches at compile time.
- Around line 54-60: The Divider component is adding positionMap[labelPosition]
even when there is no label or children; update the className construction in
Divider (where cn(...) is called) to include the position class only when a
label or children exists (e.g., guard positionMap[labelPosition] with (label ||
children) && labelPosition) or compute a positionClass variable first and pass
that to cn, ensuring positionMap[labelPosition] is not applied unless label or
children is present.
In `@packages/react/src/components/layout/Drawer/Drawer.tsx`:
- Around line 45-87: When the drawer opens you must move focus into the panel,
trap Tab/Shift+Tab inside it, and restore focus when it closes: in the existing
useEffect (and/or a new effect keyed on open) save document.activeElement into a
previousActiveElement, when open find the drawer panel element (the div with
classes "bg-base-100 text-base-content min-h-full w-80 p-4" or the element
containing {side}), focus the first focusable element (or the panel itself), and
add a keydown handler that intercepts Tab/Shift+Tab to keep focus within that
panel (cycle focus among focusable elements) while still allowing Escape (you
can keep handleKeyDown for Escape). On close (or effect cleanup) remove the
keydown listener, restore focus to previousActiveElement, and ensure aria-modal
on the element with role="dialog" reflects open; keep existing
handleOverlayClick behavior for persistent. Use unique symbols toggleId, ref,
useEffect, handleOverlayClick, and the drawer panel element to locate where to
implement these changes.
- Around line 64-85: The outer `.drawer` div currently receives spread props
(aria-label/aria-labelledby) so the element with role="dialog" (`<div
className="drawer-side">`) can't be named; update the Drawer component to pull
aria-label and aria-labelledby (and any other dialog-specific accessibility
props like aria-modal if being passed) out of the rest props and explicitly pass
them to the element with role="dialog" (the `drawer-side` div) instead of
letting them land on the outer `div` with className 'drawer'; ensure the outer
`div ref={ref}` no longer receives those aria props and that `drawer-side` keeps
role="dialog" and the correct aria-modal value.
In `@packages/react/src/components/layout/Dropdown/Dropdown.tsx`:
- Around line 167-179: The wrapper div around the trigger in Dropdown.tsx
introduces tabIndex={0} even when a focusable trigger prop (e.g., a button) is
provided, creating nested focusable elements; change the logic in the Dropdown
component so the wrapper only receives tabIndex and keyboard handlers when the
default trigger is rendered (i.e., trigger is null/undefined) or, alternatively,
use React.cloneElement to attach onClick/onKeyDown/aria attributes directly to
the provided trigger element (update the code paths that call handleTriggerClick
and handleTriggerKeyDown and ensure aria-controls={menuId},
aria-expanded={isOpen} and aria-haspopup are applied to the actual focusable
element).
- Around line 205-226: The onClick prop in DropdownItem is being unsafely cast;
update the DropdownItemProps type to declare onClick?:
React.MouseEventHandler<HTMLButtonElement> (or
React.MouseEventHandler<HTMLButtonElement> | undefined) so the prop is correctly
typed, then remove the cast in the DropdownItem component where onClick is
passed to the <button>; ensure the forwardRef generic and the component props
import/usage reference the updated DropdownItemProps so TypeScript understands
the handler signature without using "as unknown as".
In `@packages/react/src/components/layout/Modal/Modal.tsx`:
- Around line 109-133: When a title is provided, generate a stable id for the
heading and wire it into the dialog's aria-labelledby: inside the Modal
component (Modal.tsx) create a stable id (e.g. via React's useId or
useRef/useMemo) when the title prop exists, set that id on the rendered heading
element (the <h3> or title node) and add aria-labelledby={generatedId} to the
<dialog> element (in the same JSX that currently uses ref={setRefs} and
onClick={handleBackdropClick}); ensure you only add aria-labelledby when title
is present and preserve existing aria-label/other rest props.
- Around line 89-95: The cancel handler handleCancel currently only prevents
default when persistent is true, causing the dialog's native cancel to close the
DOM before React updates onClose; change handleCancel so it always calls
e.preventDefault() first (regardless of persistent) and then, if not persistent,
invoke onClose(); update any type assumptions around the Event parameter if
needed to ensure e.preventDefault() is callable in handleCancel.
In `@packages/react/src/components/layout/Popover/Popover.tsx`:
- Around line 110-140: The hover trigger currently lacks keyboard focus handling
and Escape dismissal outside the trigger; add onFocus and onBlur handlers
(paired with handleMouseEnter/handleMouseLeave logic using showDelay/hideDelay,
showTimer and hideTimer and setOpen) so triggerMode === 'hover' opens on focus
and closes on blur, and remove Escape-only-from-trigger logic from
handleKeyDown; instead create a component-level keydown listener (e.g., in a
useEffect) that, when isOpen is true, closes the popover on Escape (calling
setOpen(false)) so Escape works when focus is inside the popover or other
elements; keep handleClick and handleKeyDown behavior for triggerMode ===
'click' intact but stop skipping Escape handling globally.
In `@packages/react/src/components/layout/Tabs/Tabs.tsx`:
- Line 186: Remove the redundant runtime guard in the button onClick handler:
since the button already uses disabled={tab.disabled}, drop the `!tab.disabled
&&` check and call handleSelect directly (i.e., change the onClick to invoke
handleSelect(tab.name)). Update the button in Tabs.tsx that references
handleSelect and tab to rely on the disabled prop only and call
handleSelect(tab.name) unconditionally in the click handler.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 84bb5184-f193-48bf-8133-009f465a7ccc
📒 Files selected for processing (23)
packages/react/src/__tests__/layout/Accordion.test.tsxpackages/react/src/__tests__/layout/Card.test.tsxpackages/react/src/__tests__/layout/Collapse.test.tsxpackages/react/src/__tests__/layout/Divider.test.tsxpackages/react/src/__tests__/layout/Drawer.test.tsxpackages/react/src/__tests__/layout/Dropdown.test.tsxpackages/react/src/__tests__/layout/Grid.test.tsxpackages/react/src/__tests__/layout/Modal.test.tsxpackages/react/src/__tests__/layout/Popover.test.tsxpackages/react/src/__tests__/layout/Stack.test.tsxpackages/react/src/__tests__/layout/Tabs.test.tsxpackages/react/src/components/layout/Accordion/Accordion.tsxpackages/react/src/components/layout/Card/Card.tsxpackages/react/src/components/layout/Collapse/Collapse.tsxpackages/react/src/components/layout/Divider/Divider.tsxpackages/react/src/components/layout/Drawer/Drawer.tsxpackages/react/src/components/layout/Dropdown/Dropdown.tsxpackages/react/src/components/layout/Grid/Grid.tsxpackages/react/src/components/layout/Modal/Modal.tsxpackages/react/src/components/layout/Popover/Popover.tsxpackages/react/src/components/layout/Stack/Stack.tsxpackages/react/src/components/layout/Tabs/Tabs.tsxpackages/react/src/components/layout/index.ts
- Card: discriminated union props (CardDivProps | CardLinkProps) for correct ref/attribute types when link prop is present - Collapse: move aria-expanded from title div to the input elements where assistive tech expects it - Divider: type colorMap as Record<DaisyColor, string>, guard positionMap on label/children presence - Drawer: add focus trap (Tab/Shift+Tab cycling), focus first element on open, restore focus on close, forward aria-label/aria-labelledby to the dialog element - Dropdown: use cloneElement to attach aria/click/keyboard props directly to custom triggers (avoids nested focusable elements), properly type DropdownItem onClick as MouseEventHandler<HTMLButtonElement> - Modal: add aria-labelledby pointing to title heading, always preventDefault on cancel event to prevent DOM/React state desync - Popover: add onFocus/onBlur handlers for hover mode keyboard access, global Escape listener when open, type positionMap with Position type - Stack/Grid: replace dynamic template-string classes with static maps to prevent Tailwind CSS purging - Tabs: remove redundant !tab.disabled guard (button disabled prop already prevents clicks) - Accordion: displayName-based type guard for wrapped Collapse variants - Grid tests: add gapX-only and gapY-only fallback branch coverage - Collapse test: verify aria-expanded on input element
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
packages/react/src/components/layout/Popover/Popover.tsx (1)
144-148:⚠️ Potential issue | 🟡 MinorHonor
showDelayfor keyboard focus in hover mode.Focus-open currently bypasses the configured hover delay, so keyboard and pointer behavior diverge.
Suggested fix
const handleFocus = () => { if (triggerMode !== 'hover') return; clearTimers(); - setOpen(true); + if (showDelay > 0) { + showTimer.current = setTimeout(() => setOpen(true), showDelay); + } else { + setOpen(true); + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/components/layout/Popover/Popover.tsx` around lines 144 - 148, The focus handler currently opens immediately; change handleFocus so when triggerMode === 'hover' it clears timers and starts the same delayed-open logic used for mouse hover by scheduling setOpen(true) after the configured showDelay (use setTimeout and ensure the timer is stored/cleared by the existing clearTimers mechanism) instead of calling setOpen synchronously; reference handleFocus, triggerMode, showDelay, clearTimers and setOpen when implementing the delayed open.packages/react/src/components/layout/Drawer/Drawer.tsx (2)
46-48:⚠️ Potential issue | 🟡 MinorForward
aria-describedbyto the dialog as well.Because Line 125 still spreads
...restonto the outer.drawer,aria-describedbyattaches to the wrapper instead of the element withrole="dialog". Any consumer-supplied descriptive text will be skipped by assistive tech.🔧 Suggested fix
children, 'aria-label': ariaLabel, 'aria-labelledby': ariaLabelledBy, + 'aria-describedby': ariaDescribedBy, ...rest @@ role="dialog" aria-modal={open || undefined} aria-label={ariaLabel} aria-labelledby={ariaLabelledBy} + aria-describedby={ariaDescribedBy} >Also applies to: 125-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/components/layout/Drawer/Drawer.tsx` around lines 46 - 48, The Drawer component currently spreads ...rest onto the outer .drawer wrapper which causes aria-describedby to attach to the wrapper instead of the dialog; update the component to explicitly pull out aria-describedby (e.g., const { ariaDescribedBy, 'aria-describedby': ariaDescribedByAttr, ...rest } = props or read ariaDescribedBy/aria-describedby from props) and pass it to the element with role="dialog" (the dialog container rendered inside Drawer) alongside 'aria-label'/'aria-labelledby' while ensuring ...rest no longer contains aria-describedby so it isn't applied to the outer wrapper; adjust code around the render of the outer .drawer and the dialog element (references: Drawer component, props ariaLabel/ariaLabelledBy, the spread of ...rest and the element with role="dialog") accordingly.
25-31:⚠️ Potential issue | 🟠 MajorCompute drawer focus order from tabbable elements, not generic focusables.
Line 28 still includes controls that are not in the sequential tab order, e.g.
button/awithtabIndex={-1}and hidden inputs. Once one of those becomes the first or last candidate, the autofocus on Lines 63-70 and the wrap logic on Lines 97-108 stop matching the real tab order, soTabcan escape the drawer.🔧 Suggested fix
-function getFocusableElements(container: HTMLElement): HTMLElement[] { +function getTabbableElements(container: HTMLElement): HTMLElement[] { return Array.from( container.querySelectorAll<HTMLElement>( - 'a[href], button:not([disabled]), input:not([disabled]), select:not([disabled]), textarea:not([disabled]), [tabindex]:not([tabindex="-1"])', + 'a[href], button, input, select, textarea, [tabindex]', ), - ); + ).filter((el) => { + if (el.matches(':disabled')) return false; + if (el instanceof HTMLInputElement && el.type === 'hidden') return false; + if (el.closest('[hidden],[aria-hidden="true"]')) return false; + const style = window.getComputedStyle(el); + return el.tabIndex >= 0 && style.display !== 'none' && style.visibility !== 'hidden'; + }); } @@ - const focusable = getFocusableElements(panelRef.current); - if (focusable.length > 0) { - focusable[0].focus(); + const tabbable = getTabbableElements(panelRef.current); + if (tabbable.length > 0) { + tabbable[0].focus(); } else { panelRef.current.focus(); } @@ - const focusable = getFocusableElements(panelRef.current); - if (focusable.length === 0) { + const tabbable = getTabbableElements(panelRef.current); + if (tabbable.length === 0) { e.preventDefault(); return; } - const first = focusable[0]; - const last = focusable[focusable.length - 1]; + const first = tabbable[0]; + const last = tabbable[tabbable.length - 1]; + + if (!panelRef.current.contains(document.activeElement)) { + e.preventDefault(); + (e.shiftKey ? last : first).focus(); + return; + }Also applies to: 62-70, 90-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/components/layout/Drawer/Drawer.tsx` around lines 25 - 31, getFocusableElements currently returns elements by selector only and can include items not in the sequential tab order (e.g. elements with tabIndex = -1, hidden inputs), which breaks the drawer's autofocus and wrap logic; update getFocusableElements to first query the broad set and then filter out elements that are not tabbable by checking element.tabIndex >= 0 (use the DOM property, not the attribute), exclude input[type="hidden"], ensure the element is visible (e.g. offsetParent or getClientRects and computed style visibility/display), and skip elements with aria-hidden/inert so the returned list matches the actual tab order used by the browser; ensure the autofocus logic and wrap logic reference this filtered list from getFocusableElements (the same list used around the autofocus code and wrap handling).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react/src/components/layout/Accordion/Accordion.tsx`:
- Around line 53-66: The issue is that assigning a shared name (groupName) turns
the item into a radio so clicking an already-open panel won't fire change and
cannot close; update the rendering so Collapse (or the interactive input) only
receives the shared name when using multiple=false if you intentionally want
radio behavior — but here we want toggle-to-close, so remove or omit the name
prop for single-mode items (or ensure the input remains a checkbox) so clicks on
an open panel produce change events and allow handleToggle to close it; locate
references to groupName, handleToggle, and the Collapse/interactive input props
and adjust the name/type wiring accordingly to keep single-open semantics in
state while preserving checkbox semantics for the DOM input.
In `@packages/react/src/components/layout/Collapse/Collapse.tsx`:
- Around line 85-105: The radio/checkbox inputs in the Collapse component lack
an accessible name: update both input branches (the inputs rendered when name is
truthy and falsy) to reference the visible title by adding a unique id to the
title element (e.g., collapse-title-{autoId}) and setting aria-labelledby on the
inputs (or give the inputs an id and set aria-labelledby on the title) so the
assistive tech announces the control; ensure you modify the input elements and
the div with className "collapse-title" consistently using the existing autoId.
In `@packages/react/src/components/layout/Dropdown/Dropdown.tsx`:
- Around line 103-116: getMenuItems() is selecting elements with role="menuitem"
which in this markup is applied to non-focusable <li>, so focusItem() calls
.focus() on the <li> instead of the inner <button>; update getMenuItems (and any
other callers around menuRef usage) to return the actual focusable controls
inside each menu item (e.g., the inner <button> or the element that should
receive keyboard focus) rather than the list element, and ensure
focusItem(index) focuses that control and updates focusedIndex.current; after
fixing, add a regression test that opens the dropdown and verifies
ArrowDown/Home/End key presses move focus to the actual menu item buttons in the
expected order.
- Around line 158-168: The cloned trigger is overwriting any existing
onClick/onKeyDown handlers; update renderTrigger to compose handlers instead:
when cloning the supplied trigger (check isValidElement(trigger) and use
cloneElement), read the original handlers from (trigger.props.onClick and
trigger.props.onKeyDown) and create composed handlers that call the original
handler first (if present) and then call handleTriggerClick /
handleTriggerKeyDown (preserving event propagation and return values where
appropriate); keep aria props (triggerProps) and spread them into cloneElement
so existing behavior on the passed-in trigger is preserved while still invoking
the Dropdown's handlers.
In `@packages/react/src/components/layout/Popover/Popover.tsx`:
- Around line 199-200: The dropdown-content should not independently schedule
closing on its own mouseLeave because that can fire while the pointer is moving
back to the trigger; update the mouse handlers so that instead of attaching a
standalone onMouseLeave on the dropdown-content you either remove that binding
or change handleMouseLeave (and any dropdown-content-specific leave handler) to
check event.relatedTarget (or use contains(rootElement, relatedTarget)) and only
schedule close when the relatedTarget is outside the entire Popover root area;
reference the existing handleMouseEnter and handleMouseLeave handlers and the
dropdown-content element to locate and update the logic so leaving the content
toward the trigger does not trigger a close.
In `@packages/react/src/components/layout/Tabs/Tabs.tsx`:
- Around line 102-106: defaultActive/current selection logic can point at a
missing or disabled tab which leaves no tabbable item or panel; update the logic
around defaultActive, internalTab/setInternalTab, isControlled and current so
that both defaultTab and controlled activeTab are normalized to the first
selectable tab (tabs.find(t => !t.disabled)?.name) when they reference a missing
or disabled entry. Ensure defaultActive builds from a validated selectable name,
and when isControlled is true validate activeTab and fallback visually (and for
keyboard focus) to the first selectable tab; when uncontrolled initialize and
update internalTab via setInternalTab to always hold a selectable tab name. Use
the unique symbols defaultActive, internalTab, setInternalTab, isControlled,
current, activeTab, defaultTab and tabs to locate and change the logic
consistently wherever current is derived and used.
- Around line 144-146: The DOM selector and ID/ARIA construction use raw tab
names which can contain unsafe characters; update the logic to use the tab index
instead of tab.name: when querying within tabListRef use the index-based data
attribute (e.g., `[data-tab-index="${nextIndex}"]`) referencing enabledTabs and
nextIndex, and change creation of id and aria-controls in the element that uses
autoId and tab.name to use autoId plus the tab index (e.g.,
id={`tab-${autoId}-${index}`} and
aria-controls={`tabpanel-${autoId}-${index}`}); also update any places that
reference activeTabItem.name to use the active tab index for querySelector and
aria references so all selectors and IDs rely on numeric indices rather than raw
names (ensure data-tab attributes and any querySelector strings are updated
consistently).
---
Duplicate comments:
In `@packages/react/src/components/layout/Drawer/Drawer.tsx`:
- Around line 46-48: The Drawer component currently spreads ...rest onto the
outer .drawer wrapper which causes aria-describedby to attach to the wrapper
instead of the dialog; update the component to explicitly pull out
aria-describedby (e.g., const { ariaDescribedBy, 'aria-describedby':
ariaDescribedByAttr, ...rest } = props or read ariaDescribedBy/aria-describedby
from props) and pass it to the element with role="dialog" (the dialog container
rendered inside Drawer) alongside 'aria-label'/'aria-labelledby' while ensuring
...rest no longer contains aria-describedby so it isn't applied to the outer
wrapper; adjust code around the render of the outer .drawer and the dialog
element (references: Drawer component, props ariaLabel/ariaLabelledBy, the
spread of ...rest and the element with role="dialog") accordingly.
- Around line 25-31: getFocusableElements currently returns elements by selector
only and can include items not in the sequential tab order (e.g. elements with
tabIndex = -1, hidden inputs), which breaks the drawer's autofocus and wrap
logic; update getFocusableElements to first query the broad set and then filter
out elements that are not tabbable by checking element.tabIndex >= 0 (use the
DOM property, not the attribute), exclude input[type="hidden"], ensure the
element is visible (e.g. offsetParent or getClientRects and computed style
visibility/display), and skip elements with aria-hidden/inert so the returned
list matches the actual tab order used by the browser; ensure the autofocus
logic and wrap logic reference this filtered list from getFocusableElements (the
same list used around the autofocus code and wrap handling).
In `@packages/react/src/components/layout/Popover/Popover.tsx`:
- Around line 144-148: The focus handler currently opens immediately; change
handleFocus so when triggerMode === 'hover' it clears timers and starts the same
delayed-open logic used for mouse hover by scheduling setOpen(true) after the
configured showDelay (use setTimeout and ensure the timer is stored/cleared by
the existing clearTimers mechanism) instead of calling setOpen synchronously;
reference handleFocus, triggerMode, showDelay, clearTimers and setOpen when
implementing the delayed open.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0090fbb9-40ee-4ec2-9f39-db8a907cae0b
📒 Files selected for processing (11)
packages/react/src/__tests__/layout/Collapse.test.tsxpackages/react/src/__tests__/layout/Grid.test.tsxpackages/react/src/components/layout/Accordion/Accordion.tsxpackages/react/src/components/layout/Card/Card.tsxpackages/react/src/components/layout/Collapse/Collapse.tsxpackages/react/src/components/layout/Divider/Divider.tsxpackages/react/src/components/layout/Drawer/Drawer.tsxpackages/react/src/components/layout/Dropdown/Dropdown.tsxpackages/react/src/components/layout/Modal/Modal.tsxpackages/react/src/components/layout/Popover/Popover.tsxpackages/react/src/components/layout/Tabs/Tabs.tsx
- Accordion: remove radio name prop to preserve checkbox semantics so clicking an open panel in single mode fires onChange and closes it - Collapse: add aria-labelledby on inputs pointing to title element ID so assistive tech announces the control name - Dropdown: target inner <button> in getMenuItemButtons() instead of <li> so keyboard focus lands on the interactive element; compose original trigger handlers with dropdown handlers via cloneElement - Popover: check relatedTarget in mouseLeave to avoid closing when moving between trigger and content; use showDelay for focus handler; remove redundant handlers on content div - Tabs: validate defaultTab/activeTab against selectable tabs with fallback to first enabled tab; use index-based IDs and data attributes for querySelector and aria references - Drawer: filter getFocusableElements for tabIndex >= 0, exclude hidden inputs and aria-hidden/invisible elements; forward aria-describedby to dialog element
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (2)
packages/react/src/components/layout/Dropdown/Dropdown.tsx (2)
181-193: 🧹 Nitpick | 🔵 TrivialPrefer a native
<button>for the default trigger.The default trigger uses
<div tabIndex={0} role="button">wrapping a<span>, which doesn't provide native button semantics (e.g., form submission behavior, proper activation with Space/Enter in all browsers). Using a semantic<button>element is more accessible and requires less boilerplate.♻️ Proposed refactor
- return ( - <div - tabIndex={0} - role="button" - aria-haspopup="true" - aria-expanded={isOpen} - aria-controls={menuId} - onClick={handleTriggerClick} - onKeyDown={handleTriggerKeyDown} - > - <span className="btn btn-ghost btn-sm">{label}</span> - </div> - ); + return ( + <button + type="button" + className="btn btn-ghost btn-sm" + aria-haspopup="true" + aria-expanded={isOpen} + aria-controls={menuId} + onClick={handleTriggerClick} + onKeyDown={handleTriggerKeyDown} + > + {label} + </button> + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/components/layout/Dropdown/Dropdown.tsx` around lines 181 - 193, Replace the non-semantic div trigger with a native button: change the wrapper in Dropdown's render to a <button type="button"> that carries aria-haspopup, aria-expanded={isOpen}, aria-controls={menuId}, onClick={handleTriggerClick} and the visual classes (move btn btn-ghost btn-sm onto the button or keep the inner span minimal); remove tabIndex and role="button" (native button provides them) and you can drop handleTriggerKeyDown if it only existed to emulate button key behavior. Ensure label is rendered inside and references to handleTriggerClick, handleTriggerKeyDown, isOpen, menuId, and label are preserved.
240-255:⚠️ Potential issue | 🟠 MajorMove
role="menuitem"andaria-disabledto the focusable button element.Per WAI-ARIA menu pattern,
role="menuitem"must be on the element that receives focus and is interactive. Currently it's on the<li>container while the<button>inside is what actually receives focus and handles interaction. Thearia-disabledattribute should also follow the element with the menu role. The<li>should userole="none"to neutralize it. This affects screen reader announcements—readers may not properly identify the button as a menu item.🔧 Proposed fix
return ( <li ref={ref} - role="menuitem" - aria-disabled={disabled || undefined} + role="none" className={cn(disabled && 'disabled', className)} {...rest} > <button type="button" + role="menuitem" + aria-disabled={disabled || undefined} disabled={disabled} onClick={onClick} tabIndex={disabled ? -1 : 0} > {children} </button> </li> );Also update the selector in
getMenuItemButtons(line 107):const getMenuItemButtons = (): HTMLElement[] => { if (!menuRef.current) return []; return Array.from( menuRef.current.querySelectorAll<HTMLElement>( - '[role="menuitem"]:not([aria-disabled]) > button:not([disabled])', + 'button[role="menuitem"]:not([aria-disabled="true"]):not([disabled])', ), ); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/components/layout/Dropdown/Dropdown.tsx` around lines 240 - 255, In Dropdown.tsx move role="menuitem" and aria-disabled from the <li> to the interactive <button>: set the <li> to role="none" (or no role) and put role="menuitem" and aria-disabled={disabled || undefined} on the <button> that receives focus, keeping disabled and tabIndex behavior; then update the getMenuItemButtons selector (the getMenuItemButtons helper) so it queries the actual menu items (e.g., selecting button[role="menuitem"] or elements with role="menuitem") instead of targeting the <li> container.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react/src/components/layout/Accordion/Accordion.tsx`:
- Around line 79-94: The mapping uses the raw children indices for
open/defaultOpenIndices which will mismatch when non-Collapse children are
present; update Accordion behavior so indices refer only to Collapse elements or
document that current openIndices/defaultOpenIndices are positions in the full
children array. To fix in code, either filter items to collapseChildren first
(using isCollapseElement) and base currentOpen, defaultOpenIndices,
handleToggle, and the index passed to cloneElement on that filtered list (update
usages of items.map and currentOpen checks), or add/extend JSDoc on the
Accordion props (openIndices/defaultOpenIndices) to clearly state indices are
relative to the full children array; adjust references to isCollapseElement,
collapseChild (ReactElement<CollapseProps>), currentOpen, and handleToggle
accordingly.
- Around line 14-20: Change the isCollapseElement function to be a type
predicate so TypeScript can narrow the child without casts: update its signature
from "function isCollapseElement(child: ReactElement): boolean" to "function
isCollapseElement(child: ReactElement): child is ReactElement<any, typeof
Collapse>" and keep the same return expression that compares child.type to
Collapse or Collapse.displayName; this will allow removing the explicit cast at
the call site (the cast on line 82).
In `@packages/react/src/components/layout/Drawer/Drawer.tsx`:
- Around line 70-85: The scheduled requestAnimationFrame in the effect that
focuses the drawer (using panelRef and getFocusableElements) isn't canceled on
cleanup, so quick open->close can focus the hidden panel; fix by capturing the
requestAnimationFrame id when calling requestAnimationFrame and call
cancelAnimationFrame(id) in the effect cleanup before restoring focus to
previousActiveElement.current (ensure you still guard
previousActiveElement.current instanceof HTMLElement and keep the existing focus
restore logic).
- Around line 97-117: The Tab key handler in Drawer only wraps focus when
document.activeElement exactly equals the first/last focusable; update the logic
in the keydown handler (where e.key === 'Tab', using panelRef and
getFocusableElements) to also detect when document.activeElement is outside
panelRef.current or not found in the focusable list, and in that case treat it
as if it's before the first (for forward Tab) or after the last (for Shift+Tab)
so you always call e.preventDefault() and focus the appropriate boundary element
(first or last); this ensures the focus trap reclaims focus even when assistive
tech or programmatic focus escapes the panel.
In `@packages/react/src/components/layout/Dropdown/Dropdown.tsx`:
- Around line 142-144: The Escape key handler in Dropdown closes the menu via
setOpen(false) but does not restore focus to the trigger; update the key handler
in the Dropdown component (where e.key === 'Escape') to find and call focus() on
the trigger element before/after closing (e.g., via a stored triggerRef). Ensure
renderTrigger() captures and assigns that triggerRef when cloning or rendering
the default trigger element so the handler can reliably access
triggerRef.current.focus() to return focus to the trigger.
- Around line 170-177: The composed event handlers in onClick/onKeyDown call the
original handler and then always invoke the dropdown handlers, ignoring
event.defaultPrevented; change both compositions so they call
originalOnClick(...args) / originalOnKeyDown(e) first, then only call
handleTriggerClick() or handleTriggerKeyDown(e) if the event's defaultPrevented
is false. For onClick, detect the event from the first arg (treat args[0] as
Event and check (args[0] as Event).defaultPrevented) before calling
handleTriggerClick; for onKeyDown use the provided KeyboardEvent
e.defaultPrevented check before calling handleTriggerKeyDown. Ensure you still
call the original handlers first (originalOnClick, originalOnKeyDown) and
respect the prevented state.
In `@packages/react/src/components/layout/Popover/Popover.tsx`:
- Around line 14-29: PopoverProps is missing a persistent?: boolean prop so
consumers cannot opt out of automatic dismissal; add persistent?: boolean to the
PopoverProps interface and thread it through the Popover component where outside
mousedown and Escape handlers currently call close logic (e.g., in the functions
handling document mousedown and keydown/Escape and any internal call sites like
onOpenChange invocations or closePopover/handleClose methods) so that those
handlers skip closing when props.persistent is true, and ensure default behavior
remains unchanged when persistent is undefined/false.
- Around line 186-190: The spread "...rest" can override internal event handlers
and break popover behavior; update the Popover component to extract
onMouseEnter, onMouseLeave, onFocus, and onBlur from props (the rest object) and
compose them with the internal handlers (handleMouseEnter, handleMouseLeave,
handleFocus, handleBlur) instead of blindly spreading rest. Implement
composition so both handlers run (preserving event and calling external handlers
after or before the internal ones), then spread the remaining props; reference
the existing symbols handleMouseEnter, handleMouseLeave, handleFocus, handleBlur
and the "...rest" usage to locate where to extract and compose these handlers.
- Around line 179-183: The popover is being revealed by DaisyUI's
:focus-within/:hover CSS because the trigger wrapper is made focusable; update
the component so visibility is driven only by the React state and the
dropdown-open class: keep the className composition using positionMap and isOpen
(ensure 'dropdown-hover' only when triggerMode === 'hover'), but remove or make
the trigger wrapper non-focusable when using programmatic control (remove
role="button" and tabIndex={0} or conditionally set tabIndex={triggerMode ===
'hover' ? 0 : undefined} and omit role when not needed) so DaisyUI's focus/hover
selectors cannot open the dropdown independently of isOpen; ensure aria-expanded
and aria-hidden continue to reflect isOpen.
- Around line 168-198: The wrapper currently renders a synthetic button div
(tabIndex, role="button") and uses handleKeyDown which is disabled for hover
mode, causing nested interactive elements and a11y issues; update the Popover
rendering logic to detect if the provided trigger is a React element and
cloneElement the trigger to merge our handlers (onClick, onKeyDown, onFocus,
onBlur, onMouseEnter, onMouseLeave, ref via setRefs, aria-expanded) onto it
instead of wrapping everything in a div role="button", and only fall back to
rendering a non-interactive wrapper (or an explicit button) when the trigger is
not an element; also adjust handleKeyDown so click-mode keyboard activation
works (use setOpen(!isOpen)) and ensure no role="button" is applied in hover
mode where keyboard handler returns early.
In `@packages/react/src/components/layout/Tabs/Tabs.tsx`:
- Around line 160-164: The focus query uses nextIndex (an index into
enabledTabs) against DOM buttons that use the full tabs index, so arrow-key
focus can hit the wrong element; change the query to locate the button by the
original tab index or by matching the tab identity: after computing nextIndex
from enabledTabs, get the originalIndex from enabledTabs[nextIndex].index (or
look up the index in the full tabs array by enabledTabs[nextIndex].name) and use
that originalIndex in the tabListRef.current?.querySelector selector (i.e.,
replace `[data-tab-index="${nextIndex}"]` with
`[data-tab-index="${originalIndex}"]`) so tabListRef and handleSelect target the
same tab.
---
Duplicate comments:
In `@packages/react/src/components/layout/Dropdown/Dropdown.tsx`:
- Around line 181-193: Replace the non-semantic div trigger with a native
button: change the wrapper in Dropdown's render to a <button type="button"> that
carries aria-haspopup, aria-expanded={isOpen}, aria-controls={menuId},
onClick={handleTriggerClick} and the visual classes (move btn btn-ghost btn-sm
onto the button or keep the inner span minimal); remove tabIndex and
role="button" (native button provides them) and you can drop
handleTriggerKeyDown if it only existed to emulate button key behavior. Ensure
label is rendered inside and references to handleTriggerClick,
handleTriggerKeyDown, isOpen, menuId, and label are preserved.
- Around line 240-255: In Dropdown.tsx move role="menuitem" and aria-disabled
from the <li> to the interactive <button>: set the <li> to role="none" (or no
role) and put role="menuitem" and aria-disabled={disabled || undefined} on the
<button> that receives focus, keeping disabled and tabIndex behavior; then
update the getMenuItemButtons selector (the getMenuItemButtons helper) so it
queries the actual menu items (e.g., selecting button[role="menuitem"] or
elements with role="menuitem") instead of targeting the <li> container.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3b1cfaa8-f04c-41f0-869d-080c15658d91
📒 Files selected for processing (6)
packages/react/src/components/layout/Accordion/Accordion.tsxpackages/react/src/components/layout/Collapse/Collapse.tsxpackages/react/src/components/layout/Drawer/Drawer.tsxpackages/react/src/components/layout/Dropdown/Dropdown.tsxpackages/react/src/components/layout/Popover/Popover.tsxpackages/react/src/components/layout/Tabs/Tabs.tsx
- Accordion: use Collapse-relative indices (skip non-Collapse children); isCollapseElement is now a type predicate eliminating casts - Drawer: cancel requestAnimationFrame on cleanup to prevent stale focus; Tab trap reclaims focus when activeElement escapes panel - Dropdown: Escape restores focus to trigger via triggerRef; composed handlers respect defaultPrevented; default trigger is native <button>; role="menuitem" + aria-disabled moved from <li> to inner <button>, <li> gets role="none"; getMenuItemButtons selects button[role=menuitem] - Popover: add persistent prop to skip outside-click/Escape dismiss; extract and compose external onMouseEnter/Leave/Focus/Blur handlers from rest props; cloneElement trigger directly instead of wrapping in synthetic div[role=button]; remove dropdown-hover class to prevent DaisyUI :focus-within/:hover from overriding React state - Tabs: map enabledTabs index back to full tabs array index for querySelector focus targeting
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
packages/react/src/components/layout/Drawer/Drawer.tsx (1)
107-117:⚠️ Potential issue | 🟠 MajorForward
Tabcan still escape when focus is on the panel container.At Line 116, the forward-trap condition misses
activeIndex === -1. If focus is on the panel (tabIndex={-1}), forwardTabis not reclaimed and can move outside the drawer.🔧 Proposed fix
const first = focusable[0]; const last = focusable[focusable.length - 1]; const activeInPanel = panelRef.current.contains(document.activeElement); const activeIndex = focusable.indexOf(document.activeElement as HTMLElement); + const escapedOrUntracked = !activeInPanel || activeIndex < 0; if (e.shiftKey) { - if (!activeInPanel || activeIndex <= 0) { + if (escapedOrUntracked || activeIndex === 0) { e.preventDefault(); last.focus(); } } else { - if (!activeInPanel || activeIndex >= focusable.length - 1) { + if (escapedOrUntracked || activeIndex >= focusable.length - 1) { e.preventDefault(); first.focus(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/components/layout/Drawer/Drawer.tsx` around lines 107 - 117, The forward-Tab trap misses the case where document.activeElement is the panel container (activeIndex === -1), allowing Tab to escape; update the forward branch in the Drawer focus-trap (where panelRef, activeInPanel, activeIndex, focusable, last are used) to treat activeIndex === -1 as inside the panel by changing the condition to: if (!activeInPanel || activeIndex === -1 || activeIndex >= focusable.length - 1) { e.preventDefault(); last.focus(); } so that a forward Tab from the panel container is reclaimed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react/src/__tests__/layout/Popover.test.tsx`:
- Around line 91-100: Add two tests in
packages/react/src/__tests__/layout/Popover.test.tsx to cover dismissal when
persistent is false: render the Popover (use trigger={<button>Click</button>},
triggerMode="click", open and a vi.fn() onOpenChange), then (1) simulate Escape
via fireEvent.keyDown(document, { key: 'Escape' }) and assert onOpenChange was
called with false, and (2) simulate an outside click via
fireEvent.mouseDown(document.body) and assert onOpenChange was called with
false; locate the tests near the existing persistent test so they exercise the
same Popover behavior implemented in the Popover component's keyboard and
outside-click handlers.
In `@packages/react/src/components/layout/Accordion/Accordion.tsx`:
- Around line 90-94: The current cloneElement call overwrites a
consumer-provided Collapse onOpenChange (child.props.onOpenChange) with the
Accordion's handleToggle causing the consumer handler to be lost; update the
cloned props so onOpenChange composes both handlers: detect the original handler
on child.props.onOpenChange and return a new function that calls the original
(if present) and then calls handleToggle(idx, open) (or vice‑versa if order
matters), passing along the same boolean argument; keep the other props (key,
open, className) unchanged and ensure you use optional chaining when accessing
child.props.onOpenChange to avoid errors.
In `@packages/react/src/components/layout/Dropdown/Dropdown.tsx`:
- Around line 136-157: The roving-key logic in handleMenuKeyDown uses
getMenuItemButtons() and then performs modulo operations with items.length which
will break when items.length === 0; add an early guard in handleMenuKeyDown
(right after const items = getMenuItemButtons()) that returns immediately if
items.length === 0 to avoid NaN/invalid indices, keeping the rest of the logic
(focusedIndex, focusItem, closeFocusTrigger) unchanged.
- Around line 20-21: Change the trigger prop type from ReactNode to ReactElement
| null to match the implementation that only handles elements, stop overwriting
callers' refs when cloning the trigger, and obtain triggerRef from event
handler.currentTarget instead of injecting a ref. Specifically, update the prop
type declaration for trigger, remove the ref callback passed into
React.cloneElement (the cloneElement usage that currently clobbers refs), and in
the open/keyboard/click handlers capture and assign triggerRef =
(event.currentTarget as HTMLElement) so closeFocusTrigger() can use that stored
element for focus management.
In `@packages/react/src/components/layout/Popover/Popover.tsx`:
- Around line 257-259: The class "z-1" used on the dropdown container in Popover
(the div with className "dropdown-content ...") is invalid for Tailwind/DaisyUI;
update the class list to use a valid z-index utility such as z-[1] (for
z-index:1) or one of the built-ins (z-0, z-10, z-20, etc.) so the element stacks
correctly; make the same replacement in the analogous dropdown container in
Dropdown (the occurrence around the Dropdown component at the other file) to
keep behavior consistent.
In `@packages/react/src/components/layout/Tabs/Tabs.tsx`:
- Around line 60-75: The variantMap and colorMap use weak Record<string,string>
typings; change them to be exhaustive by typing variantMap as
Record<TabsProps['variant'], string> and colorMap as Record<DaisyColor, string>
(import DaisyColor and ensure TabsProps is in scope) so TypeScript will error on
missing or extra keys; update the maps to include all members of
TabsProps['variant'] and DaisyColor and run the build to fix any missing
mappings.
- Around line 121-123: Normalize selection for both controlled and uncontrolled
modes: when computing current (use the existing const current = ...), validate
the candidate tab with isSelectableTab(tabs, candidate) and fallback to
firstSelectableTab(tabs) so that uncontrolled internalTab is also normalized
when tabs change; likewise apply the same validation where selection is computed
at the other occurrences (the blocks around the earlier 171-173 and 197
references) and update handleSelect to ignore or remap attempts to select a
non-selectable id by validating the requested id with isSelectableTab(tabs, id)
and choosing firstSelectableTab(tabs) instead. Ensure you update the branches
that use isControlled, activeTab, internalTab, current, and handleSelect so both
controlled and uncontrolled flows never keep an invalid selection.
---
Duplicate comments:
In `@packages/react/src/components/layout/Drawer/Drawer.tsx`:
- Around line 107-117: The forward-Tab trap misses the case where
document.activeElement is the panel container (activeIndex === -1), allowing Tab
to escape; update the forward branch in the Drawer focus-trap (where panelRef,
activeInPanel, activeIndex, focusable, last are used) to treat activeIndex ===
-1 as inside the panel by changing the condition to: if (!activeInPanel ||
activeIndex === -1 || activeIndex >= focusable.length - 1) { e.preventDefault();
last.focus(); } so that a forward Tab from the panel container is reclaimed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4a9a2a81-ded4-4fe1-b7fe-0194d017db4a
📒 Files selected for processing (6)
packages/react/src/__tests__/layout/Popover.test.tsxpackages/react/src/components/layout/Accordion/Accordion.tsxpackages/react/src/components/layout/Drawer/Drawer.tsxpackages/react/src/components/layout/Dropdown/Dropdown.tsxpackages/react/src/components/layout/Popover/Popover.tsxpackages/react/src/components/layout/Tabs/Tabs.tsx
- Popover: add Escape and outside-click dismissal tests; fix z-1 → z-[1] for valid Tailwind z-index utility - Accordion: compose consumer onOpenChange with handleToggle instead of overwriting it - Dropdown: early return in handleMenuKeyDown when items empty; capture triggerRef via event.currentTarget instead of injecting ref; narrow trigger prop to ReactElement | null; fix z-1 → z-[1] - Tabs: type variantMap as Record<Variant, string> and colorMap as Record<DaisyColor, string> for exhaustive compile-time checks; normalize uncontrolled internalTab when tabs change; guard handleSelect to only accept selectable tab names - Drawer: fix forward-Tab trap when activeElement is the panel container itself (activeIndex === -1)
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
packages/react/src/components/layout/Popover/Popover.tsx (2)
242-259:⚠️ Potential issue | 🟠 MajorThe dropdown can still become visible while
isOpenis false.At Lines 245-258, the root keeps DaisyUI’s
dropdownbehavior, so any focusable trigger (for example a native<button>) can still satisfy DaisyUI’s:focus-withinselector and reveal.dropdown-contentwithoutdropdown-open. That leavesaria-expandedandaria-hiddenout of sync with the actual UI. Force the closed state withhidden={!isOpen}or another state-driven visibility override so React remains the only source of truth.Suggested fix
<div className="dropdown-content z-[1] rounded-box border border-base-300 bg-base-100 p-4 shadow-lg" aria-hidden={!isOpen} + hidden={!isOpen} >In DaisyUI's dropdown component, does `.dropdown:not(.dropdown-hover):focus-within .dropdown-content` reveal the content independently of the `dropdown-open` class?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/components/layout/Popover/Popover.tsx` around lines 242 - 259, The DaisyUI dropdown can be revealed via :focus-within even when isOpen is false, so update the Popover root div rendered by the component that uses setRefs/renderTrigger/isOpen to force controlled visibility: add a state-driven visibility override (e.g., add a hidden attribute or a style/class that sets display:none when isOpen is false) on the same div that currently spreads {...rest} so the UI cannot be shown via CSS focus rules; ensure aria-expanded on the trigger and aria-hidden on the content remain driven by isOpen and keep the existing event handlers (handleMouseEnter/handleMouseLeave/handleFocus/handleBlur) intact.
224-239:⚠️ Potential issue | 🟠 MajorHover-mode text triggers are still mouse-only.
At Line 238, the non-element hover fallback is a plain
<span>. Because it cannot receive focus, the container’sonFocus/onBlurlogic never runs, sotrigger="Help"has no keyboard path to open the popover. Render a focusable fallback in hover mode too.Suggested fix
- return <span aria-expanded={isOpen}>{trigger}</span>; + return ( + <span tabIndex={0} aria-expanded={isOpen}> + {trigger} + </span> + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/components/layout/Popover/Popover.tsx` around lines 224 - 239, The fallback for non-element triggers currently returns a plain <span> which is not focusable in hover triggerMode, so keyboard focus events never fire; update the non-element fallback inside the render trigger logic (the branch using triggerMode and isOpen) to render a focusable element when triggerMode !== 'click' (for example a <button type="button"> or a <span tabIndex={0} role="button">) and attach the existing handlers (aria-expanded={isOpen}, onKeyDown={handleKeyDown}, and any onFocus/onBlur or onClick as appropriate) so the component's onFocus/onBlur logic and keyboard activation work for trigger="Help".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react/src/components/layout/Accordion/Accordion.tsx`:
- Around line 56-73: Normalize and dedupe currentOpen before computing next
state: derive a cleanCurrent (e.g., Array.from(new Set(currentOpen)).filter(i =>
i >= 0 && i < maxIndex)) where maxIndex is the current number of collapsible
children (use children.length or the prop that represents item count), then use
cleanCurrent in handleToggle instead of currentOpen so you never append
duplicates or emit out‑of‑range indices; update both the multiple and single
branches, call setInternalOpen(next) with the normalized next, and pass the
normalized next into onOpenChange.
In `@packages/react/src/components/layout/Drawer/Drawer.tsx`:
- Around line 25-37: Update getFocusableElements to exclude elements that are
inert (have the inert attribute or are inside an inert ancestor) and to include
contenteditable elements as focusable; specifically, expand the initial selector
in getFocusableElements to also match [contenteditable] and then augment the
filter to skip any element where el.closest('[inert]') (or checking
el.hasAttribute('inert')) is true, and continue to keep the existing tabIndex,
aria-hidden, offsetParent/position checks to ensure consistent behavior.
- Around line 1-9: The import list in Drawer.tsx includes an unused symbol
useCallback; remove useCallback from the named imports (the import line
containing forwardRef, useEffect, useRef, useCallback, useId, ...) and run the
linter/TS check to ensure no other references to useCallback remain in the
Drawer component.
- Around line 146-153: The drawer-side element in the Drawer component is always
present and should be hidden from AT when closed; update the element (the JSX
with className "drawer-side" inside the Drawer component) to add an aria-hidden
attribute that is true when open is false (e.g., aria-hidden={!open} or
aria-hidden={open ? undefined : true}) so the side panel is not exposed to
assistive technologies while closed, keeping existing aria-modal/label
attributes unchanged.
In `@packages/react/src/components/layout/Dropdown/Dropdown.tsx`:
- Around line 43-44: The hover prop currently only toggles CSS and does not
update the component open state or fire ARIA callbacks; change the Dropdown
component to drive hover via JS: when hover is true attach mouseEnter/mouseLeave
(or pointerEnter/pointerLeave) handlers on the trigger/menu elements that call
the same internal open-state setter used by click (e.g., the setIsOpen or
onOpenChange path so isOpen/aria-expanded are updated and onOpenChange is
emitted), and ensure those handlers mirror the logic used at the click-open
sites (respecting controlled vs uncontrolled modes and any open prop) so both
visual hover and programmatic/ARIA state remain synchronized (apply the same fix
where hover is referenced around lines ~215-219).
- Around line 73-84: setOpen is calling onOpenChange on every invocation even
when the visibility state doesn't change; update setOpen to compute the current
effective open state (use the controlled prop `open` when `isControlled` is
true, otherwise use `internalOpen`/the internal state) and only call
onOpenChange when next !== currentEffectiveOpen. Keep the existing behavior of
updating `setInternalOpen(next)` when uncontrolled and resetting
`focusedIndex.current = -1` when closing, but guard the onOpenChange invocation
so it runs only on real transitions.
- Around line 134-160: The menu keydown handler handleMenuKeyDown should also
handle the 'Tab' key by closing the dropdown without preventing default so focus
can move naturally; inside handleMenuKeyDown (near calls to getMenuItemButtons,
focusItem, and closeFocusTrigger) add a branch for e.key === 'Tab' that calls
closeFocusTrigger() and returns (do not call e.preventDefault()) to avoid
leaving the menu open when users tab away.
In `@packages/react/src/components/layout/Popover/Popover.tsx`:
- Around line 100-108: The setOpen callback in Popover.tsx is calling
onOpenChange unconditionally, causing duplicate notifications; update setOpen
(the useCallback that currently references isControlled, setInternalOpen and
onOpenChange) to short-circuit and return early when next === isOpen so no
update or onOpenChange is invoked if the open state is unchanged; keep existing
behavior of setting internal state when uncontrolled (setInternalOpen(next)) and
still call onOpenChange only when the state actually transitions.
In `@packages/react/src/components/layout/Tabs/Tabs.tsx`:
- Around line 117-135: The uncontrolled Tab state (internalTab) can become stale
when the tabs prop changes; ensure internalTab is synchronized to the validated
current value whenever tabs or isControlled change by adding a useEffect that,
when not isControlled, computes the validated name (using isSelectableTab(tabs,
internalTab) ? internalTab : firstSelectableTab(tabs) or simply reusing current)
and calls setInternalTab with it; reference internalTab, setInternalTab,
current, isControlled, tabs, isSelectableTab, and firstSelectableTab in the
effect and import useEffect from React.
---
Duplicate comments:
In `@packages/react/src/components/layout/Popover/Popover.tsx`:
- Around line 242-259: The DaisyUI dropdown can be revealed via :focus-within
even when isOpen is false, so update the Popover root div rendered by the
component that uses setRefs/renderTrigger/isOpen to force controlled visibility:
add a state-driven visibility override (e.g., add a hidden attribute or a
style/class that sets display:none when isOpen is false) on the same div that
currently spreads {...rest} so the UI cannot be shown via CSS focus rules;
ensure aria-expanded on the trigger and aria-hidden on the content remain driven
by isOpen and keep the existing event handlers
(handleMouseEnter/handleMouseLeave/handleFocus/handleBlur) intact.
- Around line 224-239: The fallback for non-element triggers currently returns a
plain <span> which is not focusable in hover triggerMode, so keyboard focus
events never fire; update the non-element fallback inside the render trigger
logic (the branch using triggerMode and isOpen) to render a focusable element
when triggerMode !== 'click' (for example a <button type="button"> or a <span
tabIndex={0} role="button">) and attach the existing handlers
(aria-expanded={isOpen}, onKeyDown={handleKeyDown}, and any onFocus/onBlur or
onClick as appropriate) so the component's onFocus/onBlur logic and keyboard
activation work for trigger="Help".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ab47cf39-c7db-4a25-b15d-ffc7e291483a
📒 Files selected for processing (6)
packages/react/src/__tests__/layout/Popover.test.tsxpackages/react/src/components/layout/Accordion/Accordion.tsxpackages/react/src/components/layout/Drawer/Drawer.tsxpackages/react/src/components/layout/Dropdown/Dropdown.tsxpackages/react/src/components/layout/Popover/Popover.tsxpackages/react/src/components/layout/Tabs/Tabs.tsx
- Accordion: normalize/dedupe currentOpen before state computation,
filter out-of-range indices against Collapse child count
- Drawer: remove unused useCallback import; add contenteditable and
inert support to getFocusableElements; add aria-hidden={!open} on
drawer-side so AT ignores closed panel
- Dropdown: guard setOpen to skip when state unchanged (prevents
duplicate onOpenChange); add JS hover handlers (mouseEnter/Leave)
so hover prop drives React state + ARIA; add Tab key handler in
menu to close without preventing default
- Popover: guard setOpen against duplicate calls; add inline
visibility override on content to prevent DaisyUI :focus-within
from revealing closed content; make hover-mode non-element fallback
focusable (span with tabIndex/role)
- Tabs: add useEffect to sync internalTab when tabs prop changes
and current selection becomes invalid
Member
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
packages/react/src/components/layout/Dropdown/Dropdown.tsx (1)
229-234:⚠️ Potential issue | 🟠 Major
dropdown-hovercan still bypass state/ARIA alignment in controlled usage.Line 233 keeps CSS-driven hover opening (
dropdown-hover) alongside JS state management. This can still create a visual-open path not fully governed byisOpen/aria-expandedin controlled mode.In DaisyUI dropdown behavior, does the `dropdown-hover` class open the menu via CSS independently of the `dropdown-open` class/state?🔧 Proposed fix
className={cn( 'dropdown', end && 'dropdown-end', top && 'dropdown-top', - hover && 'dropdown-hover', isOpen && 'dropdown-open', className, )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/components/layout/Dropdown/Dropdown.tsx` around lines 229 - 234, The Dropdown component currently always adds the 'dropdown-hover' class when hover is true, which can let CSS open the menu independently of JS-controlled state (isOpen/aria-expanded); change the class computation so 'dropdown-hover' is only included when hover is true AND the component is uncontrolled (i.e., no external isOpen prop is provided) — use the existing props/flags (hover and isOpen) in the cn call to gate 'dropdown-hover' (e.g., include 'dropdown-hover' only when hover && typeof isOpen === "undefined"), and ensure aria-expanded continues to reflect isOpen so visual state and ARIA remain aligned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react/src/__tests__/layout/Popover.test.tsx`:
- Around line 113-122: Add a new unit test alongside the existing "does not
close when persistent" test to verify Escape is blocked when persistent=true:
render <Popover ... open persistent onOpenChange={onOpenChange}>, dispatch a
keyDown with key 'Escape' on document (using fireEvent.keyDown(document, { key:
'Escape' })), and assert onOpenChange was not called; reference the Popover
component and the existing test name to place the new test in
packages/react/src/__tests__/layout/Popover.test.tsx.
In `@packages/react/src/components/layout/Accordion/Accordion.tsx`:
- Around line 57-58: The component doesn't enforce the single-open invariant
when multiple is false: ensure cleanOpen (and any logic that derives currentOpen
from openIndices or internalOpen) filters/normalizes inputs so that when
multiple === false only a single index is returned (prefer the first valid
index); update the logic around isControlled/currentOpen/cleanOpen to collapse
arrays to a single index when multiple is false and handle both controlled
(openIndices) and uncontrolled (internalOpen/defaultOpenIndices) cases so the
component never renders multiple panels open in single mode.
In `@packages/react/src/components/layout/Dropdown/Dropdown.tsx`:
- Around line 38-49: The component spreads consumer props (...rest) after
internal mouse handlers, allowing consumers to override
onMouseEnter/onMouseLeave and break the hover open/close logic; update the
Dropdown render so consumer handlers are composed rather than overwritten: when
rendering the root element (where onMouseEnter/onMouseLeave are defined)
merge/compose handlers by reading rest.onMouseEnter and rest.onMouseLeave,
calling the internal hover handlers first (or last as desired) and then invoking
the consumer handlers with the same event, and ensure you keep using the
internal onOpenChange/open logic (references: Dropdown props, ...rest,
onMouseEnter/onMouseLeave, hover, open, onOpenChange) so hover-driven open/close
continues to work while still honoring consumer callbacks.
In `@packages/react/src/components/layout/Popover/Popover.tsx`:
- Around line 138-150: The Escape key handler in the useEffect (handleEscape
inside the Popover component) should stop propagation so Escape doesn't close
parent dismissible components; update handleEscape (which currently checks e.key
=== 'Escape' and calls setOpen(false)) to also call e.stopPropagation() (after
calling setOpen(false) or immediately) to prevent bubbling, keeping the
dependency list ([isOpen, persistent, setOpen]) unchanged.
- Around line 240-244: The fallback span in Popover's render (where trigger is
non-element) uses tabIndex={0} and role="button" but has no activation handlers;
update the Popover component to either remove role="button" (and optionally
tabIndex) for hover-only mode or add activation handlers that mirror focus
behavior: attach onClick and onKeyDown (handling Enter/Space) to the fallback
span so it opens the popover the same way focus does; locate the fallback span
in the Popover component (the JSX that returns <span tabIndex={0} role="button"
...>) and implement the chosen fix so keyboard and screen-reader users get
expected activation behavior.
In `@packages/react/src/components/layout/Tabs/Tabs.tsx`:
- Around line 128-134: The effect in Tabs.tsx that syncs internalTab uses the
tabs array in its dependency list (useEffect([... tabs, isControlled,
internalTab])) which means callers passing an inline tabs array can trigger the
effect on every render; add a short stability note to the Tabs component
docs/props describing that the tabs prop should be stable (memoized) to avoid
unnecessary re-runs — reference the useEffect behavior and helpers
isSelectableTab, firstSelectableTab, and internalTab so consumers know why they
should memoize the tabs array or derive it outside render.
---
Duplicate comments:
In `@packages/react/src/components/layout/Dropdown/Dropdown.tsx`:
- Around line 229-234: The Dropdown component currently always adds the
'dropdown-hover' class when hover is true, which can let CSS open the menu
independently of JS-controlled state (isOpen/aria-expanded); change the class
computation so 'dropdown-hover' is only included when hover is true AND the
component is uncontrolled (i.e., no external isOpen prop is provided) — use the
existing props/flags (hover and isOpen) in the cn call to gate 'dropdown-hover'
(e.g., include 'dropdown-hover' only when hover && typeof isOpen ===
"undefined"), and ensure aria-expanded continues to reflect isOpen so visual
state and ARIA remain aligned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a58596da-5bd6-4932-970f-b9d7444dfec7
📒 Files selected for processing (6)
packages/react/src/__tests__/layout/Popover.test.tsxpackages/react/src/components/layout/Accordion/Accordion.tsxpackages/react/src/components/layout/Drawer/Drawer.tsxpackages/react/src/components/layout/Dropdown/Dropdown.tsxpackages/react/src/components/layout/Popover/Popover.tsxpackages/react/src/components/layout/Tabs/Tabs.tsx
- Popover: add persistent-blocks-Escape test; stopPropagation on Escape to prevent parent dismissals; remove role="button" from hover-mode fallback span (focus-driven, not activation-driven) - Accordion: enforce single-open invariant — cleanOpen.slice(0,1) when multiple is false - Dropdown: extract and compose consumer onMouseEnter/onMouseLeave with internal hover handlers; gate dropdown-hover CSS class on uncontrolled mode only so controlled state isn't bypassed - Tabs: add JSDoc stability note on tabs prop recommending useMemo
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
packages/react/src/components/layout/Dropdown/Dropdown.tsx (1)
145-146:⚠️ Potential issue | 🟠 MajorDon't refocus the trigger on
Tab.Line 146 calls
closeFocusTrigger(), so the browser computes the next focus target from the trigger instead of from the active menu item. That breaks both forwardTaband reverseShift+Taborder.Suggested patch
if (e.key === 'Tab') { - closeFocusTrigger(); + setOpen(false); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/components/layout/Dropdown/Dropdown.tsx` around lines 145 - 146, The handler in Dropdown.tsx calls closeFocusTrigger() when e.key === 'Tab', which forces the browser to compute the next focus from the trigger instead of the currently active menu item and breaks Tab/Shift+Tab navigation; update the keydown logic in the component's keyboard handler so that closeFocusTrigger() is NOT invoked for 'Tab' (and 'Shift+Tab')—i.e., early-return or skip calling closeFocusTrigger() when e.key === 'Tab' (or when e.key === 'Tab' && e.shiftKey) and keep existing behavior for other keys, ensuring focus moves naturally from the active menu item.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react/src/__tests__/layout/Popover.test.tsx`:
- Around line 52-60: The current test in Popover.test.tsx only asserts mouse
click activation; add keyboard activation tests for click-mode by rendering
<Popover trigger={<button>Click me</button>} triggerMode="click"> and simulating
Enter and Space key events on the trigger (use fireEvent.keyDown / keyPress or
userEvent) via screen.getByText('Click me'), then assert container.firstChild
has the 'dropdown-open' class after each key press; add two test cases (one for
Enter, one for Space) alongside the existing click test to cover the keyboard
path for the Popover component.
In `@packages/react/src/components/layout/Accordion/Accordion.tsx`:
- Around line 56-73: handleToggle references cleanOpen which is declared later,
which is confusing for readers; move the derivation of cleanOpen so it appears
before the handleToggle function. Specifically, locate the cleanOpen variable
(derived from internalOpen/openIndices/multiple) and hoist its declaration above
the handleToggle function that uses it (while keeping isControlled, currentOpen
and internalOpen/setInternalOpen logic intact), so handleToggle can reference
cleanOpen without forward-looking declarations.
In `@packages/react/src/components/layout/Dropdown/Dropdown.tsx`:
- Around line 90-93: closeFocusTrigger currently assumes triggerRef.current is
set by the custom trigger click/key handlers but that ref is null when menu is
opened via hover or a controlled open prop; update the component to capture and
persist the trigger element whenever the trigger DOM node is mounted (e.g.,
assign a stable triggerElementRef from the trigger render/root node or a ref
callback used by the custom trigger) so closeFocusTrigger can always call focus
on that saved element; ensure this change is applied where triggerRef is used
(closeFocusTrigger, the hover handlers, and the custom trigger assignment spots
referenced by triggerRef) and add a regression test covering custom-trigger +
hover/controlled open + Escape to verify focus restoration.
- Around line 120-126: The arrow-key navigation should derive the starting index
from the actual focused element rather than relying solely on
focusedIndex.current; update the navigation logic (in handlers that call
focusItem, e.g., ArrowDown/ArrowUp handling near focusItem and
getMenuItemButtons) to check document.activeElement (or the focused button from
getMenuItemButtons()) when focusedIndex.current === -1, compute the correct
current index from that element, set focusedIndex.current to it, then proceed to
compute the next/previous index and call focusItem(nextIndex). Also update
focusItem/getMenuItemButtons usage to ensure focusedIndex.current is always
synchronized after programmatic focus, and add a regression test that opens the
menu via click or tabs into a menu item and then sends ArrowDown/ArrowUp to
verify the correct next item is focused.
- Around line 128-141: When handling the Escape key in both handleTriggerKeyDown
and handleMenuKeyDown, stop the event from bubbling after consuming it so parent
dismissables don't also react; specifically, call e.stopPropagation() (in
addition to e.preventDefault() where already used) before invoking
closeFocusTrigger() in handleMenuKeyDown and likewise in handleTriggerKeyDown’s
Escape branch so that closeFocusTrigger() only affects the current dropdown
layer.
In `@packages/react/src/components/layout/Popover/Popover.tsx`:
- Around line 1-15: The code references React.MutableRefObject (causing a TS
error because the React namespace isn't imported) — fix it by adding a named
import for MutableRefObject from 'react' alongside the existing named imports
and update any usage to use the imported MutableRefObject type; look for
references to React.MutableRefObject in this file (e.g., in the Popover
component's ref types) and replace them with the imported MutableRefObject so
the file compiles under jsx: "react-jsx".
---
Duplicate comments:
In `@packages/react/src/components/layout/Dropdown/Dropdown.tsx`:
- Around line 145-146: The handler in Dropdown.tsx calls closeFocusTrigger()
when e.key === 'Tab', which forces the browser to compute the next focus from
the trigger instead of the currently active menu item and breaks Tab/Shift+Tab
navigation; update the keydown logic in the component's keyboard handler so that
closeFocusTrigger() is NOT invoked for 'Tab' (and 'Shift+Tab')—i.e.,
early-return or skip calling closeFocusTrigger() when e.key === 'Tab' (or when
e.key === 'Tab' && e.shiftKey) and keep existing behavior for other keys,
ensuring focus moves naturally from the active menu item.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9ee7a423-f975-40e9-a3be-ab45a06c6040
📒 Files selected for processing (5)
packages/react/src/__tests__/layout/Popover.test.tsxpackages/react/src/components/layout/Accordion/Accordion.tsxpackages/react/src/components/layout/Dropdown/Dropdown.tsxpackages/react/src/components/layout/Popover/Popover.tsxpackages/react/src/components/layout/Tabs/Tabs.tsx
Comment on lines
+90
to
+93
| const closeFocusTrigger = useCallback(() => { | ||
| setOpen(false); | ||
| triggerRef.current?.focus(); | ||
| }, [setOpen]); |
There was a problem hiding this comment.
Custom triggers still miss focus restoration in hover/controlled opens.
For custom triggers, triggerRef.current is only assigned in Lines 197-206. If the menu opens via hover or via a controlled open update, closeFocusTrigger() at Lines 90-93 runs with null and cannot restore focus to the trigger.
Capture the trigger element independently of trigger click/key handling, and add a regression test for custom-trigger + hover/controlled open + Escape.
Also applies to: 175-176, 187-211
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react/src/components/layout/Dropdown/Dropdown.tsx` around lines 90 -
93, closeFocusTrigger currently assumes triggerRef.current is set by the custom
trigger click/key handlers but that ref is null when menu is opened via hover or
a controlled open prop; update the component to capture and persist the trigger
element whenever the trigger DOM node is mounted (e.g., assign a stable
triggerElementRef from the trigger render/root node or a ref callback used by
the custom trigger) so closeFocusTrigger can always call focus on that saved
element; ensure this change is applied where triggerRef is used
(closeFocusTrigger, the hover handlers, and the custom trigger assignment spots
referenced by triggerRef) and add a regression test covering custom-trigger +
hover/controlled open + Escape to verify focus restoration.
Comment on lines
+120
to
+126
| const focusItem = (index: number) => { | ||
| const items = getMenuItemButtons(); | ||
| if (items[index]) { | ||
| items[index].focus(); | ||
| focusedIndex.current = index; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Derive arrow-key navigation from the actually focused item.
focusedIndex.current is only updated by focusItem(). After tabbing or clicking into a menu item, it can still be -1, so the first ArrowDown/ArrowUp resolves the wrong next index.
Suggested patch
const items = getMenuItemButtons();
if (items.length === 0) return;
+ const activeIndex = items.findIndex((item) => item === document.activeElement);
+ if (activeIndex !== -1) {
+ focusedIndex.current = activeIndex;
+ }
if (e.key === 'ArrowDown') {
e.preventDefault();
const next = (focusedIndex.current + 1) % items.length;Please add a regression test for click-open/tab-into-item followed by ArrowDown/ArrowUp.
Also applies to: 150-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react/src/components/layout/Dropdown/Dropdown.tsx` around lines 120
- 126, The arrow-key navigation should derive the starting index from the actual
focused element rather than relying solely on focusedIndex.current; update the
navigation logic (in handlers that call focusItem, e.g., ArrowDown/ArrowUp
handling near focusItem and getMenuItemButtons) to check document.activeElement
(or the focused button from getMenuItemButtons()) when focusedIndex.current ===
-1, compute the correct current index from that element, set
focusedIndex.current to it, then proceed to compute the next/previous index and
call focusItem(nextIndex). Also update focusItem/getMenuItemButtons usage to
ensure focusedIndex.current is always synchronized after programmatic focus, and
add a regression test that opens the menu via click or tabs into a menu item and
then sends ArrowDown/ArrowUp to verify the correct next item is focused.
- Popover: add Enter/Space keyboard activation tests for click mode; import MutableRefObject to avoid React namespace reference; stopPropagation on Escape to prevent parent dismissals - Accordion: hoist cleanOpen derivation above handleToggle for clearer reading order - Dropdown: import MutableRefObject; Tab key just closes without refocusing trigger (natural focus flow); Escape stopPropagation on both trigger and menu handlers; sync focusedIndex from document.activeElement when -1; closeFocusTrigger falls back to containerRef querySelector for hover/controlled scenarios
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react/src/components/layout/Accordion/Accordion.tsx`:
- Around line 88-93: The accordion wrapper in the Accordion component should
include an ARIA grouping role to improve screen-reader context; update the root
div returned in Accordion (the element with ref={ref}, className using join and
className, and {...rest}) to include role="group" (or role="presentation" if you
prefer) so the container explicitly communicates grouping semantics while
preserving existing props; ensure you add the role attribute on the same element
that spreads {...rest} and does not override any individual Collapse ARIA
attributes.
In `@packages/react/src/components/layout/Popover/Popover.tsx`:
- Around line 208-210: The trigger currently sets 'aria-expanded' but not
'aria-controls', and the content panel lacks a stable id; generate a stable id
(e.g., using useId or a small id generator) and assign it to the content div
(the Popover content render block) and then add 'aria-controls': contentId to
the trigger props object (the const props: Record<string, unknown> = {
'aria-expanded': isOpen, }) and to every other trigger rendering path (the
alternate render branches that build trigger props) so the trigger points to the
content's id and the content element receives that id for proper programmatic
association.
- Around line 205-225: The renderTrigger branch treats any React element as safe
to clone, but non-interactive intrinsic elements (e.g., 'div'/'span') and
Fragments won't be focusable or accept injected props; update renderTrigger to
detect these cases before cloneElement: if triggerEl.type is React.Fragment or
an intrinsic string element that is not natively focusable, wrap the trigger in
a focusable wrapper (button for click-mode or span with tabIndex=0 for
hover-mode) or add tabIndex=0 when safe, and preserve existing
tabIndex/handlers; otherwise, when cloning via cloneElement keep injecting
aria-expanded, onClick and onKeyDown and call handleClick/handleKeyDown as
before. Ensure you reference renderTrigger, trigger, triggerMode, triggerEl,
cloneElement, handleClick and handleKeyDown when locating where to add these
checks and wrappers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5d100ebd-f722-47a6-84ca-a20635a5a82e
📒 Files selected for processing (4)
packages/react/src/__tests__/layout/Popover.test.tsxpackages/react/src/components/layout/Accordion/Accordion.tsxpackages/react/src/components/layout/Dropdown/Dropdown.tsxpackages/react/src/components/layout/Popover/Popover.tsx
- Accordion: add role="group" on wrapper for screen-reader context - Popover: generate stable contentId via useId and wire aria-controls on all trigger paths pointing to the content div id; detect non-focusable intrinsic elements (div, span, etc.) and inject tabIndex=0 when cloning to ensure keyboard reachability
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/react/src/components/layout/Popover/Popover.tsx (1)
208-220:⚠️ Potential issue | 🟠 MajorUnsupported trigger elements still slip through
renderTrigger().
isFocusableElement()only guards a small denylist and treatsReact.Fragmentas safe to clone. That leaves triggers like<img />,<label />,<li />, or<a>withouthrefunreachable from keyboard, and a fragment drops the injected DOM props entirely in click mode. Please switch this branch to an allowlist of genuinely focusable intrinsics and explicitly wrap or reject fragments beforecloneElement.Does React.Fragment accept arbitrary DOM props passed via React.cloneElement, and which native HTML elements are focusable by default without a non-negative tabIndex?Also applies to: 223-248
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/components/layout/Popover/Popover.tsx` around lines 208 - 220, The current isFocusableElement/NON_FOCUSABLE_INTRINSICS approach is a denylist that still lets non-focusable intrinsics (e.g., img, label, li, a without href) and React.Fragment slip through, causing injected props to be lost or elements to be unreachable by keyboard; update isFocusableElement (used by renderTrigger and before any React.cloneElement calls) to use an allowlist of genuinely focusable intrinsic tags (e.g., button, input, select, textarea, a with href, area with href, iframe, object) and check for a non-negative tabIndex where appropriate, and explicitly detect React.Fragment and either wrap its children in a focusable container or reject it with a clear error/ fallback so cloneElement never attempts to apply DOM props to a fragment. Ensure you reference and modify isFocusableElement, NON_FOCUSABLE_INTRINSICS (replace with a FOCUSABLE_INTRINSICS allowlist), and the renderTrigger/cloneElement control flow so fragments are handled before cloning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react/src/components/layout/Popover/Popover.tsx`:
- Around line 142-155: The Escape handler in Popover's useEffect currently only
calls setOpen(false) and stops propagation; update it to also restore focus to
the popover trigger when closing (mirror Dropdown.closeFocusTrigger()). Locate
the handleEscape function inside the useEffect for isOpen/persistent and, when
e.key === 'Escape' and you close via setOpen(false), call the existing
closeFocusTrigger() helper or implement the same focus-restoration logic used by
Dropdown to focus the trigger element before/after closing, ensuring it runs
only for non-persistent popovers.
---
Duplicate comments:
In `@packages/react/src/components/layout/Popover/Popover.tsx`:
- Around line 208-220: The current isFocusableElement/NON_FOCUSABLE_INTRINSICS
approach is a denylist that still lets non-focusable intrinsics (e.g., img,
label, li, a without href) and React.Fragment slip through, causing injected
props to be lost or elements to be unreachable by keyboard; update
isFocusableElement (used by renderTrigger and before any React.cloneElement
calls) to use an allowlist of genuinely focusable intrinsic tags (e.g., button,
input, select, textarea, a with href, area with href, iframe, object) and check
for a non-negative tabIndex where appropriate, and explicitly detect
React.Fragment and either wrap its children in a focusable container or reject
it with a clear error/ fallback so cloneElement never attempts to apply DOM
props to a fragment. Ensure you reference and modify isFocusableElement,
NON_FOCUSABLE_INTRINSICS (replace with a FOCUSABLE_INTRINSICS allowlist), and
the renderTrigger/cloneElement control flow so fragments are handled before
cloning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b69d51ee-af9e-4470-84d6-7a4a2607200c
📒 Files selected for processing (2)
packages/react/src/components/layout/Accordion/Accordion.tsxpackages/react/src/components/layout/Popover/Popover.tsx
- Popover: restore focus to trigger on Escape dismiss via containerRef querySelector; replace denylist with allowlist of natively focusable intrinsics (FOCUSABLE_INTRINSICS); detect and wrap React.Fragment triggers in a focusable container; extract wrapInFocusable helper for consistent fallback rendering
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react/src/components/layout/Popover/Popover.tsx`:
- Around line 214-216: FOCUSABLE_INTRINSICS is currently defined inside the
Popover component and gets recreated each render; hoist it to module scope by
moving the const FOCUSABLE_INTRINSICS = new Set([...]) out of the Popover
function (top of the file) so it’s instantiated once, and update any references
inside the Popover component to use the module-scoped FOCUSABLE_INTRINSICS.
- Around line 252-276: Add aria-haspopup: 'true' to the trigger props when
appropriate: inside the block that builds props for the trigger element (where
triggerEl, props, isOpen, contentId are set and triggerMode is checked) set
props['aria-haspopup'] = 'true' for click-mode triggers so assistive tech knows
the trigger opens a popup; also apply the same attribute to the fallback button
rendered later (the fallback button logic around the element returned after
cloneElement) to keep accessibility consistent. Ensure this change is added
alongside the existing aria-expanded/aria-controls logic and does not alter
existing handlers (origClick/origKeyDown, handleClick, handleKeyDown).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0d129c2c-b4a3-4f21-9caa-da564c447c09
📒 Files selected for processing (1)
packages/react/src/components/layout/Popover/Popover.tsx
Comment on lines
+214
to
+216
| const FOCUSABLE_INTRINSICS = new Set([ | ||
| 'button', 'input', 'select', 'textarea', 'a', 'area', 'iframe', 'object', | ||
| ]); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Move FOCUSABLE_INTRINSICS outside the component to avoid recreation on every render.
This Set is recreated on every render, which is wasteful. Since it's a static constant, hoist it to module scope.
♻️ Suggested refactor
+const FOCUSABLE_INTRINSICS = new Set([
+ 'button', 'input', 'select', 'textarea', 'a', 'area', 'iframe', 'object',
+]);
+
/**
* Positioned floating content triggered by hover or click.
*/
export const Popover = forwardRef<HTMLDivElement, PopoverProps>(
(
{
// ...props
},
ref,
) => {
- const FOCUSABLE_INTRINSICS = new Set([
- 'button', 'input', 'select', 'textarea', 'a', 'area', 'iframe', 'object',
- ]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const FOCUSABLE_INTRINSICS = new Set([ | |
| 'button', 'input', 'select', 'textarea', 'a', 'area', 'iframe', 'object', | |
| ]); | |
| const FOCUSABLE_INTRINSICS = new Set([ | |
| 'button', 'input', 'select', 'textarea', 'a', 'area', 'iframe', 'object', | |
| ]); | |
| /** | |
| * Positioned floating content triggered by hover or click. | |
| */ | |
| export const Popover = forwardRef<HTMLDivElement, PopoverProps>( | |
| ( | |
| { | |
| // ...props | |
| }, | |
| ref, | |
| ) => { | |
| // ... rest of component implementation |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react/src/components/layout/Popover/Popover.tsx` around lines 214 -
216, FOCUSABLE_INTRINSICS is currently defined inside the Popover component and
gets recreated each render; hoist it to module scope by moving the const
FOCUSABLE_INTRINSICS = new Set([...]) out of the Popover function (top of the
file) so it’s instantiated once, and update any references inside the Popover
component to use the module-scoped FOCUSABLE_INTRINSICS.
Comment on lines
+252
to
+276
| const triggerEl = trigger as ReactElement<Record<string, unknown>>; | ||
| const props: Record<string, unknown> = { | ||
| 'aria-expanded': isOpen, | ||
| 'aria-controls': contentId, | ||
| }; | ||
|
|
||
| // Ensure non-focusable intrinsic elements get tabIndex | ||
| if (!isFocusableElement(triggerEl)) { | ||
| props.tabIndex = 0; | ||
| } | ||
|
|
||
| if (triggerMode === 'click') { | ||
| const origClick = triggerEl.props.onClick as ((...a: unknown[]) => void) | undefined; | ||
| const origKeyDown = triggerEl.props.onKeyDown as ((...a: unknown[]) => void) | undefined; | ||
| props.onClick = (...args: unknown[]) => { | ||
| origClick?.(...args); | ||
| if (!(args[0] as Event)?.defaultPrevented) handleClick(); | ||
| }; | ||
| props.onKeyDown = (e: KeyboardEvent) => { | ||
| origKeyDown?.(e); | ||
| if (!e.defaultPrevented) handleKeyDown(e); | ||
| }; | ||
| } | ||
|
|
||
| return cloneElement(triggerEl, props); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding aria-haspopup for accessibility consistency with Dropdown.
The Dropdown component (see context snippet) consistently applies aria-haspopup: 'true' to triggers, which helps screen reader users understand that activating this element reveals additional content. Popover currently omits this attribute.
For click mode triggers, adding aria-haspopup would improve the accessibility experience by signaling the presence of a popup. For hover mode, it may be less critical since the interaction model differs.
♻️ Optional enhancement
const props: Record<string, unknown> = {
'aria-expanded': isOpen,
'aria-controls': contentId,
};
+
+ if (triggerMode === 'click') {
+ props['aria-haspopup'] = 'true';
+ }Similarly update the fallback button at line 282:
<button
type="button"
+ aria-haspopup="true"
aria-expanded={isOpen}
aria-controls={contentId}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react/src/components/layout/Popover/Popover.tsx` around lines 252 -
276, Add aria-haspopup: 'true' to the trigger props when appropriate: inside the
block that builds props for the trigger element (where triggerEl, props, isOpen,
contentId are set and triggerMode is checked) set props['aria-haspopup'] =
'true' for click-mode triggers so assistive tech knows the trigger opens a
popup; also apply the same attribute to the fallback button rendered later (the
fallback button logic around the element returned after cloneElement) to keep
accessibility consistent. Ensure this change is added alongside the existing
aria-expanded/aria-controls logic and does not alter existing handlers
(origClick/origKeyDown, handleClick, handleKeyDown).
github-project-automation
Bot
moved this from In Review
to Done
in ArtisanPack UI Overview
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Port all 11 layout components from the Livewire package to React with TypeScript and DaisyUI/Tailwind CSS: Card, Modal, Tabs, Accordion, Collapse, Drawer, Dropdown, Divider, Stack, Grid, and Popover.
Closes: #5
Type of Change
Related Issue
Issue: #5
Motivation and Context
The React component library needs layout components ported from the Livewire package to provide structural building blocks for application UIs. This is a core component category required for the v1.0 milestone.
Changes Made
<dialog>with focus restoration, escape/backdrop dismiss, persistent mode, glass effect, bottom positioning<button>layout/index.tsto export all components and typesHow Has This Been Tested?
Testing Environment:
Tests Performed:
/packages/react/layout-componentsAccessibility Tests Run
Details:
role="tablist",role="tab",role="tabpanel",aria-selected,aria-controls,aria-labelledby,aria-orientationaria-modal="true", close button witharia-label="Close", escape key dismiss, focus restorationaria-haspopup,aria-expanded,role="menu",role="menuitem",aria-disabled, full keyboard navaria-expandedon title,aria-controlson inputrole="dialog",aria-modal, overlay witharia-label="Close drawer"aria-expanded,aria-hiddenon content, keyboard activation (Enter/Space)role="separator",aria-orientationTests Added
Test details:
131 new tests across 11 files: Divider (8), Stack (9), Grid (9), Card (15), Collapse (13), Accordion (8), Tabs (21), Modal (13), Drawer (8), Dropdown (16), Popover (10)
Documentation
Documentation details:
All components have JSDoc comments on the exported component and interface. Wiki/README updates will follow in a separate PR.
Pre-Submission Checklist
Summary by CodeRabbit