Repository navigation
fix(web): screen readers can reach sidebar row actions #13962
+289
−126
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -173,7 +173,6 @@ import { | |
| resolveAdjacentThreadId, | ||
| resolveSidebarDropTarget, | ||
| resolveSidebarDropVerb, | ||
| type SidebarDropVerb, | ||
| resolveSidebarThreadStatus, | ||
| searchSidebarThreads, | ||
|
|
@@ -505,8 +504,8 @@ function SnoozeMenuButton(props: { | |
| // Subset of useSortable applied to a thread row's root <li>. Listeners go | ||
| // on the whole row (no dedicated handle): the pointer sensor's distance | ||
| // constraint keeps plain clicks working, and we skip dnd-kit's aria | ||
| // attributes since there is no keyboard sensor and the row already carries | ||
| // its own primary button. | ||
| type SortableThreadRowBag = Pick< | ||
| ReturnType<typeof useSortable>, | ||
| "listeners" | "setNodeRef" | "transform" | "transition" | "isDragging" | ||
|
|
@@ -536,6 +535,44 @@ function SortableThreadRow(props: { | |
| const draftSurfaceClassName = "bg-warning/4 hover:bg-warning/8"; | ||
| const draftPenClassName = "size-3 shrink-0 text-warning-foreground"; | ||
|
|
||
| // The row's one control for opening it, named by the row's title. It fills | ||
| // the row behind the visible content, which gives screen reader cursors and | ||
| // voice control a row-sized target, and its activation bubbles to the row | ||
| // surface's click handler like any other click on the row. Secondary actions | ||
| // must stay its siblings, never its children: WebKit drops everything inside a | ||
| // button from the accessibility tree, and NVDA's browse mode hides the | ||
| // contents of a named button. The row surface needs `isolate` so the negative | ||
| // z-index keeps this above the surface's background. | ||
| const SIDEBAR_ROW_SECONDARY_CONTROL_SELECTOR = "button:not([data-sidebar-row-primary]), a, input"; | ||
|
|
||
| // Row surface mousedown handler. A press on the row's content lands on | ||
| // elements that cannot take focus, so hand focus to the primary action, the | ||
| // way pressing a button focuses it. The keyboard and the context menu key then | ||
| // pick up from this row. | ||
| function focusSidebarRowPrimaryAction(event: ReactMouseEvent<HTMLElement>) { | ||
| if ((event.target as HTMLElement).closest(SIDEBAR_ROW_SECONDARY_CONTROL_SELECTOR)) return; | ||
| const primaryAction = event.currentTarget.querySelector<HTMLElement>( | ||
| "[data-sidebar-row-primary]", | ||
| ); | ||
| if (!primaryAction) return; | ||
| event.preventDefault(); | ||
| primaryAction.focus({ preventScroll: true }); | ||
| } | ||
|
|
||
| function SidebarRowPrimaryAction(props: { name: string; isActive: boolean; isBusy?: boolean }) { | ||
| return ( | ||
| <button | ||
| type="button" | ||
| data-sidebar-row-primary | ||
| aria-current={props.isActive ? "page" : undefined} | ||
| aria-busy={props.isBusy || undefined} | ||
| className="absolute inset-0 -z-1 cursor-pointer rounded-[inherit] outline-none focus-visible:ring-2 focus-visible:ring-inset focus-visible:ring-ring" | ||
| > | ||
| <span className="sr-only">{props.name}</span> | ||
| </button> | ||
| ); | ||
| } | ||
|
|
||
| // Structural list items — the section headers and the | ||
| // empty-section placeholders — take part in the sortable list so they shift | ||
| // with the rows and the gap can open on either side of them. They can't be | ||
|
|
@@ -729,26 +766,7 @@ const SidebarDraftRow = memo(function SidebarDraftRow(props: { | |
| promptPreview.length > 0 | ||
| ? promptPreview | ||
| : `${attachmentCount} attachment${attachmentCount === 1 ? "" : "s"}`; | ||
| const handleActivate = useCallback(() => onNavigate(draftId), [draftId, onNavigate]); | ||
| const handleDiscard = useCallback( | ||
| (event: ReactMouseEvent) => { | ||
| event.preventDefault(); | ||
|
|
@@ -760,19 +778,15 @@ const SidebarDraftRow = memo(function SidebarDraftRow(props: { | |
| return ( | ||
| <li className="list-none py-0.5"> | ||
| <div | ||
| data-testid="sidebar-draft-row" | ||
| className={cn( | ||
| "group/sidebar-row relative isolate w-full cursor-pointer overflow-hidden rounded-md text-left text-sidebar-foreground select-none", | ||
| props.isActive ? "bg-sidebar-row-active" : draftSurfaceClassName, | ||
| )} | ||
| onMouseDown={focusSidebarRowPrimaryAction} | ||
| onClick={handleActivate} | ||
| > | ||
| <SidebarRowPrimaryAction name={`${preview}, Unsent draft`} isActive={props.isActive} /> | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| <div className="relative z-10 h-[4.875rem] px-(--sidebar-row-content-inset) py-(--sidebar-content-inset)"> | ||
| <div className="flex h-5 min-w-0 items-center gap-1.5"> | ||
| <SquarePenIcon aria-hidden className={draftPenClassName} /> | ||
|
|
@@ -1253,21 +1267,12 @@ const SidebarThreadRow = memo(function SidebarThreadRow(props: { | |
| }, | ||
| [onContextMenu, threadRef], | ||
| ); | ||
| const handleDoubleClick = useCallback( | ||
| (event: ReactMouseEvent) => { | ||
| if (isRenaming || event.metaKey || event.ctrlKey || event.shiftKey || event.altKey) { | ||
| return; | ||
| } | ||
| if ((event.target as HTMLElement).closest(SIDEBAR_ROW_SECONDARY_CONTROL_SELECTOR)) return; | ||
| event.preventDefault(); | ||
| onStartRename(threadRef, thread.title); | ||
| }, | ||
|
|
@@ -1404,7 +1409,7 @@ const SidebarThreadRow = memo(function SidebarThreadRow(props: { | |
| // a useful hierarchy nor a reliable hover cue. Status now lives in the row | ||
| // content; surface is reserved for interaction (hover, multi-select, route). | ||
| const rowSurfaceClassName = cn( | ||
| "group/sidebar-row relative isolate w-full cursor-pointer overflow-hidden rounded-md text-left select-none", | ||
| variantAction === "unsettle" && "[&:not(:hover):not(:focus-within)_*]:text-secondary-label/70", | ||
| props.isActive | ||
| ? "bg-sidebar-row-active text-sidebar-foreground" | ||
|
|
@@ -1459,13 +1464,6 @@ const SidebarThreadRow = memo(function SidebarThreadRow(props: { | |
| </span> | ||
| ) : null; | ||
|
|
||
| const title = isRenaming ? ( | ||
| <input | ||
| autoFocus | ||
|
|
@@ -1512,7 +1510,13 @@ const SidebarThreadRow = memo(function SidebarThreadRow(props: { | |
| {thread.title} | ||
| </span> | ||
| ); | ||
| const primaryAction = ( | ||
| <SidebarRowPrimaryAction | ||
| name={thread.title} | ||
| isActive={props.isActive} | ||
| isBusy={isRegeneratingTitle} | ||
| /> | ||
| ); | ||
|
|
||
| // Stacks show their layer count; multiple unrelated links show their total count. | ||
| // Plain clicks open T3; individual PR links also support opening the host in a new tab. | ||
|
|
@@ -1612,21 +1616,16 @@ const SidebarThreadRow = memo(function SidebarThreadRow(props: { | |
| render={ | ||
| <div | ||
| ref={rowRef} | ||
| data-testid="sidebar-row-slim" | ||
| className={cn(rowSurfaceClassName, "flex h-9 items-center gap-2.5 px-2.5")} | ||
| onMouseDown={focusSidebarRowPrimaryAction} | ||
| onClick={handleClick} | ||
| onDoubleClick={handleDoubleClick} | ||
| onContextMenu={handleContextMenu} | ||
| /> | ||
| } | ||
| > | ||
| {primaryAction} | ||
| {/* Settled history recedes: dimmed favicon at rest, restored on | ||
| hover so the tail stays scannable when you're hunting. */} | ||
| <span | ||
|
|
@@ -1768,21 +1767,16 @@ const SidebarThreadRow = memo(function SidebarThreadRow(props: { | |
| render={ | ||
| <div | ||
| ref={rowRef} | ||
| data-testid="sidebar-row-card" | ||
| className={rowSurfaceClassName} | ||
| onMouseDown={focusSidebarRowPrimaryAction} | ||
| onClick={handleClick} | ||
| onDoubleClick={handleDoubleClick} | ||
| onContextMenu={handleContextMenu} | ||
| /> | ||
| } | ||
| > | ||
| {primaryAction} | ||
| <div className="relative z-10 h-[4.875rem] px-(--sidebar-row-content-inset) py-(--sidebar-content-inset)"> | ||
| <div className="flex h-5 min-w-0 items-center gap-1.5"> | ||
| {draftIndicator} | ||
|
|
@@ -2035,12 +2029,6 @@ const SidebarSearchResultRow = memo(function SidebarSearchResultRow(props: { | |
| onFileDropThreads: (threadRef: ScopedThreadRef, files: File[]) => void; | ||
| }) { | ||
| const { thread } = props; | ||
| const threadRef = useMemo( | ||
| () => scopeThreadRef(thread.environmentId, thread.id), | ||
| [thread.environmentId, thread.id], | ||
|
|
@@ -2117,8 +2105,12 @@ const SidebarSearchResultRow = memo(function SidebarSearchResultRow(props: { | |
| // which owns all keyboard interaction for the listbox. | ||
| tabIndex={-1} | ||
| aria-selected={props.isHighlighted} | ||
| aria-current={props.isRouteActive ? "page" : undefined} | ||
| aria-label={ | ||
| props.projectDisplayName | ||
| ? `${thread.title}, ${props.projectDisplayName}` | ||
| : thread.title | ||
| } | ||
| onMouseMove={props.onHighlight} | ||
| onClick={props.onSelect} | ||
| className={cn( | ||
|
|
@@ -4693,11 +4685,9 @@ export default function Sidebar() { | |
| <SortableContext items={sortableIds} strategy={sidebarSortingStrategy}> | ||
| <ul | ||
| ref={attachListMotionRef} | ||
| // Explicit because Safari drops list semantics from a list | ||
| // styled without markers. | ||
| role="list" | ||
|
Contributor
There was a problem hiding this comment. 🟡 Medium
🤖 Copy this AI Prompt to have your agent fix this:
Contributor
Author
There was a problem hiding this comment. Keeping Whether the list is an interaction boundary depends on the VoiceOver user's web navigation setting. Per Apple's guide, lists need interaction under Grouping items but not under DOM order. Under Grouping items, entering a list is the expected way to move through it, as it is for any other list on the web. The VoiceOver pass in Safari and Chrome, under both settings, is still the merge check for this PR. I'm leaving this thread open until that's done. If that pass shows the list gets in the way, we'll revisit it then.
Contributor
There was a problem hiding this comment. Sorry, I'm unable to act on this request because you do not have permissions within this repository. |
||
| className={cn( | ||
| "relative flex flex-col gap-px", | ||
| sidebarListItems.length > 0 && "flex-1", | ||
|
|
||
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.