Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ThreadList a11y improvements #1221

Merged
merged 1 commit into from
Nov 25, 2024
Merged

fix: ThreadList a11y improvements #1221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/green-actors-warn.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@assistant-ui/react": patch
---

fix: ThreadList a11y improvements
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Consider adding more details to the commit message.

While the message follows the conventional commit format, it would be helpful to specify which accessibility improvements were made (e.g., "fix: Add aria-current and focus styles to ThreadList").

-fix: ThreadList a11y improvements
+fix: Add aria-current attributes and focus styles to ThreadList for improved accessibility
📝 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
fix: ThreadList a11y improvements
fix: Add aria-current attributes and focus styles to ThreadList for improved accessibility
🧰 Tools
🪛 Markdownlint (0.35.0)

5-5: null
First line in a file should be a top-level heading

(MD041, first-line-heading, first-line-h1)

2 changes: 1 addition & 1 deletion packages/react/src/primitives/threadList/ThreadListNew.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export const ThreadListPrimitiveNew = forwardRef<
return (
<Primitive.button
type="button"
{...(isMain ? { "data-active": "true" } : null)}
{...(isMain ? { "data-active": "true", "aria-current": "true" } : null)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Consider using a more specific aria-current value.

While aria-current="true" is valid, consider using a more semantic value like "page" or "location" to better describe the type of current item. Also, the data-active attribute might be redundant since aria-current already indicates the active state.

-      {...(isMain ? { "data-active": "true", "aria-current": "true" } : null)}
+      {...(isMain ? { "aria-current": "page" } : null)}
📝 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
{...(isMain ? { "data-active": "true", "aria-current": "true" } : null)}
{...(isMain ? { "aria-current": "page" } : null)}

{...props}
ref={forwardedRef}
disabled={disabled || !callback}
Expand Down
2 changes: 1 addition & 1 deletion packages/react/src/primitives/threadListItem/ThreadListItemRoot.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export const ThreadListItemPrimitiveRoot = forwardRef<

return (
<Primitive.div
{...(isMain ? { "data-active": "true" } : null)}
{...(isMain ? { "data-active": "true", "aria-current": "true" } : null)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Adjust aria-current value for better accessibility

While adding aria-current is a great accessibility improvement, its value should be more specific than "true". The ARIA specification recommends using values like "page", "step", "location", "date", "time", or "false" depending on the type of navigation.

For a thread list item, "page" would be more appropriate as it indicates the current page or screen in a set.

-      {...(isMain ? { "data-active": "true", "aria-current": "true" } : null)}
+      {...(isMain ? { "data-active": "true", "aria-current": "page" } : null)}
📝 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
{...(isMain ? { "data-active": "true", "aria-current": "true" } : null)}
{...(isMain ? { "data-active": "true", "aria-current": "page" } : null)}

{...props}
ref={ref}
/>
Expand Down
2 changes: 1 addition & 1 deletion packages/react/src/styles/tailwindcss/thread.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@
}

.aui-thread-list-item {
@apply data-[active]:bg-aui-muted hover:bg-aui-muted flex items-center gap-2 rounded-lg transition-all;
@apply data-[active]:bg-aui-muted hover:bg-aui-muted focus-visible:bg-aui-muted focus-visible:ring-aui-ring flex items-center gap-2 rounded-lg transition-all focus-visible:outline-none focus-visible:ring-2;
}

.aui-thread-list-new {
Expand Down
12 changes: 6 additions & 6 deletions packages/react/src/ui/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ export { default as BranchPicker } from "./branch-picker";

export { default as Composer } from "./composer";

export { default as ContentPart } from "./content-part";

Comment on lines +23 to +24
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Accessibility implementation is present but could be enhanced

The components show some accessibility considerations:

  • TooltipIconButton includes screen reader text (aui-sr-only) and proper tooltip implementation
  • UserActionBar uses semantic button elements and tooltips for better UX
  • ContentPart is a simple text wrapper with proper semantic markup (<p> element)

However, some improvements could be made:

  • Add proper ARIA roles and attributes to the action bar items
  • Ensure keyboard focus management for the edit functionality
  • Consider adding aria-live regions for dynamic content updates
🔗 Analysis chain

Verify accessibility implementation in new components.

Since this PR focuses on a11y improvements, please ensure that the newly added components (ContentPart and UserActionBar) follow accessibility best practices:

  • Proper ARIA attributes
  • Keyboard navigation support
  • Screen reader compatibility

Let's check the accessibility implementation:

Also applies to: 41-41

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for accessibility-related code in the new components
# Expected: Find ARIA attributes, role declarations, and keyboard event handlers

# Search for accessibility-related patterns in ContentPart
echo "Checking ContentPart for accessibility implementations:"
rg -i "aria-|role=|onKey|tabIndex" packages/react/src/ui/content-part

# Search for accessibility-related patterns in UserActionBar
echo "Checking UserActionBar for accessibility implementations:"
rg -i "aria-|role=|onKey|tabIndex" packages/react/src/ui/user-action-bar

Length of output: 545

Script:

#!/bin/bash
# Let's first find the correct paths for these components
echo "Finding ContentPart component:"
fd -e tsx -e ts -e js -e jsx "content-part" packages/

echo "Finding UserActionBar component:"
fd -e tsx -e ts -e jsx -e js "user-action-bar" packages/

# After finding the paths, let's check for accessibility patterns
echo "Checking for accessibility implementations:"
fd -e tsx -e ts -e jsx -e js "(content-part|user-action-bar)" packages/ --exec rg -i "aria-|role=|onKey|tabIndex|focus|keyboard|screen.?reader|accessibility" {}

Length of output: 680

Script:

#!/bin/bash
# Let's examine the content of these components directly
echo "Content of ContentPart component:"
cat packages/react/src/ui/content-part.tsx

echo -e "\nContent of UserActionBar component:"
cat packages/react/src/ui/user-action-bar.tsx

Length of output: 2747

Script:

#!/bin/bash
# Let's check the primitives these components are based on
echo "Checking ContentPartPrimitive implementation:"
cat packages/react/src/primitives/content-part.tsx

echo -e "\nChecking ActionBarPrimitive implementation:"
cat packages/react/src/primitives/action-bar.tsx

echo -e "\nChecking TooltipIconButton implementation:"
cat packages/react/src/ui/base/tooltip-icon-button.tsx

Length of output: 1476

export {
default as AttachmentUI, // TODO name collision with Attachment
} from "./attachment-ui";
Expand All @@ -28,14 +30,12 @@ export { default as EditComposer } from "./edit-composer";

export { default as Thread } from "./thread";

export { default as UserMessage } from "./user-message";
export { default as ThreadList } from "./thread-list";

export { default as UserActionBar } from "./user-action-bar";
export { default as ThreadListItem } from "./thread-list-item";

export { default as ThreadWelcome } from "./thread-welcome";

export { default as ContentPart } from "./content-part";

export { default as ThreadList } from "./thread-list";
export { default as UserMessage } from "./user-message";

export { default as ThreadListItem } from "./thread-list-item";
export { default as UserActionBar } from "./user-action-bar";
2 changes: 1 addition & 1 deletion packages/react/src/ui/thread-config.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ export type StringsConfig = {
fallback?: string | undefined;
};
archive?: {
label?: string | undefined;
tooltip?: string | undefined;
};
};
};
Expand Down
56 changes: 24 additions & 32 deletions packages/react/src/ui/thread-list-item.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,9 @@ const ThreadListItemTitle = forwardRef<
ThreadListItemPrimitiveTitle.Element,
ThreadListItemPrimitiveTitle.Props
>(({ className, ...props }, ref) => {
const {
strings: {
threadList: { item: { title: { fallback = "New Chat" } = {} } = {} } = {},
} = {},
} = useThreadConfig();
const config = useThreadConfig();
const fallback =
config.strings?.threadList?.item?.title?.fallback ?? "New Chat";
Comment on lines +42 to +44
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Consider adding ARIA attributes for better accessibility.

While the config and fallback handling is good, consider enhancing accessibility by:

  1. Adding an appropriate aria-label or aria-labelledby attribute to the paragraph element.
  2. Consider moving the "New Chat" fallback string to a localization constant.

Example enhancement:

 return (
   <p
     ref={ref}
     className={classNames("aui-thread-list-item-title", className)}
+    aria-label={props['aria-label'] ?? fallback}
     {...props}
   >
     <ThreadListItemPrimitive.Title fallback={fallback} />
   </p>
 );

Committable suggestion skipped: line range outside the PR's diff.


return (
<p
Expand All @@ -58,34 +56,28 @@ const ThreadListItemTitle = forwardRef<

ThreadListItemTitle.displayName = "ThreadListItemTitle";

const ThreadListItemArchive = withDefaults(
forwardRef<HTMLButtonElement, TooltipIconButton.Props>(
({ className, ...props }, ref) => {
const {
strings: {
threadList: {
item: { archive: { label = "Archive thread" } = {} } = {},
} = {},
} = {},
} = useThreadConfig();
const ThreadListItemArchive = forwardRef<
HTMLButtonElement,
Partial<TooltipIconButton.Props>
>(({ className, ...props }, ref) => {
const config = useThreadConfig();
const tooltip =
config.strings?.threadList?.item?.archive?.tooltip ?? "Archive thread";

return (
<ThreadListItemPrimitive.Archive asChild>
<TooltipIconButton
{...props}
ref={ref}
className={classNames("aui-thread-list-item-archive", className)}
variant="ghost"
tooltip={label}
>
<ArchiveIcon />
</TooltipIconButton>
</ThreadListItemPrimitive.Archive>
);
},
),
{},
);
return (
<ThreadListItemPrimitive.Archive asChild>
<TooltipIconButton
ref={ref}
className={classNames("aui-thread-list-item-archive", className)}
variant="ghost"
tooltip={tooltip}
{...props}
>
<ArchiveIcon />
</TooltipIconButton>
</ThreadListItemPrimitive.Archive>
);
});

ThreadListItemArchive.displayName = "ThreadListItemArchive";

Expand Down
20 changes: 13 additions & 7 deletions packages/react/src/ui/thread-list.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,17 +27,16 @@ const ThreadListNew = forwardRef<
HTMLButtonElement,
ThreadListPrimitive.New.Props & ButtonProps
>((props, ref) => {
const {
strings: { threadList: { new: { label = "New Thread" } = {} } = {} } = {},
} = useThreadConfig();
const config = useThreadConfig();
const label = config.strings?.threadList?.new?.label ?? "New Thread";
Comment on lines +30 to +31
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Consider adding aria-label for better accessibility

While the label extraction logic is good, consider adding an explicit aria-label to the Button component to ensure screen readers announce it correctly, especially if the label contains any special characters or formatting.

 <Button
   ref={ref}
   className="aui-thread-list-new"
   variant="ghost"
+  aria-label={label}
   {...props}
 >

Committable suggestion skipped: line range outside the PR's diff.


return (
<ThreadListPrimitive.New asChild>
<Button
{...props}
ref={ref}
className="aui-thread-list-new"
variant="ghost"
{...props}
>
<PlusIcon />
{label}
Expand All @@ -47,9 +46,16 @@ const ThreadListNew = forwardRef<
});
ThreadListNew.displayName = "ThreadListNew";

const ThreadListItems: FC<{
components?: Partial<ThreadListPrimitive.Items.Props["components"]>;
}> = ({ components }) => {
namespace ThreadListItems {
export type Props = {
/**
* Optional custom components to override default thread list items
*/
components?: Partial<ThreadListPrimitive.Items.Props["components"]>;
};
}
Comment on lines +49 to +56
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Enhance JSDoc documentation with accessibility requirements

The components prop documentation could be more detailed about accessibility requirements that custom components should meet.

 /**
  * Optional custom components to override default thread list items
+ * @remarks
+ * Custom components should maintain accessibility features:
+ * - Proper heading structure
+ * - Keyboard navigation support
+ * - ARIA attributes (aria-current, aria-label)
+ * - Focus management
  */
📝 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
namespace ThreadListItems {
export type Props = {
/**
* Optional custom components to override default thread list items
*/
components?: Partial<ThreadListPrimitive.Items.Props["components"]>;
};
}
namespace ThreadListItems {
export type Props = {
/**
* Optional custom components to override default thread list items
* @remarks
* Custom components should maintain accessibility features:
* - Proper heading structure
* - Keyboard navigation support
* - ARIA attributes (aria-current, aria-label)
* - Focus management
*/
components?: Partial<ThreadListPrimitive.Items.Props["components"]>;
};
}


const ThreadListItems: FC<ThreadListItems.Props> = ({ components }) => {
return (
<ThreadListPrimitive.Items
components={{
Expand Down