Calcite-components: Bug: calcite-stepper does not work with React DOM manipulation

[noahmulfinger]

Summary

When using calcite-stepper (with layout="horizontal") in a React component, React will throw an error any time you add/remove DOM elements. I'm guessing this is because of the way calcite-stepper moves DOM elements around depending on the active step.

Actual Behavior

React will throw the error Node.insertBefore: Child to insert before is not a child of this node whenever you update the DOM inside a calcite-stepper. Here's a repro case in this codesandbox: https://codesandbox.io/s/nifty-ardinghelli-28ry7?file=/src/App.js

Expected Behavior

The calcite-stepper should work fine with React. It might require using making steps hidden instead of moving their contents into the active slot.

Reproduction Steps

1. Open https://codesandbox.io/s/nifty-ardinghelli-28ry7?file=/src/App.js
2. Hit the Toggle button
3. You will see the Node.insertBefore error.

[macandcheese]

To confirm, this only happens in "horizontal" not "vertical" layout, correct?

[noahmulfinger]

@macandcheese Interesting, yeah only horizontal.

[macandcheese]

"vertical" doesn't do anything with slotted content, just shows and hides what's inside the stepper-item. "Horizontal" basically functions as tabs and moves slotted content from the stepper item "up" to a single container in the parent calcite-stepper. It seems like this causes more hassle than is needed, and there seem to be a lot of issues with React as we've seen recently. There's probably an easier way. We might be able to try and totally re-think that.

[driskull]

Looking at the stepper code, it could use quite a bit of cleanup. I see a lot of nodes being created without JSX and some IE specific testing going on.

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[macandcheese]

Valid

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[github-actions[bot]]

This issue has been automatically closed due to inactivity.

[noahmulfinger]

Reopening since this is still an issue.

Note: On the developers site, we worked around this by putting the React code _below_ the stepper instead of _inside_ it. It just requires keeping the state of the stepper and the React DOM in sync manually.

[driskull]

@jcfranco @paulcpederson @macandcheese @julio8a I think we need to revisit this component and it's API.

We could just have a property for currentPosition called step and get rid of the methods and internal events going on.

The updateContent() method seems pretty hacky.

Would we want to do this pre 1.0?

[macandcheese]

I think maybe @eriklharper had looked at this (?) - I know it works fine in vertical mode, the bug is in the "tab" functionality in horizontal mode for React since it's (I think?) doing what label does and creating / removing Nodes after it's mounted in React

[jcfranco]

moves slotted content from the stepper item "up" to a single container in the parent calcite-stepper

We should not be manipulating nodes from the light DOM. This workflow will break in all VDOM libs since they assume their VDOM tree is the source of truth.

We could just have a property for currentPosition called step and get rid of the methods and internal events going on.

Would we ever need to support running checks/code between steps (e.g., validation)? Having a prop might make that tricker to implement.

[eriklharper]

I addressed this issue related to stepper a while back https://github.com/Esri/calcite-components/issues/1245. The way I resolved that was to refactor how I was rendering the <input> inside calcite-radio-button. Basically I converted radio-button from a shadowed to a scoped component, which allowed me to render the <input> in JSX as opposed to doing it manually with the DOM API. My guess is that this line is party to the bug because this was the exact way I used to render the input in radio-button when I was seeing weird issues.

There are lots of funky things that happen when rendering calcite components with vdom libraries if the calcite components manually move DOM outside of the standard Stencil JSX rendering. We should probably make an effort to deprecate any place where we're doing this kind of manual DOM manipulation in favor of alternate approaches. My guess is that this issue might not just be specific to React, but possibly other react-like libraries like Preact as well.

[macandcheese]

Can we just use tabs internally for horizontal stepper content?

[driskull]

Would we ever need to support running checks/code between steps (e.g., validation)? Having a prop might make that tricker to implement.

Good point.

[paulcpederson]

I don't think you need to move dom around to have both of these modes work the same with the same markup. Here's an example with CSS Grid:

https://codepen.io/paulcp/pen/OJbYEVq

Basically you can set the items to be pass through and position the title vs content where you need it on the grid without moving things around. Just show/hide the active step rather than ripping it out and dumping it somewhere else. Would this work?

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[github-actions[bot]]

This issue has been automatically closed due to inactivity.

[caripizza]

@driskull @jcfranco @julio8a should this one be assigned to a dev?

[julio8a]

This one might need to be added to the next sprint.

[driskull]

Should we have a wiki or readme file for 'areas of responsibility' with a table that has each component listed along side a lead dev, backup dev, PE and designer?

[julio8a]

Let's talk about this ^

[eriklharper]

Sounds like a good CC meeting agenda item.