Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

apply disabled fix when inside a disabled fieldset #202

Merged
merged 1 commit into from
Jan 22, 2021
Merged

apply disabled fix when inside a disabled fieldset #202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/@headlessui-react/src/components/menu/menu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { useId } from '../../hooks/use-id'
import { Keys } from '../keyboard'
import { Focus, calculateActiveIndex } from '../../utils/calculate-active-index'
import { resolvePropValue } from '../../utils/resolve-prop-value'
import { isDisabledReactIssue7711 } from '../../utils/bugs'

enum MenuStates {
Open,
Expand Down Expand Up @@ -413,7 +414,8 @@ function Item<TTag extends React.ElementType = typeof DEFAULT_ITEM_TAG>(
}, [bag, id])

const handleClick = React.useCallback(
(event: { preventDefault: Function }) => {
(event: React.MouseEvent) => {
if (isDisabledReactIssue7711(event.currentTarget)) return event.preventDefault()
if (disabled) return event.preventDefault()
dispatch({ type: ActionTypes.CloseMenu })
disposables().nextFrame(() => state.buttonRef.current?.focus({ preventScroll: true }))
Expand Down
2 changes: 2 additions & 0 deletions packages/@headlessui-react/src/components/switch/switch.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { render } from '../../utils/render'
import { useId } from '../../hooks/use-id'
import { Keys } from '../keyboard'
import { resolvePropValue } from '../../utils/resolve-prop-value'
import { isDisabledReactIssue7711 } from '../../utils/bugs'

type StateDefinition = {
switch: HTMLButtonElement | null
Expand Down Expand Up @@ -84,6 +85,7 @@ export function Switch<TTag extends React.ElementType = typeof DEFAULT_SWITCH_TA
const toggle = React.useCallback(() => onChange(!checked), [onChange, checked])
const handleClick = React.useCallback(
(event: React.MouseEvent) => {
if (isDisabledReactIssue7711(event.currentTarget)) return event.preventDefault()
event.preventDefault()
toggle()
},
Expand Down
30 changes: 30 additions & 0 deletions packages/@headlessui-react/src/utils/bugs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// See: https://github.com/facebook/react/issues/7711
// See: https://github.com/facebook/react/pull/20612
// See: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#concept-fe-disabled (2.)
export function isDisabledReactIssue7711(element: Element): boolean {
let parent = element.parentElement
let legend = null

while (parent && !(parent instanceof HTMLFieldSetElement)) {
if (parent instanceof HTMLLegendElement) legend = parent
parent = parent.parentElement
}

let isParentDisabled = parent?.getAttribute('disabled') === '' ?? false
if (isParentDisabled && isFirstLegend(legend)) return false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nitpick, but shouldn’t you return false if first legend no matter if in a disabled fieldset or not?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should result in the same behaviour:

if (isParentDisabled && isFirstLegend(legend)) return false
return isParentDisabled

Because, when the isParentDisabled is false, then the result will be false anyway. So at this point it doesn't really matter that we are in a legend or not. The edge case is that when the parent is disabled but we are in the first legend, then we should ignore that.

Here is a truth table:

isParentDisabled isFirstLegend Return Value Info
true true false Disabled, but first legend, so we can 'ignore' the disabled state.
true false true Disabled, but not the first legend. else case is executed which is the result of isParentDisabled.
false true false Not disabled. else case is executed which is the result of isParentDisabled.
false false false Not disabled. else case is executed which is the result of isParentDisabled.

I think that this is the correct behaviour 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for being unclear. I was on my phone with limited time (and UX...).

I wasn't implying it was incorrect (which is why I considered it a nitpick), merely that the code could possibly be slightly simplified/refactored to:

if (isFirstLegend(legend)) return false
return isParentDisabled

Rationale: if it's in the first legend it should always return false, no matter where it is in the DOM or if it's enclosing fieldset is disabled.

Running it by your truth table yields the same result, as far as I can tell.

Not saying you should change it necessarily, you might find the current implementation more clear. Just throwing it out there how I see it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yep, I see what you mean.

I tried to follow the wording of the spec (but it is not that readable in the spec (2.))

A form control is disabled if any of the following conditions are met:

  1. The element is a button, input, select, textarea, or form-associated custom element, and the disabled attribute is specified on this element (regardless of its value).
  2. The element is a descendant of a fieldset element whose disabled attribute is specified, and is not a descendant of that fieldset element's first legend element child, if any.

But in the end, I don't think it matters much. Ideally we remove the code once React itself is fixed.


return isParentDisabled
}

function isFirstLegend(element: HTMLLegendElement | null): boolean {
if (!element) return false

let previous = element.previousElementSibling

while (previous !== null) {
if (previous instanceof HTMLLegendElement) return false
previous = previous.previousElementSibling
}

return true
}