Skip to content

Commit

Permalink
Only add type=button for real buttons (#709)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
RobinMalfait authored Aug 2, 2021
1 parent 6b9ce87 commit 271c236
Show file tree
Hide file tree
Showing 15 changed files with 439 additions and 20 deletions.
62 changes: 61 additions & 1 deletion src/components/disclosure/disclosure.test.tsx
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
10 changes: 7 additions & 3 deletions src/components/disclosure/disclosure.tsx
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 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
3 changes: 2 additions & 1 deletion src/components/listbox/listbox.tsx
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 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 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
62 changes: 61 additions & 1 deletion src/components/popover/popover.test.tsx
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 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 271c236

Please sign in to comment.