-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
Conversation
This pull request is being automatically deployed with Vercel (learn more). headlessui-react – ./packages/@headlessui-react🔍 Inspect: https://vercel.com/tailwindlabs/headlessui-react/fve7yzhiz headlessui-vue – ./packages/@headlessui-vue🔍 Inspect: https://vercel.com/tailwindlabs/headlessui-vue/3piwm1rch |
And if we are in a disabled fieldset, double check that we are not in the first legend. Because in that case we are visually outside of the fieldset and according to the spec those elements should **not** be considered disabled. Fixes: #194
b39759a
to
bf15fcd
Compare
} | ||
|
||
let isParentDisabled = parent?.getAttribute('disabled') === '' ?? false | ||
if (isParentDisabled && isFirstLegend(legend)) return false |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- 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).
- 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.
And if we are in a disabled fieldset, double check that we are not in
the first legend. Because in that case we are visually outside of the
fieldset and according to the spec those elements should not be
considered disabled.
Fixes: #194