From 9f4632eed556653000186a7366ffbd2752abed43 Mon Sep 17 00:00:00 2001 From: Adam Wathan <4323180+adamwathan@users.noreply.github.com> Date: Thu, 24 Aug 2023 11:38:39 -0400 Subject: [PATCH 1/5] WIP --- packages/@headlessui-react/src/components/dialog/dialog.tsx | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/@headlessui-react/src/components/dialog/dialog.tsx b/packages/@headlessui-react/src/components/dialog/dialog.tsx index c82fbf28f1..1d67e096b8 100644 --- a/packages/@headlessui-react/src/components/dialog/dialog.tsx +++ b/packages/@headlessui-react/src/components/dialog/dialog.tsx @@ -119,7 +119,7 @@ let DEFAULT_DIALOG_TAG = 'div' as const interface DialogRenderPropArg { open: boolean } -type DialogPropsWeControl = 'role' | 'aria-describedby' | 'aria-labelledby' | 'aria-modal' +type DialogPropsWeControl = 'aria-describedby' | 'aria-labelledby' | 'aria-modal' let DialogRenderFeatures = Features.RenderStrategy | Features.Static @@ -131,6 +131,7 @@ export type DialogProps = Props< open?: boolean onClose(value: boolean): void initialFocus?: MutableRefObject + role?: 'dialog' | 'alertdialog' __demoMode?: boolean } > @@ -145,6 +146,7 @@ function DialogFn( open, onClose, initialFocus, + role = 'dialog', __demoMode = false, ...theirProps } = props @@ -339,7 +341,7 @@ function DialogFn( let ourProps = { ref: dialogRef, id, - role: 'dialog', + role, 'aria-modal': dialogState === DialogStates.Open ? true : undefined, 'aria-labelledby': state.titleId, 'aria-describedby': describedby, From 2213a8dbec1e44caa0ce227e14f5af80b3bfea17 Mon Sep 17 00:00:00 2001 From: Jordan Pittman Date: Mon, 28 Aug 2023 10:39:55 -0400 Subject: [PATCH 2/5] Add warning for unsupported roles to `` --- .../src/components/dialog/dialog.tsx | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/packages/@headlessui-react/src/components/dialog/dialog.tsx b/packages/@headlessui-react/src/components/dialog/dialog.tsx index 1d67e096b8..baadca247d 100644 --- a/packages/@headlessui-react/src/components/dialog/dialog.tsx +++ b/packages/@headlessui-react/src/components/dialog/dialog.tsx @@ -152,6 +152,23 @@ function DialogFn( } = props let [nestedDialogCount, setNestedDialogCount] = useState(0) + let didWarnOnRole = useRef(false) + + role = (function () { + if (role === 'dialog' || role === 'alertdialog') { + return role + } + + if (!didWarnOnRole.current) { + didWarnOnRole.current = true + console.warn( + `Invalid role [${role}] passed to . Only \`dialog\` and and \`alertdialog\` are supported. Using \`dialog\` instead.` + ) + } + + return 'dialog' + })() + let usesOpenClosedState = useOpenClosed() if (open === undefined && usesOpenClosedState !== null) { // Update the `open` prop based on the open closed state From 548c8586379e80ea3ed1659ad580bf698bb88966 Mon Sep 17 00:00:00 2001 From: Jordan Pittman Date: Mon, 28 Aug 2023 13:06:25 -0400 Subject: [PATCH 3/5] Update assertions --- .../src/test-utils/accessibility-assertions.ts | 8 ++++---- .../src/test-utils/accessibility-assertions.ts | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/@headlessui-react/src/test-utils/accessibility-assertions.ts b/packages/@headlessui-react/src/test-utils/accessibility-assertions.ts index dba8da4cc3..717c26047a 100644 --- a/packages/@headlessui-react/src/test-utils/accessibility-assertions.ts +++ b/packages/@headlessui-react/src/test-utils/accessibility-assertions.ts @@ -1301,11 +1301,11 @@ export function assertDescriptionValue(element: HTMLElement | null, value: strin // --- export function getDialog(): HTMLElement | null { - return document.querySelector('[role="dialog"]') + return document.querySelector('[role="dialog"],[role="alertdialog"]') } export function getDialogs(): HTMLElement[] { - return Array.from(document.querySelectorAll('[role="dialog"]')) + return Array.from(document.querySelectorAll('[role="dialog"],[role="alertdialog"]')) } export function getDialogTitle(): HTMLElement | null { @@ -1358,7 +1358,7 @@ export function assertDialog( assertHidden(dialog) - expect(dialog).toHaveAttribute('role', 'dialog') + expect(dialog).toHaveAttribute('role', options.attributes?.['role'] ?? 'dialog') expect(dialog).not.toHaveAttribute('aria-modal', 'true') if (options.textContent) expect(dialog).toHaveTextContent(options.textContent) @@ -1373,7 +1373,7 @@ export function assertDialog( assertVisible(dialog) - expect(dialog).toHaveAttribute('role', 'dialog') + expect(dialog).toHaveAttribute('role', options.attributes?.['role'] ?? 'dialog') expect(dialog).toHaveAttribute('aria-modal', 'true') if (options.textContent) expect(dialog).toHaveTextContent(options.textContent) diff --git a/packages/@headlessui-vue/src/test-utils/accessibility-assertions.ts b/packages/@headlessui-vue/src/test-utils/accessibility-assertions.ts index dba8da4cc3..717c26047a 100644 --- a/packages/@headlessui-vue/src/test-utils/accessibility-assertions.ts +++ b/packages/@headlessui-vue/src/test-utils/accessibility-assertions.ts @@ -1301,11 +1301,11 @@ export function assertDescriptionValue(element: HTMLElement | null, value: strin // --- export function getDialog(): HTMLElement | null { - return document.querySelector('[role="dialog"]') + return document.querySelector('[role="dialog"],[role="alertdialog"]') } export function getDialogs(): HTMLElement[] { - return Array.from(document.querySelectorAll('[role="dialog"]')) + return Array.from(document.querySelectorAll('[role="dialog"],[role="alertdialog"]')) } export function getDialogTitle(): HTMLElement | null { @@ -1358,7 +1358,7 @@ export function assertDialog( assertHidden(dialog) - expect(dialog).toHaveAttribute('role', 'dialog') + expect(dialog).toHaveAttribute('role', options.attributes?.['role'] ?? 'dialog') expect(dialog).not.toHaveAttribute('aria-modal', 'true') if (options.textContent) expect(dialog).toHaveTextContent(options.textContent) @@ -1373,7 +1373,7 @@ export function assertDialog( assertVisible(dialog) - expect(dialog).toHaveAttribute('role', 'dialog') + expect(dialog).toHaveAttribute('role', options.attributes?.['role'] ?? 'dialog') expect(dialog).toHaveAttribute('aria-modal', 'true') if (options.textContent) expect(dialog).toHaveTextContent(options.textContent) From 9eb909de603c714c0c63e1a2f3f5d719e2add8a9 Mon Sep 17 00:00:00 2001 From: Jordan Pittman Date: Mon, 28 Aug 2023 13:18:31 -0400 Subject: [PATCH 4/5] Add test for React --- .../src/components/dialog/dialog.test.tsx | 94 ++++++++++++++++++- 1 file changed, 93 insertions(+), 1 deletion(-) diff --git a/packages/@headlessui-react/src/components/dialog/dialog.test.tsx b/packages/@headlessui-react/src/components/dialog/dialog.test.tsx index 15255a889f..328c46d5ed 100644 --- a/packages/@headlessui-react/src/components/dialog/dialog.test.tsx +++ b/packages/@headlessui-react/src/components/dialog/dialog.test.tsx @@ -1,6 +1,6 @@ import { createPortal } from 'react-dom' import React, { createElement, useRef, useState, Fragment, useEffect, useCallback } from 'react' -import { render } from '@testing-library/react' +import { render, screen } from '@testing-library/react' import { Dialog } from './dialog' import { Popover } from '../popover/popover' @@ -101,6 +101,98 @@ describe('Rendering', () => { }) ) + it( + 'should be able to explicitly choose role=dialog', + suppressConsoleLogs(async () => { + function Example() { + let [isOpen, setIsOpen] = useState(false) + + return ( + <> + + + + + + ) + } + render() + + assertDialog({ state: DialogState.InvisibleUnmounted }) + + await click(document.getElementById('trigger')) + + await nextFrame() + + assertDialog({ state: DialogState.Visible, attributes: { role: 'dialog' } }) + }) + ) + + it( + 'should be able to explicitly choose role=alertdialog', + suppressConsoleLogs(async () => { + function Example() { + let [isOpen, setIsOpen] = useState(false) + + return ( + <> + + + + + + ) + } + render() + + assertDialog({ state: DialogState.InvisibleUnmounted }) + + await click(document.getElementById('trigger')) + + await nextFrame() + + assertDialog({ state: DialogState.Visible, attributes: { role: 'alertdialog' } }) + }) + ) + + it( + 'should fall back to role=dialog for an invalid role', + suppressConsoleLogs(async () => { + function Example() { + let [isOpen, setIsOpen] = useState(false) + + return ( + <> + + + + + + ) + } + render() + + assertDialog({ state: DialogState.InvisibleUnmounted }) + + await click(document.getElementById('trigger')) + + await nextFrame() + + assertDialog({ state: DialogState.Visible, attributes: { role: 'dialog' } }) + }, 'warn') + ) + it( 'should complain when an `open` prop is provided without an `onClose` prop', suppressConsoleLogs(async () => { From 46ae30557bbf05c42476604e39422ad8ec985701 Mon Sep 17 00:00:00 2001 From: Jordan Pittman Date: Mon, 28 Aug 2023 10:44:48 -0400 Subject: [PATCH 5/5] Add support for `role=alertdialog` to Vue --- .../src/components/dialog/dialog.test.ts | 99 +++++++++++++++++++ .../src/components/dialog/dialog.ts | 19 +++- 2 files changed, 117 insertions(+), 1 deletion(-) diff --git a/packages/@headlessui-vue/src/components/dialog/dialog.test.ts b/packages/@headlessui-vue/src/components/dialog/dialog.test.ts index c35494525d..727f9bb506 100644 --- a/packages/@headlessui-vue/src/components/dialog/dialog.test.ts +++ b/packages/@headlessui-vue/src/components/dialog/dialog.test.ts @@ -191,6 +191,105 @@ describe('Rendering', () => { }) ) + it( + 'should be able to explicitly choose role=dialog', + suppressConsoleLogs(async () => { + renderTemplate({ + template: ` +
+ + + + +
+ `, + setup() { + let isOpen = ref(false) + return { + isOpen, + setIsOpen(value: boolean) { + isOpen.value = value + }, + } + }, + }) + + assertDialog({ state: DialogState.InvisibleUnmounted }) + + await click(document.getElementById('trigger')) + + await nextFrame() + + assertDialog({ state: DialogState.Visible, attributes: { role: 'dialog' } }) + }) + ) + + it( + 'should be able to explicitly choose role=alertdialog', + suppressConsoleLogs(async () => { + renderTemplate({ + template: ` +
+ + + + +
+ `, + setup() { + let isOpen = ref(false) + return { + isOpen, + setIsOpen(value: boolean) { + isOpen.value = value + }, + } + }, + }) + + assertDialog({ state: DialogState.InvisibleUnmounted }) + + await click(document.getElementById('trigger')) + + await nextFrame() + + assertDialog({ state: DialogState.Visible, attributes: { role: 'alertdialog' } }) + }) + ) + + it( + 'should fall back to role=dialog for an invalid role', + suppressConsoleLogs(async () => { + renderTemplate({ + template: ` +
+ + + + +
+ `, + setup() { + let isOpen = ref(false) + return { + isOpen, + setIsOpen(value: boolean) { + isOpen.value = value + }, + } + }, + }) + + assertDialog({ state: DialogState.InvisibleUnmounted }) + + await click(document.getElementById('trigger')) + + await nextFrame() + + assertDialog({ state: DialogState.Visible, attributes: { role: 'dialog' } }) + }) + ) + it( 'should complain when an `open` prop is not a boolean', suppressConsoleLogs(async () => { diff --git a/packages/@headlessui-vue/src/components/dialog/dialog.ts b/packages/@headlessui-vue/src/components/dialog/dialog.ts index ef3ffbb0f7..1e6a8ad70d 100644 --- a/packages/@headlessui-vue/src/components/dialog/dialog.ts +++ b/packages/@headlessui-vue/src/components/dialog/dialog.ts @@ -77,6 +77,7 @@ export let Dialog = defineComponent({ open: { type: [Boolean, String], default: Missing }, initialFocus: { type: Object as PropType, default: null }, id: { type: String, default: () => `headlessui-dialog-${useId()}` }, + role: { type: String as PropType<'dialog' | 'alertdialog'>, default: 'dialog' }, }, emits: { close: (_close: boolean) => true }, setup(props, { emit, attrs, slots, expose }) { @@ -85,6 +86,22 @@ export let Dialog = defineComponent({ ready.value = true }) + let didWarnOnRole = false + let role = computed(() => { + if (props.role === 'dialog' || props.role === 'alertdialog') { + return props.role + } + + if (!didWarnOnRole) { + didWarnOnRole = true + console.warn( + `Invalid role [${role}] passed to . Only \`dialog\` and and \`alertdialog\` are supported. Using \`dialog\` instead.` + ) + } + + return 'dialog' + }) + let nestedDialogCount = ref(0) let usesOpenClosedState = useOpenClosed() @@ -285,7 +302,7 @@ export let Dialog = defineComponent({ ...attrs, ref: internalDialogRef, id, - role: 'dialog', + role: role.value, 'aria-modal': dialogState.value === DialogStates.Open ? true : undefined, 'aria-labelledby': titleId.value, 'aria-describedby': describedby.value,