calcite-value-list: Removing an item after re-ordering removes more than one item in certain cases

[jmanke]

Actual Behavior

Removing an item from the calcite-value-list removes more than one item.

Expected Behavior

Removing an item from the calcite-value-list should only remove the item selected to be removed.

Reproduction Steps and Sample

NOTE: This bug seems specific to Ember

Setup

1. Create a calcite-value-list with drag-enabled prop
2. Create a backing class with two items to be rendered as calcite-value-list-items (see code sample for example)
3. When list is re-orded or an item is removed, ensure backing data reflects the change

Repro Steps

1. Drag the second item into the first position of the list
2. Remove the last item in the list
3. Observe both items are removed from the list. However, the backing data is still correct.

Sample:

1. Ember Twiddle

Relevant Info

• Removing the item externally (preventing the item from being removed inside the calcite-value-list) still causes the bug to occur.
• I notice the calcite-value-list takes care of modifying the HTML when re-ordering. It would be nice if the calcite-value-list only emitted an event when it wants to be re-ordered.

Reproducible version: @esri/[email protected]

• [x] CDN
• [ ] NPM package

[jmanke]

This is a high priority issue for the Dashboards team 🙂

[Elijbet]

I also noticed that when you drag item 2 into the first position in the list and remove item 2, it won't remove that visually either. The data is still correct (item 1 is the only one in the list) for now, but if you move item 1 again, it just keeps adding it to the list. Deleting one of them removes all of them at once.

https://user-images.githubusercontent.com/19231036/159350790-4ec78f81-36e7-49c2-bcf3-b411e413bac2.mov

[jcfranco]

It seems like DOM modifications from SortableJS cause issues with Ember's rendering engine. This makes sense and is also the case with any framework or lib that manages the DOM.

Introducing a prop to skip DOM modifications seems like the best course of action here. This would apply to both:

• calcite-value-list
• calcite-sortable-list

I'll prototype something for this next week.

[jcfranco]

Moving to the Feb milestone as this will need more time for testing and will most likely introduce a new feature to address this (the current milestone is strictly for patches).

[jcfranco]

cc @geospatialem

[geospatialem]

Re-allocating to the upcoming March milestone.

[geospatialem]

Will evaluate a fix in the upcoming April milestone.

[jcfranco]

Sharing some way overdue findings:

• it seems the main issue is caused by Sortable's way of rearranging the DOM and this messing up Ember's rendering
• issues seem present in both Ember v3 and v4
• testing of a managed-sort option (undoing value-list DOM modifications and then emitting change events) still shows Ember state issues

@jmanke Could you share any workarounds you have to mitigate this issue and also let us know what the priority/impact of this is for you?

I propose bumping the estimate on this one and moving it to a future milestone. Hopefully, https://github.com/SortableJS/Sortable/issues/1931 can land by the time we revisit this.

cc @driskull – this could come up again in https://github.com/Esri/calcite-components/issues/6430

[jmanke]

Sharing some way overdue findings:

* it seems the main issue is caused by Sortable's way of rearranging the DOM and this messing up Ember's rendering

* issues seem present in both Ember v3 and v4

* testing of a managed-sort option (undoing value-list DOM modifications and then emitting change events) still shows Ember state issues

@jmanke Could you share any workarounds you have to mitigate this issue and also let us know what the priority/impact of this is for you?

I propose bumping the estimate on this one and moving it to a future milestone. Hopefully, SortableJS/Sortable#1931 can land by the time we revisit this.

cc @driskull – this could come up again in #6430

Thanks for looking into this @jcfranco! So far the only workaround is to remove the component from the DOM and add it back the next frame. While this is occurring, we display a loading indicator.

Since we have a workaround, the priority is low-medium. Waiting for a future milestone works for us.

[geospatialem]

Moved to the July milestone for evaluation and upped the time estimate, since it could impact other uses across components.

[jcfranco]

@geospatialem We might need to push this further as there hasn't been any updates on the PR referenced above. 🥲

[geospatialem]

Moved to the October milestone, which will evaluate if:

1. This issue is still valid with the deprecated component and parity of list via #6430, and
2. If the Sortable and Ember conflict can be mitigated

[geospatialem]

Spike to determine if the following above is true, and if we should proceed, or close out the above issue.

[jcfranco]

There hasn't been any updates on Sortable regarding this. Unless there are any objections, I'm going to mark this as blocked and we'll revisit once activity resumes on the linked PR above.

[github-actions[bot]]

cc @geospatialem, @brittneytewks