Closable tabs issues

[RSantosGIS]

Actual Behavior

edit: please see conversation for description of additional issue with navigation between tabs also breaking, related to this same root cause.

I started using the closable tabs recently and overall I think it's great work. I observed a few issues during my time getting things setup (this is in React):

• When you close the active tab, the active tab automatically changes (as intended) but this does not fire the onCalciteTabsActivate or onCalciteTabChange events for CalciteTabTitle and CalciteTabNav, respectively
• In my app, the number of tabs is dynamically generated based on a state value. As you would expect, when a tab gets closed I also update the state. This causes a re-render event which actually eliminates the closed CalciteTabTitle and CalciteTab from the dom. However, the internal behavior of CalciteTabs is to just set a 'closed' attribute and hide the tab, not actually unmount it. This leads to an uncontrolled exception when the parent CalciteTabs component (which does not re-render) attempts to access a property on what is now a non-existent array index in tab.js:

Expected Behavior

• events should fire for the active tab changing when the active tab is the one closed
• I recommend adding handling to assume that a tab may no longer exist. You can fix the screenshot I sent above just by doing the following (I tested it by modifying the code in the console):

tabIds = tabDomIndexes.reduce((ids, indexInDOM, registryIndex) => {
        if (this.tabs[registryIndex]) {
          ids[indexInDOM] = this.tabs[registryIndex].id;
        }
        return ids;
      }, []);

Reproduction Sample

ArcGIS Knowledge Studio
https://codesandbox.io/s/summer-mountain-y3jcf3?file=/src/App.js

Reproduction Steps

To reproduce in the sandbox updated to 1.5.1:

1. Select tab labeled "Select Me"
2. Close the tab labeled "... then close me"
3. After closing a tab, you'll get the error message referenced in the original issue message
4. After closing the error message, you will see that you cannot navigate to the tab to the right (123) until your first re-click on "Select Me"

To reproduce in Studio:

For the events:

• checkout Knowledge Studio out of our internal repo
• cd /studio
• npm install
• Open the file MainDisplayZone.tsx
• add a console log into an event handler for onCalciteTabsActivate or onCalciteTabChange on CalciteTabTitle or CalciteTabNav
• npm start
• open the developer console
• close the active tab
• note that the event handlers do not fire

For the null access error:

• checkout Knowledge Studio out of our internal repo
• cd /studio
• npm install
• Open the file RootLayoutZone.tsx
• comment out the line "node_modules/@esri/calcite-components/dist/components/tabs.js?:113:52":
  "Cannot read properties of undefined (reading 'id')" under "SUPPRESSED_PROMISE_ERRORS"
• npm start
• open the developer console
• close any tab

Reproduction Version

1.4.2

Relevant Info

No response

Regression?

No response

Priority impact

p3 - want for upcoming milestone

Impact

I can work around the lack of events firing using other handlers, and we are currently just suppressing the error from the null access. Because of the way the code is written where the error is occurring inside a series of async functions that are called for each tab index, the fact that the call fails with an uncontrolled exception for the tab that disappeared from the DOM doesn't actually break anything for us... I think. Obviously it makes me nervous 😄

Esri team

ArcGIS Knowledge

[geospatialem]

@RSantosGIS Could you put together a code sample for us to troubleshoot using a CodeSandbox Calcite and React template?

[RSantosGIS]

@geospatialem Unfortunately, I cannot right now. We are in the home stretch to code freeze for Studio and the best I can do is write up Calcite issues when I find them in the course of our work. I'd be happy to hop on a call to walk someone through it if it's helpful.

[RSantosGIS]

@geospatialem This is become a bigger issue for us because there appears to be other bugged behavior associated with closing a tab when it is part of a dynamically generated tab list from state. I made a sandbox that shows a simple case of things breaking here, and will add it to the issue: https://codesandbox.io/s/suspicious-elion-hk8tgf?file=/src/App.js https://codesandbox.io/s/funny-sound-6vn45x?file=/src/App.js

[RSantosGIS]

I'm also now observing behavior where if I close a tab to the left of the active tab, the user cannot navigate to the tab immediately to the right of the active tab without first clicking on some other tab. There appears to be some kind of hidden index tracking inside tabs that is getting messed up. So for example, if your active tab is in slot 4 and you close a tab in slot 2, the active tab moves into slot 3 and you cannot now open the tab in slot 4.

Unfortunately, I cannot produce a sandbox for this beyond the one I already made because the first error I reported already crashes the sample. But I believe it is almost certainly also the same root cause - tab closing doesn't know how to properly handle the tab also being removed from the DOM

[mpriour]

I found a couple of issues in the sandbox that where making the behavior strange.
Here are my corrections that work https://codesandbox.io/s/upbeat-fast-yfwn23

1. .splice does NOT return the modified array but the deleted items, so even if everything had re-rendered as expected, the onTabClose function was setting the new tab config state to the deleted item and not the updated array.
2. You should never directly modify non-primitive state objects even if it is to immediately set the state to the newly modified object / array / etc.... There are some new array methods that are now supported or will be fully supported very soon that will make this easier. However, for now you need to at least make a shallow copy of the array before modifying it and then set the state to the modified copy.
3. The REAL workaround / solution. Add a key parameter to the <CalciteTabs> component that is unique to the configured tabs. In this example, I simply joined the tab title strings together. The reason you need to do this is that you are modifying the internal parts of a web component that the web component cares about and tracks. Therefore you need to instruct react to fully re-render the web component itself. Without the key on the parent web component element, you are getting the web component internal state and the dom state out of sync and then things don't work anymore. I've had this issue with other web components that keep track of their children via slot attributes. Changing a child does not reset the parent's state of trigger a re-render of the parent directly.

[RSantosGIS]

Hey @mpriour , thanks for taking a look at this. Summarizing our conversation on teams and adding a little more:

• First, thank you for catching the problems with the array in my sample. When I found the bug with navigation this morning I was eager to get something to you guys and I should have been more careful.
• Unfortunately, adding a key to the CalciteTabs parent component isn't a solution that works for us. This is because using a new key (or any other prop) to re-render the parent also triggers a full re-render of each child tab even if none of its props have changed, which in turn causes map views inside the tab to regenerate. Things like pan/zoom are lost (because they aren't tracked by framework state), layers have to be reloaded, and any changes made to the map using map tools that occurred without the user saving them are also lost.
• I did find a goofy workaround that if I click on the active tab title after closing a tab to the left, it appears that Calcite resets itself and properly syncs the new indexes and I can then navigate to the tab immediately to the right. I can have our app programmatically click the active tab title every time a tab is closed, but obviously that doesn't feel great. edit: workaround doesn't work consistently

I still feel like this is something you should take a look at. I would expect when closing the tab that no part of the CalciteTabs component would still rely on the tab existing internally. The fact that if you close a tab and then it disappears from the DOM the breaks the navigation for the tab immediately to the right of the active tab still feels like a bug, because a full re-render of the component to resync everything causes you to lose capabilities.

[RSantosGIS]

Updated my sample with Matt's suggested change, and it still demonstrates both problems.
Repro steps:

• select tab labeled "Select Me"
• Close the tab labeled "... then close me"
• After closing a tab, you'll get the error message referenced in the original issue message
• After closing the error message, you will see that you cannot navigate to the tab to the right (123) until your first re-click on "Select Me"

[jcfranco]

@RSantosGIS Could you update the repro case or the steps above to make sure they're in sync? The latest sample looks as follows:

Just making sure we're reproducing accurately.

[RSantosGIS]

@jcfranco ugh, no it's not. I will do it again and update

[RSantosGIS]

@jcfranco finally got around to updating this, sorry I've been slammed with Studio's end of iteration. https://codesandbox.io/s/funny-sound-6vn45x?file=/src/App.js

Please let me know if the example or anything about what we are trying to achieve is unclear

[RSantosGIS]

@mpriour suggested that changing CalciteTab before CalciteTabTitle resolves the null access error, and I confirmed that indeed it does. However, it does not resolve the navigation problem:
navigate to tab 3 of 4
close tab 2
attempting to navigate to the tab that now occupies slot 3 (since they all moved one to the left) does not work

Still, it might be worth changing the docs in the interim to at least recommend the order change.

[geospatialem]

Updated the repro case to remove the extraneous components in the sandbox and updated to 1.5.1. Triaging will occur in the next few weeks.

[eriklharper]

This issue isn't specific to React: https://codepen.io/eriklharper/pen/JjqXKxO

[github-actions]

Installed and assigned for verification.

[geospatialem]

Verified with https://codepen.io/eriklharper/pen/JjqXKxO in 2.11.0-next.21. 🎉