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.

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

Switch thumb not disabled when enclosed in a disabled <fieldset> #194

Closed
ulken opened this issue Jan 11, 2021 · 10 comments · Fixed by #202
Closed

Switch thumb not disabled when enclosed in a disabled <fieldset> #194

ulken opened this issue Jan 11, 2021 · 10 comments · Fixed by #202

Comments

@ulken
Copy link
Contributor

ulken commented Jan 11, 2021

Problem

Switch thumb not disabled when enclosed in a disabled <fieldset>.

Demo

Cause

All form fields (<input>, <button> etc.) inside a <fieldset> are disabled. This results in the wrapping <button>, representing the track, being disabled. However, the <span> representing the thumb is not, as it's not considered a form field. Thus the Switch is still toggleable.

Workaround

Explicitly add disabled to <button>.

Solution

Not really sure what would be a clean solution of determining if enclosed in a (disabled) <fieldset>. Unfortunately the disabled property of the <button> is not set. Walking up the DOM is one way. Any other suggestions?

Anyway, I would assume the entire Switch to be disabled in this case.

@RobinMalfait
Copy link
Member

Hey! Thank you for your bug report!
Much appreciated! 🙏

This is a very interesting issue. I see what is happening and I think there might be a good solution here. Long story short if you would press the label or the gray area you will notice that it doesn't work.

Apparently, something I didn't know, is that if you have an element like a button with an onClick in a disabled fieldset, then the onClick will not be fired. That's the reason why it works as expected when clicking on the label or the gray area. Because the underlying button is now disabled.

Wow, while I was investigating this, I came up with this example: https://codesandbox.io/s/angry-wing-k4lif?file=/src/App.js
Notice that you can't click the button text, but you can click the text that is inside the span inside the button.

I re-created this in vanillajs and there it works as expected... https://codesandbox.io/s/currying-cache-mtso0?file=/src/index.js

I believe that we found a React bug!


Seems to be reported before: facebook/react#7711

@RobinMalfait
Copy link
Member

I commented on the open issue on the React repo. Hopefully we can figure something out.

In the mean time, one of the temporary fixes would be to add a disabled prop to your button as you mentioned. Another potential temporary fix could be something like this:

button:disabled * {
  pointer-events: none;
}

It's more like a global fix everywhere, and once that is fixed inside React you should be able to drop this.

If React will not fix it, then I'll think about a fix inside Headless UI itself!

@ulken
Copy link
Contributor Author

ulken commented Jan 17, 2021

I believe that we found a React bug!


Seems to be reported before: facebook/react#7711

Well, what do you know, huh? That's what you get for using synthetic events, aye? A re-implementation is tricky to get right...

Nice digging. Hope they finally address it. How much time will you give them before taking things in your own hands?

The only part I disagree on is:

> That's the reason why it works as expected when clicking on the label or the gray area.

I'd say the opposite. That's unexpected behavior.

Nevermind me. You're absolutely right.

@ulken
Copy link
Contributor Author

ulken commented Jan 17, 2021

Another potential temporary fix could be something like this:

button:disabled * {

  pointer-events: none;

}

This won't work with the label though, right?

... but that's already handled by the <fieldset>.

@RobinMalfait
Copy link
Member

RobinMalfait commented Jan 19, 2021

I'm going to try and implement a relatively easy fix inside Headless UI itself this Friday. I also saw that they answered on the React repo so maybe things will go quick enough. We'll see.

RobinMalfait added a commit that referenced this issue Jan 22, 2021
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
RobinMalfait added a commit that referenced this issue Jan 22, 2021
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
RobinMalfait added a commit that referenced this issue Jan 22, 2021
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
@ulken
Copy link
Contributor Author

ulken commented Jan 22, 2021

Sweet! I’ll make sure to try it out.

@ulken
Copy link
Contributor Author

ulken commented Jan 22, 2021

Hum, running npm i @headlessui/react@dev doesn't install a new version. Care to update?

@RobinMalfait
Copy link
Member

@ulken Sorry about that, I didn't publish it back then. Can you retry?

@ulken
Copy link
Contributor Author

ulken commented Jan 29, 2021

No worries. Yes, it works now. Thanks!

@imclint21
Copy link

Any update about how to disabled a switch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants