From c1117840fda78cd088ff28ac26305f081ce33fae Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Mon, 2 Aug 2021 13:57:58 +0200 Subject: [PATCH 1/4] Only add `type=button` for real buttons (#709) * add `{type:'button'}` only for buttons We will try and infer the type based on the passed in `props.as` prop or the default tag. However, when somebody uses `as={CustomComponent}` then we don't know what it will render. Therefore we have to pass it a ref and check if the final result is a button or not. If it is, and it doesn't have a `type` yet, then we can set the `type` correctly. * update changelog --- CHANGELOG.md | 8 +- .../components/disclosure/disclosure.test.tsx | 62 +++++++++++- .../src/components/disclosure/disclosure.tsx | 10 +- .../src/components/listbox/listbox.test.tsx | 60 ++++++++++++ .../src/components/listbox/listbox.tsx | 3 +- .../src/components/menu/menu.test.tsx | 59 +++++++++++ .../src/components/menu/menu.tsx | 3 +- .../src/components/popover/popover.test.tsx | 62 +++++++++++- .../src/components/popover/popover.tsx | 8 +- .../src/components/switch/switch.test.tsx | 60 ++++++++++++ .../src/components/switch/switch.tsx | 15 ++- .../src/components/tabs/tabs.test.tsx | 72 ++++++++++++++ .../src/components/tabs/tabs.tsx | 5 +- .../src/hooks/use-resolve-button-type.ts | 34 +++++++ .../src/hooks/use-sync-refs.ts | 2 +- .../test-utils/accessibility-assertions.ts | 4 +- .../components/disclosure/disclosure.test.ts | 94 +++++++++++++++++- .../src/components/disclosure/disclosure.ts | 33 ++++++- .../src/components/listbox/listbox.test.tsx | 97 ++++++++++++++++++- .../src/components/listbox/listbox.ts | 17 +++- .../src/components/menu/menu.test.tsx | 90 +++++++++++++++++ .../src/components/menu/menu.ts | 17 +++- .../src/components/popover/popover.test.ts | 92 +++++++++++++++++- .../src/components/popover/popover.ts | 23 ++++- .../src/components/switch/switch.test.tsx | 90 ++++++++++++++++- .../src/components/switch/switch.ts | 22 +++-- .../src/hooks/use-resolve-button-type.ts | 33 +++++++ .../test-utils/accessibility-assertions.ts | 4 +- 28 files changed, 1026 insertions(+), 53 deletions(-) create mode 100644 packages/@headlessui-react/src/hooks/use-resolve-button-type.ts create mode 100644 packages/@headlessui-vue/src/hooks/use-resolve-button-type.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c631d366e..a34b450b19 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,11 +7,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased - React] -- Nothing yet! +### Fixes + +- Only add `type=button` to real buttons ([#709](https://github.com/tailwindlabs/headlessui/pull/709)) ## [Unreleased - Vue] -- Nothing yet! +### Fixes + +- Only add `type=button` to real buttons ([#709](https://github.com/tailwindlabs/headlessui/pull/709)) ## [@headlessui/react@v1.4.0] - 2021-07-29 diff --git a/packages/@headlessui-react/src/components/disclosure/disclosure.test.tsx b/packages/@headlessui-react/src/components/disclosure/disclosure.test.tsx index a6528a2d6d..6c72cff4e1 100644 --- a/packages/@headlessui-react/src/components/disclosure/disclosure.test.tsx +++ b/packages/@headlessui-react/src/components/disclosure/disclosure.test.tsx @@ -20,7 +20,7 @@ jest.mock('../../hooks/use-id') afterAll(() => jest.restoreAllMocks()) function nextFrame() { - return new Promise(resolve => { + return new Promise(resolve => { requestAnimationFrame(() => { requestAnimationFrame(() => { resolve() @@ -296,6 +296,66 @@ describe('Rendering', () => { assertDisclosurePanel({ state: DisclosureState.Visible }) }) ) + + describe('`type` attribute', () => { + it('should set the `type` to "button" by default', async () => { + render( + + Trigger + + ) + + expect(getDisclosureButton()).toHaveAttribute('type', 'button') + }) + + it('should not set the `type` to "button" if it already contains a `type`', async () => { + render( + + Trigger + + ) + + expect(getDisclosureButton()).toHaveAttribute('type', 'submit') + }) + + it('should set the `type` to "button" when using the `as` prop which resolves to a "button"', async () => { + let CustomButton = React.forwardRef((props, ref) => ( + + + Contents + + + + + ) + } + render() + + assertDialog({ state: DialogState.InvisibleUnmounted }) + + // Open dialog + await click(document.getElementById('trigger')) + + // Verify it is open + assertDialog({ + state: DialogState.Visible, + attributes: { id: 'headlessui-dialog-1' }, + }) + + // Close dialog + await press(Keys.Escape) + + // Verify it is close + assertDialog({ state: DialogState.InvisibleUnmounted }) + }) + ) + + it( + 'should not be possible to close the dialog with Escape, when a field is focused but cancels the event', + suppressConsoleLogs(async () => { + function Example() { + let [isOpen, setIsOpen] = useState(false) + return ( + <> + + + Contents + { + event.preventDefault() + event.stopPropagation() + }} + /> + + + + ) + } + render() + + assertDialog({ state: DialogState.InvisibleUnmounted }) + + // Open dialog + await click(document.getElementById('trigger')) + + // Verify it is open + assertDialog({ + state: DialogState.Visible, + attributes: { id: 'headlessui-dialog-1' }, + }) + + // Try to close the dialog + await press(Keys.Escape) + + // Verify it is still open + assertDialog({ state: DialogState.Visible }) + }) + ) }) }) diff --git a/packages/@headlessui-react/src/components/dialog/dialog.tsx b/packages/@headlessui-react/src/components/dialog/dialog.tsx index 2055bd4ac6..839ca69087 100644 --- a/packages/@headlessui-react/src/components/dialog/dialog.tsx +++ b/packages/@headlessui-react/src/components/dialog/dialog.tsx @@ -7,15 +7,14 @@ import React, { useMemo, useReducer, useRef, + useState, // Types ContextType, ElementType, MouseEvent as ReactMouseEvent, - KeyboardEvent as ReactKeyboardEvent, MutableRefObject, Ref, - useState, } from 'react' import { Props } from '../../types' @@ -217,6 +216,16 @@ let DialogRoot = forwardRefWithAs(function Dialog< close() }) + // Handle `Escape` to close + useWindowEvent('keydown', event => { + if (event.key !== Keys.Escape) return + if (dialogState !== DialogStates.Open) return + if (hasNestedDialogs) return + event.preventDefault() + event.stopPropagation() + close() + }) + // Scroll lock useEffect(() => { if (dialogState !== DialogStates.Open) return @@ -282,16 +291,6 @@ let DialogRoot = forwardRefWithAs(function Dialog< onClick(event: ReactMouseEvent) { event.stopPropagation() }, - - // Handle `Escape` to close - onKeyDown(event: ReactKeyboardEvent) { - if (event.key !== Keys.Escape) return - if (dialogState !== DialogStates.Open) return - if (hasNestedDialogs) return - event.preventDefault() - event.stopPropagation() - close() - }, } let passthroughProps = rest diff --git a/packages/@headlessui-vue/src/components/dialog/dialog.test.ts b/packages/@headlessui-vue/src/components/dialog/dialog.test.ts index e01d98ee9c..e706b1d5fa 100644 --- a/packages/@headlessui-vue/src/components/dialog/dialog.test.ts +++ b/packages/@headlessui-vue/src/components/dialog/dialog.test.ts @@ -526,6 +526,111 @@ describe('Keyboard interactions', () => { assertDialog({ state: DialogState.InvisibleUnmounted }) }) ) + + it( + 'should be possible to close the dialog with Escape, when a field is focused', + suppressConsoleLogs(async () => { + renderTemplate({ + template: ` +
+ + + Contents + + + +
+ `, + setup() { + let isOpen = ref(false) + return { + isOpen, + setIsOpen(value: boolean) { + isOpen.value = value + }, + toggleOpen() { + isOpen.value = !isOpen.value + }, + } + }, + }) + + assertDialog({ state: DialogState.InvisibleUnmounted }) + + // Open dialog + await click(document.getElementById('trigger')) + + // Verify it is open + assertDialog({ + state: DialogState.Visible, + attributes: { id: 'headlessui-dialog-1' }, + }) + + // Close dialog + await press(Keys.Escape) + + // Verify it is close + assertDialog({ state: DialogState.InvisibleUnmounted }) + }) + ) + + it( + 'should not be possible to close the dialog with Escape, when a field is focused but cancels the event', + suppressConsoleLogs(async () => { + renderTemplate({ + template: ` +
+ + + Contents + + + +
+ `, + setup() { + let isOpen = ref(false) + return { + isOpen, + setIsOpen(value: boolean) { + isOpen.value = value + }, + toggleOpen() { + isOpen.value = !isOpen.value + }, + cancel(event: KeyboardEvent) { + event.preventDefault() + event.stopPropagation() + }, + } + }, + }) + + assertDialog({ state: DialogState.InvisibleUnmounted }) + + // Open dialog + await click(document.getElementById('trigger')) + + // Verify it is open + assertDialog({ + state: DialogState.Visible, + attributes: { id: 'headlessui-dialog-1' }, + }) + + // Try to close the dialog + await press(Keys.Escape) + + // Verify it is still open + assertDialog({ state: DialogState.Visible }) + }) + ) }) }) diff --git a/packages/@headlessui-vue/src/components/dialog/dialog.ts b/packages/@headlessui-vue/src/components/dialog/dialog.ts index 346e7deaa3..d2a7f84ad3 100644 --- a/packages/@headlessui-vue/src/components/dialog/dialog.ts +++ b/packages/@headlessui-vue/src/components/dialog/dialog.ts @@ -87,7 +87,6 @@ export let Dialog = defineComponent({ 'aria-labelledby': this.titleId, 'aria-describedby': this.describedby, onClick: this.handleClick, - onKeydown: this.handleKeyDown, } let { open: _, initialFocus, ...passThroughProps } = this.$props @@ -205,6 +204,16 @@ export let Dialog = defineComponent({ nextTick(() => target?.focus()) }) + // Handle `Escape` to close + useWindowEvent('keydown', event => { + if (event.key !== Keys.Escape) return + if (dialogState.value !== DialogStates.Open) return + if (containers.value.size > 1) return // 1 is myself, otherwise other elements in the Stack + event.preventDefault() + event.stopPropagation() + api.close() + }) + // Scroll lock watchEffect(onInvalidate => { if (dialogState.value !== DialogStates.Open) return @@ -260,16 +269,6 @@ export let Dialog = defineComponent({ handleClick(event: MouseEvent) { event.stopPropagation() }, - - // Handle `Escape` to close - handleKeyDown(event: KeyboardEvent) { - if (event.key !== Keys.Escape) return - if (dialogState.value !== DialogStates.Open) return - if (containers.value.size > 1) return // 1 is myself, otherwise other elements in the Stack - event.preventDefault() - event.stopPropagation() - api.close() - }, } }, }) From 3f14839c3343f627779bc6285cb2e074e64d6462 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Mon, 30 Aug 2021 13:48:49 +0200 Subject: [PATCH 4/4] Warn instead of error when there are no focusable elements (#775) * warn instead of error when there are no focusable elements * update changelog Co-authored-by: Krystof Rehacek --- CHANGELOG.md | 2 ++ .../components/focus-trap/focus-trap.test.tsx | 25 +++++++------- .../src/hooks/use-focus-trap.ts | 2 +- .../components/focus-trap/focus-trap.test.ts | 34 ++++++++----------- .../src/hooks/use-focus-trap.ts | 2 +- 5 files changed, 30 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 296b954b1c..b804164bd0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Only add `type=button` to real buttons ([#709](https://github.com/tailwindlabs/headlessui/pull/709)) - Fix `escape` bug not closing Dialog after clicking in Dialog ([#754](https://github.com/tailwindlabs/headlessui/pull/754)) +- Use `console.warn` instead of throwing an error when there are no focusable elements ([#775](https://github.com/tailwindlabs/headlessui/pull/775)) ## [Unreleased - Vue] @@ -19,6 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Only add `type=button` to real buttons ([#709](https://github.com/tailwindlabs/headlessui/pull/709)) - Add Vue emit types ([#679](https://github.com/tailwindlabs/headlessui/pull/679), [#712](https://github.com/tailwindlabs/headlessui/pull/712)) - Fix `escape` bug not closing Dialog after clicking in Dialog ([#754](https://github.com/tailwindlabs/headlessui/pull/754)) +- Use `console.warn` instead of throwing an error when there are no focusable elements ([#775](https://github.com/tailwindlabs/headlessui/pull/775)) ## [@headlessui/react@v1.4.0] - 2021-07-29 diff --git a/packages/@headlessui-react/src/components/focus-trap/focus-trap.test.tsx b/packages/@headlessui-react/src/components/focus-trap/focus-trap.test.tsx index f0de0f330b..58c2c523b0 100644 --- a/packages/@headlessui-react/src/components/focus-trap/focus-trap.test.tsx +++ b/packages/@headlessui-react/src/components/focus-trap/focus-trap.test.tsx @@ -62,20 +62,19 @@ it('should focus the initialFocus element inside the FocusTrap even if another e assertActiveElement(document.getElementById('c')) }) -it( - 'should error when there is no focusable element inside the FocusTrap', - suppressConsoleLogs(() => { - expect(() => { - render( - - Nothing to see here... - - ) - }).toThrowErrorMatchingInlineSnapshot( - `"There are no focusable elements inside the "` +it('should warn when there is no focusable element inside the FocusTrap', () => { + let spy = jest.spyOn(console, 'warn').mockImplementation(jest.fn()) + + function Example() { + return ( + + Nothing to see here... + ) - }) -) + } + render() + expect(spy.mock.calls[0][0]).toBe('There are no focusable elements inside the ') +}) it( 'should not be possible to programmatically escape the focus trap', diff --git a/packages/@headlessui-react/src/hooks/use-focus-trap.ts b/packages/@headlessui-react/src/hooks/use-focus-trap.ts index 2b7eb905cc..68b4c282ac 100644 --- a/packages/@headlessui-react/src/hooks/use-focus-trap.ts +++ b/packages/@headlessui-react/src/hooks/use-focus-trap.ts @@ -89,7 +89,7 @@ export function useFocusTrap( focusElement(initialFocus.current) } else { if (focusIn(container.current, Focus.First) === FocusResult.Error) { - throw new Error('There are no focusable elements inside the ') + console.warn('There are no focusable elements inside the ') } } diff --git a/packages/@headlessui-vue/src/components/focus-trap/focus-trap.test.ts b/packages/@headlessui-vue/src/components/focus-trap/focus-trap.test.ts index ee70eedf46..8243a82ff4 100644 --- a/packages/@headlessui-vue/src/components/focus-trap/focus-trap.test.ts +++ b/packages/@headlessui-vue/src/components/focus-trap/focus-trap.test.ts @@ -109,28 +109,22 @@ it('should focus the initialFocus element inside the FocusTrap even if another e assertActiveElement(document.getElementById('c')) }) -it( - 'should error when there is no focusable element inside the FocusTrap', - suppressConsoleLogs(async () => { - expect.assertions(1) +it('should warn when there is no focusable element inside the FocusTrap', async () => { + expect.assertions(1) + let spy = jest.spyOn(console, 'warn').mockImplementation(jest.fn()) - renderTemplate({ - template: html` - - Nothing to see here... - - `, - errorCaptured(err: unknown) { - expect((err as Error).message).toMatchInlineSnapshot( - `"There are no focusable elements inside the "` - ) - return false - }, - }) + renderTemplate( + html` + + Nothing to see here... + + ` + ) - await new Promise(nextTick) - }) -) + await new Promise(nextTick) + + expect(spy.mock.calls[0][0]).toBe('There are no focusable elements inside the ') +}) it( 'should not be possible to programmatically escape the focus trap', diff --git a/packages/@headlessui-vue/src/hooks/use-focus-trap.ts b/packages/@headlessui-vue/src/hooks/use-focus-trap.ts index 3236a14b69..e5a070e721 100644 --- a/packages/@headlessui-vue/src/hooks/use-focus-trap.ts +++ b/packages/@headlessui-vue/src/hooks/use-focus-trap.ts @@ -53,7 +53,7 @@ export function useFocusTrap( } } - if (!couldFocus) throw new Error('There are no focusable elements inside the ') + if (!couldFocus) console.warn('There are no focusable elements inside the ') } previousActiveElement.value = document.activeElement as HTMLElement