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

Author: RobinMalfait

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
 

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/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', () => {

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

@@ -32,6 +32,7 @@ import { isDisabledReactIssue7711 } from '../../utils/bugs'
 import { isFocusableElement, FocusableMode } from '../../utils/focus-management'
 import { useWindowEvent } from '../../hooks/use-window-event'
 import { useOpenClosed, State, OpenClosedProvider } from '../../internal/open-closed'
+import { useResolveButtonType } from '../../hooks/use-resolve-button-type'
 
 enum ListboxStates {
   Open,
@@ -370,7 +371,7 @@ let Button = forwardRefWithAs(function Button<TTag extends ElementType = typeof
   let propsWeControl = {
     ref: buttonRef,
     id,
-    type: 'button',
+    type: useResolveButtonType(props, state.buttonRef),
     'aria-haspopup': true,
     'aria-controls': state.optionsRef.current?.id,
     'aria-expanded': state.disabled ? undefined : state.listboxState === ListboxStates.Open,

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

@@ -184,6 +184,65 @@ describe('Rendering', () => {
         assertMenu({ state: MenuState.Visible })
       })
     )
+    describe('`type` attribute', () => {
+      it('should set the `type` to "button" by default', async () => {
+        render(
+          <Menu>
+            <Menu.Button>Trigger</Menu.Button>
+          </Menu>
+        )
+
+        expect(getMenuButton()).toHaveAttribute('type', 'button')
+      })
+
+      it('should not set the `type` to "button" if it already contains a `type`', async () => {
+        render(
+          <Menu>
+            <Menu.Button type="submit">Trigger</Menu.Button>
+          </Menu>
+        )
+
+        expect(getMenuButton()).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(
+          <Menu>
+            <Menu.Button as={CustomButton}>Trigger</Menu.Button>
+          </Menu>
+        )
+
+        expect(getMenuButton()).toHaveAttribute('type', 'button')
+      })
+
+      it('should not set the type if the "as" prop is not a "button"', async () => {
+        render(
+          <Menu>
+            <Menu.Button as="div">Trigger</Menu.Button>
+          </Menu>
+        )
+
+        expect(getMenuButton()).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(
+          <Menu>
+            <Menu.Button as={CustomButton}>Trigger</Menu.Button>
+          </Menu>
+        )
+
+        expect(getMenuButton()).not.toHaveAttribute('type')
+      })
+    })
   })
 
   describe('Menu.Items', () => {

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

@@ -34,6 +34,7 @@ import { isFocusableElement, FocusableMode } from '../../utils/focus-management'
 import { useWindowEvent } from '../../hooks/use-window-event'
 import { useTreeWalker } from '../../hooks/use-tree-walker'
 import { useOpenClosed, State, OpenClosedProvider } from '../../internal/open-closed'
+import { useResolveButtonType } from '../../hooks/use-resolve-button-type'
 
 enum MenuStates {
   Open,
@@ -294,7 +295,7 @@ let Button = forwardRefWithAs(function Button<TTag extends ElementType = typeof
   let propsWeControl = {
     ref: buttonRef,
     id,
-    type: 'button',
+    type: useResolveButtonType(props, state.buttonRef),
     'aria-haspopup': true,
     'aria-controls': state.itemsRef.current?.id,
     'aria-expanded': props.disabled ? undefined : state.menuState === MenuStates.Open,

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

@@ -23,7 +23,7 @@ jest.mock('../../hooks/use-id')
 afterAll(() => jest.restoreAllMocks())
 
 function nextFrame() {
-  return new Promise(resolve => {
+  return new Promise<void>(resolve => {
     requestAnimationFrame(() => {
       requestAnimationFrame(() => {
         resolve()
@@ -319,6 +319,66 @@ describe('Rendering', () => {
         assertPopoverPanel({ state: PopoverState.Visible })
       })
     )
+
+    describe('`type` attribute', () => {
+      it('should set the `type` to "button" by default', async () => {
+        render(
+          <Popover>
+            <Popover.Button>Trigger</Popover.Button>
+          </Popover>
+        )
+
+        expect(getPopoverButton()).toHaveAttribute('type', 'button')
+      })
+
+      it('should not set the `type` to "button" if it already contains a `type`', async () => {
+        render(
+          <Popover>
+            <Popover.Button type="submit">Trigger</Popover.Button>
+          </Popover>
+        )
+
+        expect(getPopoverButton()).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(
+          <Popover>
+            <Popover.Button as={CustomButton}>Trigger</Popover.Button>
+          </Popover>
+        )
+
+        expect(getPopoverButton()).toHaveAttribute('type', 'button')
+      })
+
+      it('should not set the type if the "as" prop is not a "button"', async () => {
+        render(
+          <Popover>
+            <Popover.Button as="div">Trigger</Popover.Button>
+          </Popover>
+        )
+
+        expect(getPopoverButton()).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(
+          <Popover>
+            <Popover.Button as={CustomButton}>Trigger</Popover.Button>
+          </Popover>
+        )
+
+        expect(getPopoverButton()).not.toHaveAttribute('type')
+      })
+    })
   })
 
   describe('Popover.Panel', () => {