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

fix: added formDisabledCallback for formAssociated elements #5053

Closed
wants to merge 1 commit into from
Closed

fix: added formDisabledCallback for formAssociated elements #5053

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
21 changes: 20 additions & 1 deletion button/internal/button.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ export abstract class Button extends LitElement implements FormSubmitter {

protected override render() {
// Link buttons may not be disabled
const isDisabled = this.disabled && !this.href;
const isDisabled = this.shouldBeDisabled && !this.href;

const button = this.href ? literal`a` : literal`button`;
// Needed for closure conformance
Expand Down Expand Up @@ -164,4 +164,23 @@ export abstract class Button extends LitElement implements FormSubmitter {
private handleSlotChange() {
this.hasIcon = this.assignedIcons.length > 0;
}

/**
* Whether the element should be disabled either through its own `disabled` or
* its formDisabled.
*/
private get shouldBeDisabled() {
return this.disabled || this.formDisabled;
}

/**
* Whether the element is currently disabled due to form state.
*/
private formDisabled = false;

/** @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 •
edited
Loading

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!

this.requestUpdate();
}
}
27 changes: 23 additions & 4 deletions checkbox/internal/checkbox.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ export class Checkbox extends LitElement {
if (changed.has('checked') || changed.has('disabled') ||
changed.has('indeterminate')) {
this.prevChecked = changed.get('checked') ?? this.checked;
this.prevDisabled = changed.get('disabled') ?? this.disabled;
this.prevDisabled = changed.get('disabled') ?? this.shouldBeDisabled;
this.prevIndeterminate =
changed.get('indeterminate') ?? this.indeterminate;
}
Expand All @@ -218,7 +218,7 @@ export class Checkbox extends LitElement {
const isIndeterminate = this.indeterminate;

const containerClasses = classMap({
'disabled': this.disabled,
'disabled': this.shouldBeDisabled,
'selected': isChecked || isIndeterminate,
'unselected': !isChecked && !isIndeterminate,
'checked': isChecked,
Expand All @@ -240,7 +240,7 @@ export class Checkbox extends LitElement {
aria-checked=${isIndeterminate ? 'mixed' : nothing}
aria-label=${ariaLabel || nothing}
aria-invalid=${ariaInvalid || nothing}
?disabled=${this.disabled}
?disabled=${this.shouldBeDisabled}
?required=${this.required}
.indeterminate=${this.indeterminate}
.checked=${this.checked}
Expand All @@ -250,7 +250,7 @@ export class Checkbox extends LitElement {
<div class="outline"></div>
<div class="background"></div>
<md-focus-ring part="focus-ring" for="input"></md-focus-ring>
<md-ripple for="input" ?disabled=${this.disabled}></md-ripple>
<md-ripple for="input" ?disabled=${this.shouldBeDisabled}></md-ripple>
<svg class="icon" viewBox="0 0 18 18" aria-hidden="true">
<rect class="mark short" />
<rect class="mark long" />
Expand Down Expand Up @@ -315,4 +315,23 @@ export class Checkbox extends LitElement {
formStateRestoreCallback(state: string) {
this.checked = state === 'true';
}

/**
* Whether the element should be disabled either through its own `disabled` or
* its formDisabled.
*/
private get shouldBeDisabled() {
return this.disabled || this.formDisabled;
}

/**
* Whether the element is currently disabled due to form state.
*/
private formDisabled = false;

/** @private */
formDisabledCallback(formDisabled: boolean) {
this.formDisabled = formDisabled;
this.requestUpdate();
}
}
25 changes: 22 additions & 3 deletions iconbutton/internal/icon-button.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ export class IconButton extends LitElement implements FormSubmitter {
aria-haspopup="${!this.href && ariaHasPopup || nothing}"
aria-expanded="${!this.href && ariaExpanded || nothing}"
aria-pressed="${ariaPressedValue}"
?disabled="${!this.href && this.disabled}"
?disabled="${!this.href && this.shouldBeDisabled}"
@click="${this.handleClick}">
${this.renderFocusRing()}
${this.renderRipple()}
Expand Down Expand Up @@ -184,7 +184,7 @@ export class IconButton extends LitElement implements FormSubmitter {
private renderRipple() {
return html`<md-ripple
for=${this.href ? 'link' : nothing}
?disabled="${!this.href && this.disabled}"
?disabled="${!this.href && this.shouldBeDisabled}"
></md-ripple>`;
}

Expand All @@ -196,7 +196,7 @@ export class IconButton extends LitElement implements FormSubmitter {
private async handleClick(event: Event) {
// Allow the event to propagate
await 0;
if (!this.toggle || this.disabled || event.defaultPrevented) {
if (!this.toggle || this.shouldBeDisabled || event.defaultPrevented) {
return;
}

Expand All @@ -207,4 +207,23 @@ export class IconButton extends LitElement implements FormSubmitter {
// Additionally, native change event is not an InputEvent.
this.dispatchEvent(new Event('change', {bubbles: true}));
}

/**
* Whether the element should be disabled either through its own `disabled` or
* its formDisabled.
*/
private get shouldBeDisabled() {
return this.disabled || this.formDisabled;
}

/**
* Whether the element is currently disabled due to form state.
*/
private formDisabled = false;

/** @private */
formDisabledCallback(formDisabled: boolean) {
this.formDisabled = formDisabled;
this.requestUpdate();
}
}
25 changes: 22 additions & 3 deletions radio/internal/radio.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ export class Radio extends LitElement {
return html`
<div class="container ${classMap(classes)}" aria-hidden="true">
<md-ripple part="ripple" .control=${this}
?disabled=${this.disabled}></md-ripple>
?disabled=${this.shouldBeDisabled}></md-ripple>
<md-focus-ring part="focus-ring" .control=${this}></md-focus-ring>
<svg class="icon" viewBox="0 0 20 20">
<mask id="${this.maskId}">
Expand All @@ -132,7 +132,7 @@ export class Radio extends LitElement {
tabindex="-1"
.checked=${this.checked}
.value=${this.value}
?disabled=${this.disabled}
?disabled=${this.shouldBeDisabled}
>
</div>
`;
Expand All @@ -143,7 +143,7 @@ export class Radio extends LitElement {
}

private async handleClick(event: Event) {
if (this.disabled) {
if (this.shouldBeDisabled) {
return;
}

Expand Down Expand Up @@ -185,4 +185,23 @@ export class Radio extends LitElement {
formStateRestoreCallback(state: string) {
this.checked = state === 'true';
}

/**
* Whether the element should be disabled either through its own `disabled` or
* its formDisabled.
*/
private get shouldBeDisabled() {
return this.disabled || this.formDisabled;
}

/**
* Whether the element is currently disabled due to form state.
*/
private formDisabled = false;

/** @private */
formDisabledCallback(formDisabled: boolean) {
this.formDisabled = formDisabled;
this.requestUpdate();
}
}
27 changes: 23 additions & 4 deletions select/internal/select.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ export abstract class Select extends LitElement {

private getRenderClasses() {
return {
'disabled': this.disabled,
'disabled': this.shouldBeDisabled,
'error': this.error,
'open': this.open,
};
Expand All @@ -428,7 +428,7 @@ export abstract class Select extends LitElement {
role="combobox"
part="field"
id="field"
tabindex=${this.disabled ? '-1' : '0'}
tabindex=${this.shouldBeDisabled ? '-1' : '0'}
aria-label=${(this as ARIAMixinStrict).ariaLabel || nothing}
aria-describedby="description"
aria-expanded=${this.open ? 'true' : nothing}
Expand All @@ -437,7 +437,7 @@ export abstract class Select extends LitElement {
label=${this.label}
.focused=${this.focused || this.open}
.populated=${!!this.displayText}
.disabled=${this.disabled}
.disabled=${this.shouldBeDisabled}
.required=${this.required}
.error=${this.hasError}
?has-start=${this.hasLeadingIcon}
Expand Down Expand Up @@ -525,7 +525,7 @@ export abstract class Select extends LitElement {
* is closed.
*/
private handleKeydown(event: KeyboardEvent) {
if (this.open || this.disabled || !this.menu) {
if (this.open || this.shouldBeDisabled || !this.menu) {
return;
}

Expand Down Expand Up @@ -820,4 +820,23 @@ export abstract class Select extends LitElement {
formStateRestoreCallback(state: string) {
this.value = state;
}

/**
* Whether the element should be disabled either through its own `disabled` or
* its formDisabled.
*/
private get shouldBeDisabled() {
return this.disabled || this.formDisabled;
}

/**
* Whether the element is currently disabled due to form state.
*/
private formDisabled = false;

/** @private */
formDisabledCallback(formDisabled: boolean) {
this.formDisabled = formDisabled;
this.requestUpdate();
}
}
35 changes: 27 additions & 8 deletions slider/internal/slider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,8 +434,8 @@ export class Slider extends LitElement {

private renderHandle({start, hover, label}:
{start: boolean, hover: boolean, label: string}) {
const onTop = !this.disabled && start === this.startOnTop;
const isOverlapping = !this.disabled && this.handlesOverlapping;
const onTop = !this.shouldBeDisabled && start === this.startOnTop;
const isOverlapping = !this.shouldBeDisabled && this.handlesOverlapping;
const name = start ? 'start' : 'end';
return html`<div class="handle ${classMap({
[name]: true,
Expand All @@ -447,7 +447,7 @@ export class Slider extends LitElement {
${when(this.labeled, () => this.renderLabel(label))}
<md-focus-ring part="focus-ring" for=${name}></md-focus-ring>
<md-ripple for=${name} class=${name} ?disabled=${
this.disabled}></md-ripple>
this.shouldBeDisabled}></md-ripple>
</div>`;
}

Expand Down Expand Up @@ -478,7 +478,7 @@ export class Slider extends LitElement {
@input=${this.handleInput}
@change=${this.handleChange}
id=${name}
.disabled=${this.disabled}
.disabled=${this.shouldBeDisabled}
.min=${String(this.min)}
aria-valuemin=${ariaMin}
.max=${String(this.max)}
Expand Down Expand Up @@ -543,8 +543,8 @@ export class Slider extends LitElement {
// Since handle moves to pointer on down and there may not be a move,
// it needs to be considered hovered..
this.handleStartHover =
!this.disabled && isStart && Boolean(this.handleStart);
this.handleEndHover = !this.disabled && !isStart && Boolean(this.handleEnd);
!this.shouldBeDisabled && isStart && Boolean(this.handleStart);
this.handleEndHover = !this.shouldBeDisabled && !isStart && Boolean(this.handleEnd);
}

private async handleUp(event: PointerEvent) {
Expand Down Expand Up @@ -583,8 +583,8 @@ export class Slider extends LitElement {
* slider is updated.
*/
private handleMove(event: PointerEvent) {
this.handleStartHover = !this.disabled && inBounds(event, this.handleStart);
this.handleEndHover = !this.disabled && inBounds(event, this.handleEnd);
this.handleStartHover = !this.shouldBeDisabled && inBounds(event, this.handleStart);
this.handleEndHover = !this.shouldBeDisabled && inBounds(event, this.handleEnd);
}

private handleEnter(event: PointerEvent) {
Expand Down Expand Up @@ -735,6 +735,25 @@ export class Slider extends LitElement {
this.value = Number(state);
this.range = false;
}

/**
* Whether the element should be disabled either through its own `disabled` or
* its formDisabled.
*/
private get shouldBeDisabled() {
return this.disabled || this.formDisabled;
}

/**
* Whether the element is currently disabled due to form state.
*/
private formDisabled = false;

/** @private */
formDisabledCallback(formDisabled: boolean) {
this.formDisabled = formDisabled;
this.requestUpdate();
}
}

function inBounds({x, y}: PointerEvent, element?: HTMLElement|null) {
Expand Down
25 changes: 22 additions & 3 deletions switch/internal/switch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ export class Switch extends LitElement {
role="switch"
aria-label=${(this as ARIAMixin).ariaLabel || nothing}
?checked=${this.selected}
?disabled=${this.disabled}
?disabled=${this.shouldBeDisabled}
?required=${this.required}
@change=${this.handleChange}
>
Expand All @@ -239,7 +239,7 @@ export class Switch extends LitElement {
return {
'selected': this.selected,
'unselected': !this.selected,
'disabled': this.disabled,
'disabled': this.shouldBeDisabled,
};
}

Expand All @@ -250,7 +250,7 @@ export class Switch extends LitElement {
return html`
${this.renderTouchTarget()}
<span class="handle-container">
<md-ripple for="switch" ?disabled="${this.disabled}"></md-ripple>
<md-ripple for="switch" ?disabled="${this.shouldBeDisabled}"></md-ripple>
<span class="handle ${classMap(classes)}">
${this.shouldShowIcons() ? this.renderIcons() : html``}
</span>
Expand Down Expand Up @@ -345,4 +345,23 @@ export class Switch extends LitElement {
formStateRestoreCallback(state: string) {
this.selected = state === 'true';
}

/**
* Whether the element should be disabled either through its own `disabled` or
* its formDisabled.
*/
private get shouldBeDisabled() {
return this.disabled || this.formDisabled;
}

/**
* Whether the element is currently disabled due to form state.
*/
private formDisabled = false;

/** @private */
formDisabledCallback(formDisabled: boolean) {
this.formDisabled = formDisabled;
this.requestUpdate();
}
}
Loading
Loading