Skip to content
New issue

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

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

Already on GitHub? # to your account

[✨] Improving focus-related accessibility after modal exit #1002

Closed
sangpok opened this issue Nov 3, 2024 · 2 comments
Closed

[✨] Improving focus-related accessibility after modal exit #1002

sangpok opened this issue Nov 3, 2024 · 2 comments
Labels
STATUS-1: needs triage This doesn't seem right TYPE: enhancement New feature or request

Comments

@sangpok
Copy link

sangpok commented Nov 3, 2024

Is your feature request related to a problem?

The W3C's APG has accessibility guidance for the Modal pattern, which explains what should happen when the modal ends.

When a dialog closes, focus returns to the element that invoked the dialog unless either:
The invoking element no longer exists. Then, focus is set on another element that provides logical work flow.
The work flow de#cludes the following conditions that can occasionally make focusing a different element a more logical choice:
It is very unlikely users need to immediately re-invoke the dialog.
The task completed in the dialog is directly related to a subsequent step in the work flow.
For example, a grid has an associated toolbar with a button for adding rows. The Add Rows button opens a dialog that prompts for the number of rows. After the dialog closes, focus is placed in the first cell of the first new row.

https://www.w3.org/WAI/ARIA/apg/patterns/dialog-modal

QwikUI's modals don't have a default logic for moving focus when a modal ends. So it would be nice to add functionality for this accessibility.

Describe the solution you'd like

If possible, I was wondering if it might be possible to set up something like returnFocusOnClose in the Root property of a QwikUI Modal. This would move the focus to the trigger button after the modal closes.

Where to attach the property

I think the Root component property is a good fit because it's the parent component that contains the Panel and the trigger button behaviour.

The name

The name returnFocusOnClose, which I mentioned earlier, is based on the idea of returning to the trigger button. I think it would be helpful if the documentation explained that, but a different name would also work.

Describe alternatives you've considered

I thought of it like this:

  1. Change the modal context
    Add defaultFocusOnClose.
/** modal-context.tsx */
export type ModalContext = {
  // core state
  localId: string;
  showSig: Signal<boolean>;
  onShow$?: QRL<() => void>;
  onClose$?: QRL<() => void>;
+  defaultFocusOnClose?: boolean;
  closeOnBackdropClick?: boolean;
  alert?: boolean;
};
  1. Change the modal type of the root.
    Add returnFocusOnClose to the modal root prop type.
/** modal-root.tsx */
type ModalRootProps = {
  onShow$?: QRL<() => void>;
  onClose$?: QRL<() => void>;
  'bind:show'?: Signal<boolean>;
  closeOnBackdropClick?: boolean;
+  returnFocusOnClose?: boolean;
  alert?: boolean;
} & PropsOf<'div'>;
  1. Change the root's context provider value
/** modal-root.tsx */
  const {
    'bind:show': givenShowSig,
    closeOnBackdropClick,
+    returnFocusOnClose,
    onShow$,
    onClose$,
    alert,
  } = props;

  const defaultShowSig = useSignal<boolean>(false);
  const showSig = givenShowSig ?? defaultShowSig;
+  const defaultFocusOnClose = returnFocusOnClose ?? true;

  const context: ModalContext = {
    localId,
    showSig,
    closeOnBackdropClick,
+    defaultFocusOnClose,
    onShow$,
    onClose$,
    alert,
  };

  useContextProvider(modalContextId, context);
  1. Change the logic for the modal trigger.
/** modal-trigger.tsx */
export const HModalTrigger = component$((props: PropsOf<'button'>) => {
  const context = useContext(modalContextId);

+  const triggerRef = useSignal<HTMLDialogElement>();
+
+  useTask$(async function focusOnTrigger({ track }) {
+    const isOpen = track(() => context.showSig.value);
+
+    if (!triggerRef.value) return;
+
+    if (!isOpen && context.defaultFocusOnClose) {
+      /**
+       * this code is from 'select-trigger'
+       * https://github.com/qwikifiers/qwik-ui/blob/50c61ba65fb84999a93aac486236fcea36a32352/packages/kit-headless/src/components/select/select-trigger.tsx#L116
+       **/
+      while (triggerRef.value !== document.activeElement) {
+        await new Promise((resolve) => setTimeout(resolve, 5));
+        triggerRef.value?.focus();
+      }
+    }
+  });

  const handleClick$ = $(() => {
    context.showSig.value = !context.showSig.value;
  });

  return (
    <button
+      ref={triggerRef}
      aria-haspopup="dialog"
      aria-expanded={context.showSig.value}
      data-open={context.showSig.value ? '' : undefined}
      data-closed={!context.showSig.value ? '' : undefined}
      onClick$={[handleClick$, props.onClick$]}
      {...props}
    >
      <Slot />
    </button>
  );
});

Usage

Then, you can use it like this

<Modal.Root returnFocusOnClose={false}>

Additional context

I'm new to Qwik library. I followed the guide but may have missed something. I appreciate your understanding.
and if possible, I'd like to write a PR with the above. Is that possible?

@sangpok sangpok added STATUS-1: needs triage This doesn't seem right TYPE: enhancement New feature or request labels Nov 3, 2024
@thejackshelton
Copy link
Collaborator

Hey Sangpok!

Sorry for the late response, been a bit swamped at work lately.

What in particular doesn't return focus? For example, hitting the escape key will already put focus on the trigger, maybe this needs to happen when it is programmatically closed as well? (if it's not currently)

Seems like this needed! Appreciate the explanation.

Also, what about the case where the dialog is opened programmatically? If that's the case, the library may not have access to the user's trigger in this case.

I think the next steps are:

  • creating a failing test of the current expected behavior
  • making sure the changes pass the tests, and return focus in each of the intended areas

A PR would be great 😄 .

@thejackshelton
Copy link
Collaborator

Just went ahead and checked with both a mouse, keyboard, and touch device that the invoking element does have focus after the dialog is closed by default.

If you don't think that is the case, please re-open this issue with a minimal reproduction

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
STATUS-1: needs triage This doesn't seem right TYPE: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants