We will need additional logic here to remove any listeners from a previously attached element in case the trigger button changes dynamically and one is already set at that time. We should write a test case for this scenario as well.

This part of the "should update old and new triggers on trigger change" test will test the event listeners being removed when the trigger element changes:

trigger1.click();
expect(expansionPanel.open).to.be.true;

expansionPanel.trigger = 'button-id2';
expect(trigger1.getAttribute('aria-controls')).to.be.null;
expect(trigger1.getAttribute('aria-expanded')).to.be.null;
trigger1.click();
expect(expansionPanel.open).to.be.true; //expansion panel stays open when old trigger is clicked

May be worth adding a note about ensuring to control the existence of the trigger button and the panel in the DOM at the same time to avoid any hard to debug issues when it comes to async/incremental rendering.

These types of scenarios are why we may want to consider adding a triggerElement property to allow devs to set the instance directly in cases where timing can be an issue.

Eventually we can invert this process by using the Invoker Commands API with a custom command so that the button will instead notify the panel rather than the other way around which will simplify things a lot.

It looks like this is unused, let's go ahead and remove it.

We may want to handle the scenario where the trigger element is detached from the DOM dynamically and release our internal reference to it each time this method runs by checking if the trigger element ref is not connected.

I honestly wouldn't even bother throwing an exception in this scenario... I think if someone is choosing the more explicit triggerElement property to control it then it should take precedence IMO. It should be very rare if someone uses these two together, and it's probably ok to assume the element reference is of higher priority. I could be convinced otherwise though if there's good reason to throw.

Sounds good to me!

Other components may have this same issue, but we should instead get the root node to ensure it is scoped to the specific DOM tree given the potential for other shadow root boundaries (if we were to use the expansion panel within the shadow of another element then it wouldn't work):

const triggerEl = this._component.getRootNode().getElementById(id);

The reason I have document here and not getRootNode() is because when getRootNode() gets called after component disconnect, you get a rootNode.getElementById is not a function error, since getRootNode() just finds the component itself (I guess because at this point the component is disconnected from the document).

The difference between expansion panel trigger and tooltip anchor in this regard is that expansion panel needs to do something with the trigger element (removing the event listener) on destroy. Unfortunately there isn't something like a beforeDisconnectCallback() hook.

What do you think about:

const triggerEl = deepQuerySelectorAll(document.documentElement, `#${id}`)[0] as HTMLElement;

This solves the shadow root problem you brought up, but maybe raises performance issues?

Or, maybe a solution is to set a reference to the root node on init and use that instead of this._component.getRootNode() dynamically? Is it a safe assumption (or rare enough to be ok) that the component's root node won't have changed since init?

If this code attempts to run while the element is disconnected I would just not do anything. It will be updated when it's connected/disconnected anyway. This is why this pattern is very finicky and can introduce memory leaks when holding onto element references that are not owned by the element itself. Once the element is disconnected from the DOM we need to drop the references (set them to undefined so the GC can clean them up) and then things like this just won't do anything until place in the DOM.

Forgot to mention the other thing the expansion panel is doing when it disconnects is removing the ARIA attributes from the trigger button.

So your suggestion is to leave const triggerEl = document.getElementById(id); as is but add this._adapter.triggerElementRef = undefined to destroy() in core? And there will just still be that limitation with the shadow root boundaries.

Remembering now the shadow root boundary issue is kind of a no-go. It makes the trigger feature unusable when the expansion panel is used within another component's shadow root. What do you think about this:

  private _tryLocateTriggerElement(id: string): HTMLElement | null {
    if (id) {
      if (this._component.getRootNode() !== this._component) {
        return (this._component.getRootNode() as Document | ShadowRoot).getElementById(id);
      } else {
        return deepQuerySelectorAll(document.documentElement, `#${id}`)[0] as HTMLElement;
      }
    } else {
      return null;
    }
  }

It limits the less performant deepQuerySelectorAll call just to when the component disconnects.

We should use assignedElements({ flatten: true }) here instead to ensure that it handles composed slot elements (slots acting as passthrough slots on potential parent elements).

Thanks, it looks like I misunderstood how flatten worked. I thought if the slotted element was for example a <forge-scaffold>, then assignedElements({ flatten: true })[0] was going to return what was in the header slot of <forge-scaffold>, but I see now that's not how it works.