element-internals-polyfill icon indicating copy to clipboard operation
element-internals-polyfill copied to clipboard

polyfill not robust for elements that do not call `attachInternals` (e.g. -buttons)

Open christophe-g opened this issue 11 months ago • 2 comments

Hey - thanks a lot for this polyfill !

I run into a situation where elements that have a formAssociated = true , but do not call attachInternals in their constructor.

For example: https://github.com/material-components/material-web/blob/c3d303eaac1e38e48ea138e0eec034bffdc84c4b/button/internal/button.ts#L42

The polyfill breaks when those elements are attached to the DOM and call upgradeInternals :

	const upgradeInternals = (ref) => {
			if (ref.constructor['formAssociated']) {
					const internals = internalsMap.get(ref);
					const { labels, form } = internals;
					initLabels(ref, labels);
					initForm(ref, form, internals);
			}
	};

as internals is undefined in this case, the app throws with: Uncaught TypeError: Cannot destructure property 'labels' of 'internals' as it is undefined.

Those elements are then not rendered.

christophe-g avatar Mar 01 '24 14:03 christophe-g

@calebdwilliams - I can submit a PR for this, just tell me if it has any chances of being be reviewed.

It basically rewrites upgradeInternals as

export const upgradeInternals = (ref: ICustomElement) => {
  if (ref.constructor['formAssociated']) {
    let internals = internalsMap.get(ref);
    // we might have cases where the internals are not set
    if (internals === undefined) {
      console.warn('ElementInternals missing from the element', ref);
      ref.attachInternals();
      internals = internalsMap.get(ref);
    }
    const { labels, form } = internals;
    initLabels(ref, labels);
    initForm(ref, form, internals);

  }
};

christophe-g avatar May 02 '24 14:05 christophe-g

Yeah, I’ll look at it. Submit a PR and make sure there’s a regression test.

calebdwilliams avatar May 03 '24 13:05 calebdwilliams