calebdwilliams / element-internals-polyfill

A polyfill for the element internals specification
MIT License
97 stars 30 forks source link

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

Open christophe-g opened 7 months ago

christophe-g commented 7 months ago

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 commented 5 months ago

@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);

  }
};
calebdwilliams commented 5 months ago

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