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

fix: added formDisabledCallback for formAssociated elements #5053

Closed
wants to merge 1 commit into from

Conversation

datvm
Copy link
Contributor

@datvm datvm commented Oct 3, 2023

Controls should be disabled when a parent fieldset has disabled=true. Fix #5049

image

@datvm
Copy link
Contributor Author

datvm commented Oct 3, 2023

Right now Radio and Slider are disabled inside disabled fieldset but their UIs are not updated. I found out their CSS is using [disabled]. It should be :disabled but I do not know how to edit them.

Also beside using a variable to track formDisabledCallback, we can also use this.matches(":disabled") instead.


/** @private */
formDisabledCallback(formDisabled: boolean) {
this.formDisabled = formDisabled;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need this property. You should be able to set this.disabled = formDisabled, which should also handle updating the UI correctly. Can you try that?

You also won't need this.requestUpdate() when setting this.disabled, since it's a @property

Copy link
Contributor Author

@datvm datvm Oct 3, 2023

Choose a reason for hiding this comment

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

I planned to do that but it would be a wrong behavior. If you set disabled of the control at formDisabledCallback, the control itself would be disabled.

So in case you have a fieldset with two controls, one is disabled (by itself) and one is not, when the fieldset disabled is lifted, one control should still be disabled:

<fieldset>
   <input disabled /> <!-- This should stay disabled if the fieldset is disabled and then enabled -->
   <button></button>
</fieldset>

Also standard elements (like input) still has its disabled property being false even when it's disabled by its parent. That's why I used an extra variable to keep track of the state. An alternative should be using .matches(":disabled").

Copy link
Collaborator

Choose a reason for hiding this comment

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

Gotcha, thanks for the explanation!

@asyncLiz asyncLiz changed the title Added formDisabledCallback for formAssociated elements. fix: added formDisabledCallback for formAssociated elements Oct 3, 2023
@asyncLiz
Copy link
Collaborator

asyncLiz commented Oct 5, 2023

FYI on this, we're working on mixin behaviors for shared functionality, including form associated mixins.

We may hold off on this PR for a bit while that's in development

@datvm
Copy link
Contributor Author

datvm commented Oct 5, 2023

@asyncLiz Yes no problem, when making this change I also thought this should be a common behavior for all controls that are formAssociated and should inherit from a common ancestor/mixin. I will close this PR for now then.

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 this pull request may close these issues.

use formDisabledCallback to support <fieldset> disabled attribute
2 participants