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(docs): resolve keyboard navigation issues on Actions menu #4528

Open
wants to merge 1 commit into
base: canary
Choose a base branch
from
Open

fix(docs): resolve keyboard navigation issues on Actions menu #4528

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
31 changes: 28 additions & 3 deletions apps/docs/components/code-window/window-actions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React from "react";
import React, {useEffect, useCallback, useState} from "react";
import {tv} from "tailwind-variants";
import {clsx} from "@nextui-org/shared-utils";

Expand All @@ -19,23 +19,48 @@ const windowIconStyles = tv({
});

export const WindowActions: React.FC<WindowActionsProps> = ({title, className, ...props}) => {
const [isMenuVisible, setIsMenuVisible] = useState(true);

const handleKeyDown = useCallback((e: KeyboardEvent) => {
if (e.key === "Escape") {
setIsMenuVisible(false); // Close the menu
}
}, []);

Comment on lines +22 to +28
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

Enhance focus management and initial state handling

Consider the following improvements:

  1. Initialize isMenuVisible to false by default to prevent the menu from being open on mount
  2. Implement focus management to return focus to the trigger element when menu closes
  3. Add a focus trap to keep focus within the menu while it's open
-const [isMenuVisible, setIsMenuVisible] = useState(true);
+const [isMenuVisible, setIsMenuVisible] = useState(false);
+const previousFocusRef = useRef<HTMLElement | null>(null);
+
+const handleClose = useCallback(() => {
+  setIsMenuVisible(false);
+  if (previousFocusRef.current) {
+    previousFocusRef.current.focus();
+  }
+}, []);

 const handleKeyDown = useCallback((e: KeyboardEvent) => {
   if (e.key === "Escape") {
-    setIsMenuVisible(false);
+    handleClose();
   }
 }, []);

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

useEffect(() => {
window.addEventListener("keydown", handleKeyDown);

return () => {
window.removeEventListener("keydown", handleKeyDown);
};
}, [handleKeyDown]);

if (!isMenuVisible) return null; // Hide component if menu is closed

return (
<div
aria-hidden={!isMenuVisible} // Visibility state
aria-labelledby="window-actions-title"
className={clsx(
"flex items-center sticky top-0 left-0 px-4 z-10 justify-between h-8 bg-code-background w-full",
className,
)}
role="dialog" // Semantic role
{...props}
Comment on lines +42 to +48
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

Enhance ARIA attributes for better screen reader support

Add the following ARIA attributes to improve accessibility:

  1. aria-modal="true" to indicate modal behavior
  2. aria-describedby for additional context
  3. tabindex="-1" to manage focus properly
 <div
   aria-hidden={!isMenuVisible}
   aria-labelledby="window-actions-title"
+  aria-modal="true"
+  aria-describedby="window-actions-description"
+  tabIndex={-1}
   role="dialog"
   className={clsx(

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

>
<div className="flex items-center gap-2 basis-1/3">
<div className={windowIconStyles({color: "red"})} />
<div className={windowIconStyles({color: "yellow"})} />
<div className={windowIconStyles({color: "green"})} />
</div>
<div className="flex basis-1/3 h-full justify-center items-center">
<div className="flex basis-1/3 h-full justify-center items-center" id="window-actions-title">
{title && <p className="text-white/30 text-xs font-light">{title}</p>}
</div>
<div className="flex basis-1/3" />
<div className="flex basis-1/3">
<button className="ml-auto text-xs text-white/50" onClick={() => setIsMenuVisible(false)}>
Close
</button>
</div>
</div>
Comment on lines +59 to +63
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

Improve close button accessibility and UX

The close button needs the following improvements:

  1. Add aria-label for screen readers
  2. Add keyboard shortcut hint using aria-keyshortcuts
  3. Enhance button styling for better visibility
 <div className="flex basis-1/3">
   <button
+    aria-label="Close window actions menu"
+    aria-keyshortcuts="Escape"
-    className="ml-auto text-xs text-white/50"
+    className="ml-auto text-xs text-white/50 hover:text-white/70 focus:text-white/70 focus:outline-none focus:ring-2 focus:ring-white/20 rounded px-2 py-1"
     onClick={() => setIsMenuVisible(false)}
   >
     Close
   </button>
 </div>
📝 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
<div className="flex basis-1/3">
<button className="ml-auto text-xs text-white/50" onClick={() => setIsMenuVisible(false)}>
Close
</button>
</div>
<div className="flex basis-1/3">
<button
aria-label="Close window actions menu"
aria-keyshortcuts="Escape"
className="ml-auto text-xs text-white/50 hover:text-white/70 focus:text-white/70 focus:outline-none focus:ring-2 focus:ring-white/20 rounded px-2 py-1"
onClick={() => setIsMenuVisible(false)}
>
Close
</button>
</div>

);
};
Loading