`formDisabledCallback` should always fire when governing fieldset's disabled state changes

[bennypowers]

Consider the case of <x-button>, a form-associated custom element:

customElements.define('x-button', class XButton extends HTMLElement {
  static formAssociated = true;
  static observedAttributes = ['disabled'];
  get disabled() { return this.hasAttribute('disabled'); }
  set disabled(b) { this.toggleAttribute('disabled', !!b); }
  constructor() { /* add shadow button */ }
  attributeChangedCallback(_, __, newV) {
    this.disabled = newV != null;
  }
  formDisabledCallback(disabled) {
    this.disabled = disabled;
  }
});

Like most custom element component sets do, x-button observes it's disabled attribute, and reflects it's disabled DOM property to it's attribute. (Shoelace, FAST, Material, to name a few examples).

Should the user place this button in a fieldset, they should expect that whenever the fieldset's disabled state toggles, so does the button's:

<form>
  <fieldset>
    <x-button>OK</x-button>
  </fieldset>
</form>

But instead what will happen is that when first setting the disabled attr on the fieldset, the button will disable, but thereafter, changes to the fieldset's disabled attribute will have no effect on the button.

See this demo page which illustrates the problem

Both Mozilla and Google have asserted that their implementations are aligned with the spec. I could argue that the spec leaves room for interpretation, but in any even suggest that the spec should require formDisabledCallback to fire when the fieldset's state changes, regardless of the presence of the disabled attribute on x-button. The spec as is would cause many existing CE's to break if they became FACEs.

[domenic]

(Can you link to the browser bugs you mention, where Chrome and Firefox engineers are claiming they match the spec?)

There seems to be a very bad spec bug here, which is that there is no normative spec text saying to fire formDisabledCallback at all. (Except on upgrades.) There is informative text in the "Custom element reactions" intro, but nothing normative that actually calls "enqueue a custom element reaction" appropriately. Given this, it's easy to see why implementations might not have gotten this right.

To fix this, we should add attribute change steps for the disabled element, which checks:

• If the element is a FACE, and it previously had no disabled attribute but now does, or vice versa, it should enqueue the reaction.
• If the element is a fieldset element, and it previously had no disabled attribute but now does, or vice versa, it should enqueue the reaction for all its FACE descendants in tree order. (Unsure if this should be shadow-including or not; https://html.spec.whatwg.org/#concept-fe-disabled clause 2 is not precise. Maybe that needs tests too.)

[bennypowers]

bugzilla: https://bugzilla.mozilla.org/show_bug.cgi?id=1793559 chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=1370956 and webkit's preliminary work is linked above.

Thanks for the attention on this.

[domenic]

Oh, I see the issue. There is still a spec bug, but indeed your button is written in a buggy way, and maybe there is no Firefox or Chrome bug.

The issue is your button actually sets the disabled="" attribute whenever its parent fieldset becomes disabled. This is not how native form controls behave; they have a disabled state, which is derived from a combination of the fieldset and their own disabled="" attribute. They don't sprout or remove disabled="" attributes when their fieldset changes.

Sprouting a disabled="" attribute will indeed cause the problem here, because now the actual disabled state is not changed by changing the fieldset; the disabled="" attribute has taken precedence.

So, you should not use formDisabledCallback() to modify the disabled="" attribute. You should let users modify that, if they want to override the fieldset behavior.

[bennypowers]

ok but that's true of almost every custom form control currently published, so they're all holding it wrong?

[domenic]

Sounds like it! Sprouting attributes is never a good idea.

[domenic]

The cases you list don't seem to be using formDisabledCallback in this way:

• https://github.com/shoelace-style/shoelace/blob/next/src/components/button/button.ts
• https://github.com/microsoft/fast/blob/master/packages/web-components/fast-foundation/src/button/button.ts
• https://github.com/material-components/material-web/blob/1b2bea04875769ac7700a80736f35da6810b19b9/button/lib/button.ts

(no matches for Ctrl+F "formDisabledCallback" in any of them)

[bennypowers]

the reason i cited those cases is because it's become the norm for custom elements with a disabled DOM property to reflect that property to a boolean attribute. This is a well-established cowpath with years or development behind it.

According to this, now, when those libraries move to implement FACE, they will have to keep in mind that disabled is a special case and doesn't not follow the usual rules of custom element development. This will be confusing, and the ability to :host([disabled]) will disappear per spec for FACEs.

[domenic]

the reason i cited those cases is because it's become the norm for custom elements with a disabled DOM property to reflect that property to a boolean attribute. This is a well-established cowpath with years or development behind it.

Yes. You just don't use formDisabledCallback to do that. You don't use attributeChangedCallback either! You do it like you do every other reflected property/attribute pair: using the getter and setter. There's nothing special or exceptional about disabled.

Your example code just has redundant stuff all over the place, in an attempt to implement the reflection in three different ways. Setting the disabled property causes the disabled="" attribute to be set which causes attributeChangedCallback() and formDisabledCallback() to fire, which causes disabled to be set which causes disabled="" to be modified again for no reason. (Thankfully it's not an infinite loop because attributeChangedCallback() and formDisabledCallback() won't fire the second time.)

There's no need to change anything when moving to FACE. Just don't abuse formDisabledCallback() for something it isn't meant for. (Implementing attribute reflection.)

the ability to :host([disabled]) will disappear per spec for FACEs.

If you're attempting to find out whether the element has the disabled="" attribute, this still works fine.

If you're attempting to find out whether the element is disabled, then you should use :disabled instead. Just like on regular form controls, these are two different things.

[radium-v]

The cases you list don't seem to be using formDisabledCallback in this way:

• https://github.com/shoelace-style/shoelace/blob/next/src/components/button/button.ts
• https://github.com/microsoft/fast/blob/master/packages/web-components/fast-foundation/src/button/button.ts
• https://github.com/material-components/material-web/blob/1b2bea04875769ac7700a80736f35da6810b19b9/button/lib/button.ts

(no matches for Ctrl+F "formDisabledCallback" in any of them)

For FAST Foundation, the formDisabledCallback is present in the base mixin class FormAssociated, located here: https://github.com/microsoft/fast/blob/master/packages/web-components/fast-foundation/src/form-associated/form-associated.ts#L581

[claviska]

If you're attempting to find out whether the element is disabled, then you should use :disabled instead. Just like on regular form controls, these are two different things.

I'd be perfectly happy with this if we had a :state() selector for custom states. Alas, we don't and it's not uncommon for custom elements to have custom states where valid, invalid, and disabled aren't appropriate. As a stop gap, Shoelace and many other libraries reflect attributes. I understand this is an arguable practice, but short of reflecting attributes or abusing parts for state, we don't have any other options.

What would you recommend as a best practice for custom states? Is there something else in the works for that?

[domenic]

I'll mark that comment and this one as off-topic, since it's not related to the bug reported here, but we do have a custom state API: https://wicg.github.io/custom-state-pseudo-class/#customstateset

[p-bakker]

@domenic you quoted disabled state above. The second condition in that part of the spec states that a form control ought to be disabled when 'the element is a descendant of a fieldset element'

In this context, would 'descendants' also have to include formAssociated custom elements that are slotted (in other custom elements)?

I have this structure: customElementA.shadowRoot.div.fieldset.slot[face1.shadowRoot.div.section.slot[span.face2]]

Both the face1 and face2 implementations call this.attachInternals() in their constructor, have the static formAssociated property set to true and have a formDisabledCallback.

Toggling the disabled attribute on the fieldset does NOT trigger the formDisabledCallback method on either the slotted face1 instance nor the nested slotted face2 instance

Am wondering if the current behavior I'm observing is expected behavior or not Or maybe I'm missing something in my impl or maybe the spec doesn't clearly specified how things ought to behave in this case. At least FF, GC & ME all seem to behave in the same way