TabContainer: multiple issues with nested tabs and selection

[Lukas742]

Bug Description

• Programatically selecting tabs doesn't reset the currently selected tab.
• Programatically selecting nested tabs isn't changing the selected tab indicator (blue underscore)
• getTabInStripDomRef returns undefined for subTabs

Expected Behavior

• When adding/removing or changing the selected value, the TabContainer should reset all previously selected tabs.
• Programatically selecting nested tabs should work
• getTabInStripDomRef should always return the element or it should be outlined, that it's only working for non nested tabs.

Steps to Reproduce

1. Go to codeSandbox
2. Select Tab 3 by clicking on the corresponding button --> Tab 1 & 3 are selected
3. Refresh the page to have a clean state
4. Select Tab 2.2 by clicking on the corresponding button
   1. The Content of the TabContainer didn't change
   2. Tab 2 is not selected
5. Open the popover of Tab 2 --> The list item of Tab 2.2 is selected
6. Close the popover --> Now Tab 1 & 2 are selected
7. Press "Unselect Tab 2.2" --> Tab 1 & 2 are still selected
8. Open the popover again --> Tab 2.2 is not selected
9. Close the popover w/o clicking on a list item --> Tab 2 is not selected anymore as well
10. Click the Dom Ref button --> see that only for Tab 3 the element is logged, for Tab 2.2 the returned value is undefined

Isolated Example

https://codesandbox.io/s/ui5-webcomponents-forked-05992l?file=/src/index.js

Context

• UI5 Web Components version: 1.3.0

Priority

• [ ] Low
• [ ] Medium
• [x] High
• [ ] Very High

[C3ntraX]

Hello @Lukas742,

I found another issue.

If you wrap a TabContainer in a TabContainer and you will select a tab in the second TabContainer, than you will get onTabSelect event from both TabContainer.

Additional, the Typescript definitions for the event are not correct. Currently: (property) onTabSelect?: ((event: Ui5CustomEvent<HTMLElement, { tab: React.ReactNode; tabIndex: number; }>) => void) | undefined

But I think the tab type ReactNode is not correct? Can you check this? Because it should be someing like Tab as HTMLElement? Because I could get the Id from the tab, to distinguish the tabs in the TabContainer event onTabSelect. With the Type ReactNode itself, you couln´t do anything else, than casting the ReactNode to type HtmlElement or ignore it.

Steps to Reproduce

1. Go to https://codesandbox.io/s/p3tli7?file=/src/App.js
2. Click on "SubTab2"
3. Open console and see, that both TabContainer triggered the event

[Lukas742]

Hi @C3ntraX

you are right, normally the respective dom ref should be passed there. Unfortunately this is currently not possible, as there is no information given by the web component which element is expected there, so we'll need to discuss how this can be achieved first. (probably the easiest way would be imo to replace all HTMLElement types with the respective component type like ITab) In the meantime I'm going to replace all ReactNode typings with HTMLElement as ReactNode really makes no sense at all. You can find the PR here.

[C3ntraX]

@Lukas742

thanks for the change :) Can you check the issue with the tabcontainer as well?

[Lukas742]

I'd guess that nesting TabContainers is not supported, but let's rather wait for an answer from a member of the UI5 web components team, as I am not part of their team and only raised the issues I discovered here.

[unazko]

Hello @SAP/ui5-webcomponents-topic-rd,

The issues are reproducible as described by @Lukas742 and @C3ntraX via the provided code snippets. I've labelled this one as a component enhancement as there are a lot of separate issue, which would be hard to track in a single bug request.

Best Regards, Boyan Rakilovski

[olannyv]

These fixes will be handled with https://jira.tools.sap/browse/BGSOFUIRODOPI-2811 backlog item

[georgimkv]

Hello @Lukas742 and @C3ntraX, I'll address the raised issues below:

1. Programatically selecting tabs doesn't reset the currently selected tab.
2. Programatically selecting nested tabs isn't changing the selected tab indicator (blue underscore).

I think this is, in part, a bug that #5449 will solve, but also, another part of this is a misunderstanding of what how HTML elements function and what should be expected when using web components.

The tab container should not remove the selected property of the other tabs when another tab becomes selected programatically. There is no concept in HTML of one element changing another element. As with any other HTML element, the does not alter the markup that it is defined with. If the ui5-tabcontainer mutated any elements other than itself, this will end up with a changed document structure and frameworks that rely on declarative UI will suddenly have deviating output from what they declared or expected.

Here's an example component declared with React that should illustrate the point:

  function Tabs() {
    return (
      <Ui5TabContainer>
        <Ui5Tab selected={true} text={"Always selected"} />
        <Ui5Tab text={"Tab 2"} />
        <Ui5Tab text={"Tab 3"} />
      </Ui5TabContainer>
    )
  }

Now I click on Tab 2. Consider if you are using a framework such as React that has clearly defined what the markup should be, but the ui5-tabcontainer and its ui5-tabs have made these changes without going through the "happy path" of React. The proposed behavior of the ui5-tabcontainer to internally change the tabs' selected property will happen without the framework's knowledge. No state was mutated, no components were re-rendered. This will result in DOM differing from what the Virtual DOM last remembers, because this didn't go through the rendering lifecycle.

If you want to programatically set a tab as selected, then you have to make sure that the other tabs are de-selected yourself. Because otherwise you will end up with two tabs being marked as selected. Managing which is the currently selected tab, and making sure there is ever only one selected tab at a time is a concern of the application that uses web components.

3. getTabInStripDomRef returns undefined for subTabs

Tabs being placed in the subTabs slot do not get rendered in the "tab strip". Because of that, I think this is not a bug. I will add a clarification in the Tab' documentation that will make this more clear.

4. If you wrap a TabContainer in a TabContainer and you will select a tab in the second TabContainer, than you will get onTabSelect event from both TabContainer.

This is quite normal behavior - it is how events in the browser work. Once fired, all events bubble to the root of the document. You can click on a <div> inside a <div> inside another <div> and still listen for that click event all the way on the body element. So, if you have a TabContainer within a TabContainer, be sure to check the relatedTarget property on that event to track which element was actually clicked.

Let me know what you think.

[C3ntraX]

Hello @gmkv and @Lukas742 ,

"The proposed behavior of the ui5-tabcontainer to internally change the tabs' selected property will happen without the framework's knowledge. No state was mutated, no components were re-rendered. This will result in DOM differing from what the Virtual DOM last remembers, because this didn't go through the rendering lifecycle."

Exactly this problem we have with react. Many components this framework offers are not controllable with react. This causes the DOM to differing from what the Virtual DOM described (React markup). If I have the control of a component, than I have to set the selected tab (only one), this shouldn´t be done internally. See below, how popular ui-librarys handels the problem. I have some issues with this, see: #5210, #4883 and https://github.com/SAP/ui5-webcomponents-react/issues/2928.

https://codesandbox.io/s/scrollabletabsbuttonprevent-demo-material-ui-forked-xw8jh0?file=/demo.js:0-898 extracted from https://mui.com/material-ui/react-tabs/#main-content

This codesandbox offers the way, how to avoid many problems with tabs:

A tab within a tabcontext has no selected property, this helps with the problems:

1. A tabcontext cann´t have multiple selected tabs, because the tabcontext manages the selected tab
2. If the user clicks on a tab, the tabcontext fires an event, with which index is requested. The eventhandler now sets the new value in the tabcontext, which selects the new tab. If you don´t want to change the tab, than you don´t have to (this would solve: #5210). This is called controlled state.
3. Currently there is a unclear behaviour with TabContainer, because all tabs will get mounted, regardless of which tab is selected... Only the selected tab should be mounted, but this is unclear. To solve this, you have to conditional render the tab, if the index is selected. But the component could internally selects another tab, if the user clicked it, without the knowlege of react. This could intensify the stated problem without controllable components above. See https://mui.com/material-ui/react-tabs/#main-content Basic tabs for reference.

With this in mind, you should rethink your current component and maybe there is an breaking change necessary.

The other thing: "This is quite normal behavior - it is how events in the browser work. Once fired, all events bubble to the root of the document." Yes I understand. But this wasn´t clear for me, that your library using event bubbling like plain HTML components.

[vladitasev]

Hello,

I'd like to comment on the architecture and philosophy of UI5 Web Components regarding state and events.

1. Generally, state is owned by the app (properties, slots).
2. However, some properties can change due to user interaction. Such are ui5-input's value, ui5-checkbox's checked, and also ui5-tab's selected.

There are generally 2 approaches to handling such properties А. As UI5 Web Components currently work: when the user interacts, we change the property internally, and always notify the app by firing an event. Most of these events can be prevented, which gives the app the opportunity to create custom conditions when the user can interact with a component. B. We never ever change state, even due to user interaction. We only fire events and it's up to the app to set the state to the new values, requested by the user.

Solution B. is more "pure" and works really well if you're using a framework such as React, and you subscribe to the events and update the model. Then the model reflects the changes in the DOM.

However, there are 2 reasons we've settled on our current approach (A)

• This is how standard HTML elements work
• This makes it much easier to develop if you're not using a framework ,and you don't have a model. If we had decided for solution B, a lot of extra work would be needed to create even simple things - such as inputting text, etc... Instead, we tried to be as close to pure HTML elements as possible.

Therefore, if you're using React, you must indeed bind selected={condition} for each tab, so once the model updates, each tab will get the new value for its selected property. It can be a bit "ugly" if you hard-coded the tabs in the HTML, but usually the tabs are created in a loop, and then there is only one line inside the tab-container tag and there you can bind the selected condition.

Now to the other question, raised in this thread: why such API? Why have selected property for each tab individually, instead of selectedIndex on the tab container itself. The reason is again: to be as close to standard HTML as possible. Having a selected property/attribute makes the DOM easier to understand and is more declarative. The React wrappers may choose to implement another API on top of ours, but the components themselves, again, try to be as close to pure HTML elements as possible.

I think making the tab change event preventable makes sense, so this is something we should do for the next patch.

Regards, Vladi

[C3ntraX]

Hello togehter,

yes I understand your points and agree with them.

"I think making the tab change event preventable makes sense, so this is something we should do for the next patch." For the react wrapper adding an additional property makes the api more complex and shouldn´t be the way, how to develop a react app. What you have pointed out is, that solution "B" is the standard react way to keep the state in sync.

My point is, there is maybe a way to simplify the react components if we are using approch "B" for react while keeping the approch "A" for the standard HTML elements.

So, you could add the additional property for the standard and in react you could use the property to make the component controllable with the TabContainer with the use of the mentioned selectedTab (or value like MUI) property. If the react markup sets the value in selectedTab, then the TabContainer is in controlled state and will use the preventable event property. @Lukas742 and @MarcusNotheis

This approch is easier for react development and will keep the way how standard HTML elements work.

What do you think? I mean, the apis will differ, but react has his own storybook, so this shouldn´t be the problem.

[Lukas742]

Hi all!

@gmkv

I think this is, in part, a bug that #5449 will solve,

Thanks, I think this was the original issue I've encountered when trying to replace our custom nested tab implementation from the ObjectPage with the default ui5-tab-container.

but also, another part of this is a misunderstanding of what how HTML elements function and what should be expected when using web components.

You are right, looking again at the example I posted, it doesn't really make sense to implement it like this. I tested it again and except for the above mentioned issue, it's working as expected. (ui5-webcomponents example, wcr example) Thanks for pointing this out!

@vladitasev

Thank you as well for the explanation. I also feel like option A is the way to go with web components. For React and other frameworks it would really be a great help if all events that change the components state, could allow disabling this behavior, so the component is fully controllable. I opened an issue (#4883) some time ago that only mentions the default behavior of selection-change to be preventable. Should I edit and extend it so it covers all events that are somehow mutating internal properties?

@C3ntraX

To implement such behavior we would need to extend the components, and maybe even the web components, which would result in a tremendous increase of work for maintaining the components and also updating them to a new version. Also, it makes it much harder to identify the root of bugs and could also introduce a new source where bugs could be generated. In my opinion, what also speaks strongly against changing the web components is the someday coming React support for web components. When React supports web components, there is no need for our wrapper implementation anymore and if we changed too much there, the migration to "pure" web components would not be possible without having to rewrite a lot of code. After all we are "just" a wrapper for ui5 web components. We try to make the use of events, booleans and slots more like it is with React but under the hood we render the default web component. Except for the onChange discrepancy, which we are currently looking into (https://github.com/SAP/ui5-webcomponents-react/issues/3180), we don't plan to change any APIs of internal implementations of the web components.

[C3ntraX]

Hello @videlov and @Lukas742,

if the tab change event is preventable, will the event still fire? Because I need to be noticed, if the user wants to change the Tab. With preventable events, the inner event handling should only preventable, but should still fire.

Do we mean the same?

[georgimkv]

The changed event 'tab-select' will still be fired, but the new addition is that you will be able to prevent it.

[C3ntraX]

This means, that I´ll add the new addition. This prevents the inner event handling, that the tab will not selected but the event will still fire? If this is the case, then I can set the selected tab manually, if the user clicked another tab.

If the event will not fire, the problem will be still there and nothing has changed for this case. Because this fix should also be a fix for #5210.

[georgimkv]

The interaction will the the following:

1. User is on "tab 1".
2. They click on another tab.
3. tab-select event is fired.
4. You can call event.preventDefault() and the tab will not be changed. 4.1. Optionally, since you manage the state (using React and what we discussed above), you could change the selected tab by walking through all tabs and setting the property or attribute selected on the tab that the user should be shown.

I have opened a PR #5449 that should address both issues, but it's not ready to be reviewed yet because it proves tricky when combined with other features from the TabContainer component. I will have more time to work on it next week as I am currently occupied with other tasks. I appreciate your prompt responses and patience. Thanks!

[Lukas742]

Hi colleagues,

just a quick question if the issue @C3ntraX mentioned here is still on the radar as I didn't find it in the JIRA backlog item. Also, I created a codeSandbox using plain ui5 web components to reproduce it, the steps are the same as mentioned below.

I found another issue.

If you wrap a TabContainer in a TabContainer and you will select a tab in the second TabContainer, than you will get onTabSelect event from both TabContainer.

(...)

Steps to Reproduce

1. Go to https://codesandbox.io/s/p3tli7?file=/src/App.js
2. Click on "SubTab2"
3. Open console and see, that both TabContainer triggered the event

[dimovpetar]

Hi colleagues,

just a quick question if the issue @C3ntraX mentioned here is still on the radar as I didn't find it in the JIRA backlog item. Also, I created a codeSandbox using plain ui5 web components to reproduce it, the steps are the same as mentioned below.

I found another issue. If you wrap a TabContainer in a TabContainer and you will select a tab in the second TabContainer, than you will get onTabSelect event from both TabContainer. (...) Steps to Reproduce

1. Go to https://codesandbox.io/s/p3tli7?file=/src/App.js
2. Click on "SubTab2"
3. Open console and see, that both TabContainer triggered the event

Hello @Lukas742 ,

This is expected behavior of the events, explained here in the Bubbling and capturing article. The result will be the same if you assign an event listener for this event on any parent element - document.documentElement.addEventListener("tab-select", console.log);. You can always find out which is the target of the event using event.target. For other issues please open separate incidents.

Best regards, Petar

[Lukas742]

Hi @dimovpetar

Thanks for the answer, I tried preventing the bubbling with event.StopPropagation and for some reason it didn't work the first time I tried it, so I used the target approach. After your answer I double checked, and now it's working.

https://codesandbox.io/s/ui5-webcomponents-forked-25rwk6?file=/src/index.js:308-331

Sorry for the confusion.

[dimovpetar]

Hello @Lukas742 ,

The tab-select event is now preventable as discussed above, thanks to #5661. Also the rest of the issues should be fixed with https://github.com/SAP/ui5-webcomponents/pull/5449.

Best regards, Petar