Merge pull request #713 from tailwindlabs/develop

Next release

Author: RobinMalfait

CHANGELOG.md

@@ -7,11 +7,20 @@ 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))
+- 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]
 
-- Nothing yet!
+### Fixes
+
+- 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
 

packages/@headlessui-react/src/components/dialog/dialog.test.tsx

@@ -430,6 +430,90 @@ describe('Keyboard interactions', () => {
         assertDialog({ state: DialogState.InvisibleUnmounted })
       })
     )
+
+    it(
+      'should be possible to close the dialog with Escape, when a field is focused',
+      suppressConsoleLogs(async () => {
+        function Example() {
+          let [isOpen, setIsOpen] = useState(false)
+          return (
+            <>
+              <button id="trigger" onClick={() => setIsOpen(v => !v)}>
+                Trigger
+              </button>
+              <Dialog open={isOpen} onClose={setIsOpen}>
+                Contents
+                <input id="name" />
+                <TabSentinel />
+              </Dialog>
+            </>
+          )
+        }
+        render(<Example />)
+
+        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 (
+            <>
+              <button id="trigger" onClick={() => setIsOpen(v => !v)}>
+                Trigger
+              </button>
+              <Dialog open={isOpen} onClose={setIsOpen}>
+                Contents
+                <input
+                  id="name"
+                  onKeyDown={event => {
+                    event.preventDefault()
+                    event.stopPropagation()
+                  }}
+                />
+                <TabSentinel />
+              </Dialog>
+            </>
+          )
+        }
+        render(<Example />)
+
+        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 })
+      })
+    )
   })
 })
 

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
 

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<void>(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(
+          <Disclosure>
+            <Disclosure.Button>Trigger</Disclosure.Button>
+          </Disclosure>
+        )
+
+        expect(getDisclosureButton()).toHaveAttribute('type', 'button')
+      })
+
+      it('should not set the `type` to "button" if it already contains a `type`', async () => {
+        render(
+          <Disclosure>
+            <Disclosure.Button type="submit">Trigger</Disclosure.Button>
+          </Disclosure>
+        )
+
+        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<HTMLButtonElement>((props, ref) => (
+          <button ref={ref} {...props} />
+        ))
+
+        render(
+          <Disclosure>
+            <Disclosure.Button as={CustomButton}>Trigger</Disclosure.Button>
+          </Disclosure>
+        )
+
+        expect(getDisclosureButton()).toHaveAttribute('type', 'button')
+      })
+
+      it('should not set the type if the "as" prop is not a "button"', async () => {
+        render(
+          <Disclosure>
+            <Disclosure.Button as="div">Trigger</Disclosure.Button>
+          </Disclosure>
+        )
+
+        expect(getDisclosureButton()).not.toHaveAttribute('type')
+      })
+
+      it('should not set the `type` to "button" when using the `as` prop which resolves to a "div"', async () => {
+        let CustomButton = React.forwardRef<HTMLDivElement>((props, ref) => (
+          <div ref={ref} {...props} />
+        ))
+
+        render(
+          <Disclosure>
+            <Disclosure.Button as={CustomButton}>Trigger</Disclosure.Button>
+          </Disclosure>
+        )
+
+        expect(getDisclosureButton()).not.toHaveAttribute('type')
+      })
+    })
   })
 
   describe('Disclosure.Panel', () => {

packages/@headlessui-react/src/components/disclosure/disclosure.tsx

@@ -7,6 +7,7 @@ import React, {
   useEffect,
   useMemo,
   useReducer,
+  useRef,
 
   // Types
   Dispatch,
@@ -26,6 +27,7 @@ import { useId } from '../../hooks/use-id'
 import { Keys } from '../keyboard'
 import { isDisabledReactIssue7711 } from '../../utils/bugs'
 import { OpenClosedProvider, State, useOpenClosed } from '../../internal/open-closed'
+import { useResolveButtonType } from '../../hooks/use-resolve-button-type'
 
 enum DisclosureStates {
   Open,
@@ -226,7 +228,8 @@ let Button = forwardRefWithAs(function Button<TTag extends ElementType = typeof
   ref: Ref<HTMLButtonElement>
 ) {
   let [state, dispatch] = useDisclosureContext([Disclosure.name, Button.name].join('.'))
-  let buttonRef = useSyncRefs(ref)
+  let internalButtonRef = useRef<HTMLButtonElement | null>(null)
+  let buttonRef = useSyncRefs(internalButtonRef, ref)
 
   let panelContext = useDisclosurePanelContext()
   let isWithinPanel = panelContext === null ? false : panelContext === state.panelId
@@ -290,13 +293,14 @@ let Button = forwardRefWithAs(function Button<TTag extends ElementType = typeof
     [state]
   )
 
+  let type = useResolveButtonType(props, internalButtonRef)
   let passthroughProps = props
   let propsWeControl = isWithinPanel
-    ? { type: 'button', onKeyDown: handleKeyDown, onClick: handleClick }
+    ? { ref: buttonRef, type, onKeyDown: handleKeyDown, onClick: handleClick }
     : {
         ref: buttonRef,
         id: state.buttonId,
-        type: 'button',
+        type,
         'aria-expanded': props.disabled
           ? undefined
           : state.disclosureState === DisclosureStates.Open,

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(
-        <FocusTrap>
-          <span>Nothing to see here...</span>
-        </FocusTrap>
-      )
-    }).toThrowErrorMatchingInlineSnapshot(
-      `"There are no focusable elements inside the <FocusTrap />"`
+it('should warn when there is no focusable element inside the FocusTrap', () => {
+  let spy = jest.spyOn(console, 'warn').mockImplementation(jest.fn())
+
+  function Example() {
+    return (
+      <FocusTrap>
+        <span>Nothing to see here...</span>
+      </FocusTrap>
     )
-  })
-)
+  }
+  render(<Example />)
+  expect(spy.mock.calls[0][0]).toBe('There are no focusable elements inside the <FocusTrap />')
+})
 
 it(
   'should not be possible to programmatically escape the focus trap',

packages/@headlessui-react/src/components/listbox/listbox.test.tsx

@@ -326,6 +326,66 @@ describe('Rendering', () => {
         assertListboxButtonLinkedWithListboxLabel()
       })
     )
+
+    describe('`type` attribute', () => {
+      it('should set the `type` to "button" by default', async () => {
+        render(
+          <Listbox value={null} onChange={console.log}>
+            <Listbox.Button>Trigger</Listbox.Button>
+          </Listbox>
+        )
+
+        expect(getListboxButton()).toHaveAttribute('type', 'button')
+      })
+
+      it('should not set the `type` to "button" if it already contains a `type`', async () => {
+        render(
+          <Listbox value={null} onChange={console.log}>
+            <Listbox.Button type="submit">Trigger</Listbox.Button>
+          </Listbox>
+        )
+
+        expect(getListboxButton()).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<HTMLButtonElement>((props, ref) => (
+          <button ref={ref} {...props} />
+        ))
+
+        render(
+          <Listbox value={null} onChange={console.log}>
+            <Listbox.Button as={CustomButton}>Trigger</Listbox.Button>
+          </Listbox>
+        )
+
+        expect(getListboxButton()).toHaveAttribute('type', 'button')
+      })
+
+      it('should not set the type if the "as" prop is not a "button"', async () => {
+        render(
+          <Listbox value={null} onChange={console.log}>
+            <Listbox.Button as="div">Trigger</Listbox.Button>
+          </Listbox>
+        )
+
+        expect(getListboxButton()).not.toHaveAttribute('type')
+      })
+
+      it('should not set the `type` to "button" when using the `as` prop which resolves to a "div"', async () => {
+        let CustomButton = React.forwardRef<HTMLDivElement>((props, ref) => (
+          <div ref={ref} {...props} />
+        ))
+
+        render(
+          <Listbox value={null} onChange={console.log}>
+            <Listbox.Button as={CustomButton}>Trigger</Listbox.Button>
+          </Listbox>
+        )
+
+        expect(getListboxButton()).not.toHaveAttribute('type')
+      })
+    })
   })
 
   describe('Listbox.Options', () => {