Skip to content

feat: modal accessibility and escape key features for unity react and… - #1753

Open
spaceperson wants to merge 2 commits into
devfrom
uds02-2211
Open

feat: modal accessibility and escape key features for unity react and…#1753
spaceperson wants to merge 2 commits into
devfrom
uds02-2211

Conversation

@spaceperson

Copy link
Copy Markdown
Contributor

… bootstrap

Description

Checklist

  • Tests pass for relevant code changes

Important Reminders

Links

@spaceperson
spaceperson requested a review from a team as a code owner July 23, 2026 20:27
@asu-jenkins-devops

Copy link
Copy Markdown
Collaborator

@davidornelas11 davidornelas11 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

changes in comments

id="uds-modal"
role="dialog"
aria-modal="true"
aria-labelledby={modalTitle}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

aria-labelledby needs to reference an id this just points to a string of text. I would actually switch this to use aria-label instead of aria-labelledby.


const handleOpen = () => {
setOpen(true);
focusOnModalInput();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

focusOnModalInput() runs synchronously right after setOpen(true), but React hasn't re-rendered yet. #uds-modal-container doesn't exist in the DOM on first open, so modal here resolves to undefined and no focus/trap gets set. Move this logic into a useEffect keyed on openState instead, so it runs after the DOM updates.

Claude suggested this refactor but feel free to come up with another solution if you dont think it works well:

const handleOpen = () => {
  setOpen(true);
};

const handleClose = () => {
  setOpen(false);
};

useEffect(() => {
  if (!openState) return;

  const focusableElements =
    'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])';
  const modal = document.getElementsByClassName("uds-modal-container")[0];
  const focusableContent = modal?.querySelectorAll<HTMLElement>(focusableElements);
  const firstFocusableElement = focusableContent?.[0];
  const lastFocusableElement = focusableContent?.[focusableContent.length - 1];

  firstFocusableElement?.focus();

  const handleTabKey = (e: KeyboardEvent) => {
    if (e.key !== "Tab" || !firstFocusableElement || !lastFocusableElement) return;

    if (e.shiftKey && document.activeElement === firstFocusableElement) {
      lastFocusableElement.focus();
      e.preventDefault();
    } else if (!e.shiftKey && document.activeElement === lastFocusableElement) {
      firstFocusableElement.focus();
      e.preventDefault();
    }
  };

  document.addEventListener("keydown", handleTabKey);
  return () => document.removeEventListener("keydown", handleTabKey);
}, [openState]);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants