DE54776 - Prevent entity disposal if element is being reordered

[semibran]

DE54776: Rubric cells can become unaligned after adding a new rubric level

Required fix for https://github.com/Brightspace/d2l-rubric/pull/2957 -- this will help us avoid using _getEntity and clean up a bunch of the shadow root queries

When reordering criteria using lit repeat, elements are disconnected and immediately reconnected to the DOM in their new positions. Currently the associated entities are being disposed of as part of EntityMixinLit.disconnectedCallback which breaks entity watchers thus introducing a whole slew of rubrics bugs. While this is presented as a rubrics-specific issue it could theoretically occur in any part of the lms where lit repeat is used with reordering elements using this mixin.

There is no feasible way to prevent EntityMixinLit.disconnectedCallback from being called lower in the inheritance chain. We just have to make sure the disposal logic doesn't fire if the elements aren't actually being permanently disconnected.

[github-actions[bot]]

PR Checklist:

Did you use a semver keyword in a commit message?

• [ ] Yes I used one in at least one of my commits
• [x] No but I will use a semver keyword in the commit message when I merge and squash

Did you paste the Rally US URL in the description?

• [x] Yes
• [ ] No as there's no corresponding Rally US

[jesseag]

Does anyone foresee any scenarios where other consumers of the entity-watcher-mixin are counting on entities being disposed even though they are still connected to the DOM? My only thought is that this could break others if they are counting on that scenario. It could be a breaking change but I'm really hoping that it isn't.

I could also see this introducing a bug where we end up having to try to dispose entities twice because the node is not disconnected from the DOM yet. Anyways, it's just more a thought/sanity check. I understand that if disconnectedCallback() is running then ideally we should be getting rid of the these entity watchers except in the case that we are handling here.

[semibran]

Does anyone foresee any scenarios where other consumers of the entity-watcher-mixin are counting on entities being disposed even though they are still connected to the DOM?

I think from a systems design perspective we would want entity listeners to remain intact as long as their DOM elements are still connected. Reordering is just an extreme corner case so ideally this change should be invisible to consumers

I could also see this introducing a bug where we end up having to try to dispose entities twice because the node is not disconnected from the DOM yet.

Not entirely sure I understand the scenario you're describing but (1) entity disposal is idempotent and (2) if order of operations re: connection/disconnection were messed up that'd strike me as a browser issue and not something we'd have to work around

[jesseag]

Does anyone foresee any scenarios where other consumers of the entity-watcher-mixin are counting on entities being disposed even though they are still connected to the DOM?

I think from a systems design perspective we would want entity listeners to remain intact as long as their DOM elements are still connected. Reordering is just an extreme corner case so ideally this change should be invisible to consumers

I could also see this introducing a bug where we end up having to try to dispose entities twice because the node is not disconnected from the DOM yet.

Not entirely sure I understand the scenario you're describing but (1) entity disposal is idempotent and (2) if order of operations re: connection/disconnection were messed up that'd strike me as a browser issue and not something we'd have to work around

Okay. There are 2 scenarios here which came to mind. 1) The repeat directive is used and we go through this code flow. In this instance, this.isConnected = true so we return early. This scenario works just fine. 2) Let's say the repeat directive is not used and we are simply deleting a criterion. In this scenario, I just want to ensure that this.isConnected is always false which will dispose of the entity correctly. If this is the case, then I am fine with it. My concern was if we couldn't guarantee that this.isConnected is always equal to false.

[d2l-github-release-tokens[bot]]

:tada: This PR is included in version 2.116.0 :tada:

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot :package::rocket: