Skip to content

Commit

Permalink
add {type:'button'} only for buttons
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
RobinMalfait committed Aug 2, 2021
1 parent d25f80a commit ab614c1
Show file tree
Hide file tree
Showing 27 changed files with 1,020 additions and 51 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import React, {
useEffect,
useMemo,
useReducer,
useRef,

// Types
Dispatch,
Expand All @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
60 changes: 60 additions & 0 deletions packages/@headlessui-react/src/components/listbox/listbox.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
59 changes: 59 additions & 0 deletions packages/@headlessui-react/src/components/menu/menu.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
3 changes: 2 additions & 1 deletion packages/@headlessui-react/src/components/menu/menu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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', () => {
Expand Down
8 changes: 6 additions & 2 deletions packages/@headlessui-react/src/components/popover/popover.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import {
} from '../../utils/focus-management'
import { useWindowEvent } from '../../hooks/use-window-event'
import { OpenClosedProvider, State, useOpenClosed } from '../../internal/open-closed'
import { useResolveButtonType } from '../../hooks/use-resolve-button-type'

enum PopoverStates {
Open,
Expand Down Expand Up @@ -309,6 +310,7 @@ let Button = forwardRefWithAs(function Button<TTag extends ElementType = typeof
ref,
isWithinPanel ? null : button => dispatch({ type: ActionTypes.SetButton, button })
)
let withinPanelButtonRef = useSyncRefs(internalButtonRef, ref)

// TODO: Revisit when handling Tab/Shift+Tab when using Portal's
let activeElementRef = useRef<Element | null>(null)
Expand Down Expand Up @@ -468,17 +470,19 @@ let Button = forwardRefWithAs(function Button<TTag extends ElementType = typeof
[state]
)

let type = useResolveButtonType(props, internalButtonRef)
let passthroughProps = props
let propsWeControl = isWithinPanel
? {
type: 'button',
ref: withinPanelButtonRef,
type,
onKeyDown: handleKeyDown,
onClick: handleClick,
}
: {
ref: buttonRef,
id: state.buttonId,
type: 'button',
type,
'aria-expanded': props.disabled ? undefined : state.popoverState === PopoverStates.Open,
'aria-controls': state.panel ? state.panelId : undefined,
onKeyDown: handleKeyDown,
Expand Down
Loading

0 comments on commit ab614c1

Please sign in to comment.