[ObjectPage]: Renders all sections regardless which one is active

[TinaAngeloski]

Bug Description

The ObjectPage renders all ObjectPageSections regardless which sections is active. In our case, we have an ObjectPage with three section, and at the beginning the user sees only the first section. Our app breaks, because in the background it tries to render and load also the other sections which are not visible to the user.

Versions:
The bug happened after we updated to @ui5/webcomponents: ^1.22.0 and @ui5/webcomponents-react: ^1.25.1. Before it was working with versions @ui5/webcomponents: 1.21.2 and @ui5/webcomponents-react: 1.24.0.

Note: The same started happening to FlexibleColumnLayout, and this is the related issue

Affected Component

ObjectPage, FlexibleColumnLayout

Expected Behaviour

It should render only the visible sections as before the update.

Isolated Example

https://stackblitz.com/edit/github-acayej

Steps to Reproduce

Create an object page with multiple sections in tab version. Even if the first one is only visible to the user and selected, all other sections are loaded in the background.

As shown in the isolated example.

Log Output, Stack Trace or Screenshots

Priority

High

UI5 Web Components Version

@ui5/webcomponents-base: ^1.22.0 and @ui5/webcomponents-react: ^1.25.1

Browser

Chrome

Operating System

No response

Additional Context

No response

Organization

SAP Signavio

Declaration

• I’m not disclosing any internal or sensitive information.

[ilhan007]

Hi colleagues,
it's a new behaviour that the author experiences after an upgrade to newer @ui5/webcomponents: ^1.22.0 and @ui5/webcomponents-react: ^1.25.1 versions.

In this issue, the affected component is the ObjectPage and there is similar issue for the FCL, that I did not transfer, but probably the root cause would be the same.

My first thought is an application problem, but could take a look?

[Lukas742]

Hi @TinaAngeloski

I wasn't able to reproduce the issue with the given StackBlitz example. The ObjectPage still only renders the current selected section in IconTabBar mode and there's no error thrown. (see video) Could you please verify if the example is really showing the bug? If so, please add information about the browser and OS used.

2024-03-13_14-06-43.mp4

[TinaAngeloski]

Hi, maybe we should have made it more descriptive. If you stay on 'section 1' the text should say 'Am a really section one?' but, instead the text comes from the 'section 2' useEffect, which changes the text to 'Am I really section two?' after 3 seconds... which means section 2 gets rendered too.
So our goal was to prove that 'section 2' is rendered despite not being selected or visible, which wasn't the case before the update.

[Lukas742]

Thanks for the clarification :)

The linked PR will fix this issue.

However, I would recommend not relying on side-effects too much, since this can lead to undesired behavior like in this case. E.g. just because a component has no representation in the DOM or its DOM elements aren't visible, doesn't mean that it's not mounted and therefore registered in the React fiber tree. Using states and props to control behavior inside a React App is always preferable. You can find out more about it here: https://react.dev/learn/you-might-not-need-an-effect

[signavio-fghedina]

Thank you @Lukas742
it was just to make an effective quick example; about he mounting hints
you are right and there are many reasons for that, but I dont think this is the right place to discuss

What matters here is that 1.21.2 was definitely not considering columns which should not be part of the layout (the other case is similar) ...but from 1.22.0 they are ,
and if one does not know and it is not written anywhere , result are really bad.

Thanks God we have some tests in place and I'm surprised there are no tests preventing those changes to reach ui5-webcomponents-react prod env

Have fun 👋

[Lukas742]

Hi @signavio-fghedina, @TinaAngeloski

I forgot to mention that the error shown in the screenshot was still not reproducible for me with the given stackblitz example. So the linked PR will only fix mounting all children initially. If we should investigate this further, please add an isolated example that is showing the issue.

What matters here is that 1.21.2 was definitely not considering columns which should not be part of the layout (the SAP/ui5-webcomponents#8434 is similar) ...but from 1.22.0 they are ,
and if one does not know and it is not written anywhere , result are really bad.

This doesn't seem to be related to the ObjectPage but the FlexibleColumnLayout or am I missing something here? If so, please describe more clearly what problems you're facing.

[qscheitlin]

@Lukas742

Chiming in for a quick second as I'm also experiencing the same issue. I'm still attempting to isolate the issue, so I unfournately don't have a reproducible example to share yet. But I do believe it has to do with the ObjectPage and less with the FlexibleColumnLayout.

You can see in the attached video of our demo data that when the midColumn does not include an ObjectPage, no error occurs when I route to a new page (00:00 - 00:12 of first video). However, when an ObjectPage is added to the midColumn (00:00-00:08 of second video), the same error experienced by @signavio-fghedina occurs. This object page is of course a bit more dynamic which is why I'm still trying to isolate the issue.

The line setSectionSpacer(objectPage.getBoundingClientRect().bottom - tabContainerContainerRef.current.getBoundingClientRect().bottom - lastSubSectionOrSection.getBoundingClientRect().height - TAB_CONTAINER_HEADER_HEIGHT); is where the error originates from. Specifically, the tabContainerContainerRef.current is null at the time the error occurs. I know that this probably isn't that helpful, but I thought I'd at least share some of my thoughts as I continue to try and isolate the issue.

const tabContainerContainerRef = useRef(null);
  useEffect(() => {
    const objectPage = objectPageRef.current;
    const sections = objectPage.querySelectorAll('[id^="ObjectPageSection"]');
    const lastSection = sections[sections.length - 1];
    const observer = new ResizeObserver(([sectionElement]) => {
      const subSections = lastSection.querySelectorAll('[id^="ObjectPageSubSection"]');
      const lastSubSection = subSections[subSections.length - 1];
      const lastSubSectionOrSection = lastSubSection ?? sectionElement.target;
      if (currentTabModeSection && !lastSubSection || sections.length === 1 && !lastSubSection) {
        setSectionSpacer(0);
      } else {
       setSectionSpacer(objectPage.getBoundingClientRect().bottom - tabContainerContainerRef.current.getBoundingClientRect().bottom - lastSubSectionOrSection.getBoundingClientRect().height - TAB_CONTAINER_HEADER_HEIGHT);
      }
    });
    if (objectPage && lastSection) {
      observer.observe(lastSection, {
        box: 'border-box'
      });
    }
    return () => {
      observer.disconnect();
    };
  }, [headerCollapsed, topHeaderHeight, headerContentHeight, currentTabModeSection, children]);

NoObjectPage.mov

WithObjectPage.mov

[TinaAngeloski]

When you have complex applications its challenging to reproduce the same behaviour on a simple sandbox.
We too were not able to reproduce the bug in the sandbox, but I can confirm the exact same error reported above form @qscheitlin about
setSectionSpacer(objectPage.getBoundingClientRect().bottom - tabContainerContainerRef.current.getBoundingClientRect().bottom - lastSubSectionOrSection.getBoundingClientRect().height - TAB_CONTAINER_HEADER_HEIGHT);
@Lukas742 I saw that one of the changes in the release touched the ObjectPage in this https://github.com/SAP/ui5-webcomponents-react/pull/5430/files so maybe the bug is related to this change?

[Lukas742]

Thanks @qscheitlin

I tried a few things using your description and videos as guide, but unfortunately still wasn't able to reproduce the error. At first I thought it might be related to the unmount order of components, but the observer seems to be never be called before unmounting the component where the tabContainerContainerRef is registered to for me.
Another thing I've found is that we don't extract the current value of the tabContainerContainerRef inside the useEffect and therefore the reference can change between renders which could be the reason for this bug.

Since this is most probably related to the container element of the TabContainer not being defined at the time the setSectionSpacer is called, I will create a fix that will add a safeguard there and also make sure that the reference stays stable while in the useEffect. Hopefully this will fix the issue.

@TinaAngeloski I know it's hard to isolate bugs inside complex structures, but how can we ensure to fix an issue if we don't know its root cause? Structuring bug reports to provide as much information as possible is essential, especially when no reproducible example is given. As mentioned above, a fix is hopefully simple here, but this might not be the case for other issues. Sometimes, the root cause doesn't even lie within our repository, but in external dependencies or unsupported usage (not that I'm saying that's the case here). That's why it's common in other repositories for issues to be rejected if the problem cannot be reproduced and no detailed information is provided.

@Lukas742 I saw that one of the changes in the release touched the ObjectPage in this https://github.com/SAP/ui5-webcomponents-react/pull/5430/files so maybe the bug is related to this change?

Yes, this regression was most probably introduced with this commit.

Could you please provide us with these information so we can investigate further?

• @qscheitlin it seems like the error is thrown before you navigate to another page, correct? If so, could you please tell us what router you're using. @TinaAngeloski is this occurring for you as well when navigating, or does it throw the error in some other situation?
• How does the markup of your components look like? Especially I'm curious about the layout options you're using for the FlexibleColumnLayout and if you're using a placeholder of the ObjectPage at any point.
• Are you using conditional rendering for a column of the FlexibleColumnLayout, the ObjectPage or any of its subcomponents?

[Lukas742]

Hi all,

we've just released a snapshot version (0.0.0-ef04d01ba4) containing the linked fixes. If you find the time to do so, we would greatly appreciate it if you could test this version and let us know if the issues have been resolved. Additionally, please note that if you're using our chart package, you may encounter a peer dependency error during module installations, which can be disregarded.

[qscheitlin]

@Lukas742

I have tested the snapshot version (0.0.0-ef04d01ba4) and I did not observe any errors in the ObjectPage component. Thank you so much for your swift response and fix on this issue! It's very much appreciated!

From my side, I'm not sure if further investigation is needed, but here are the answers to your previous questions if they are still needed:

Question 1: @qscheitlin it seems like the error is thrown before you navigate to another page, correct? If so, could you please tell us what router you're using.

react-router-dom

Question 2: How does the markup of your components look like? Especially I'm curious about the layout options you're using for the FlexibleColumnLayout and if you're using a placeholder of the ObjectPage at any point.

For FlexibleColumnLayout, I allows users to switch between OneColumn layout, TwoColumnsMidExpanded layout, and MidColumnFullScreen layout.

I do not use the placeholder of the Object Page at any point, but some sections of the ObjectPage may have an IllustratedMessage displayed in instances of no data.

Question 3: Are you using conditional rendering for a column of the FlexibleColumnLayout, the ObjectPage or any of its subcomponents?

Currently, the midColumn isn't conditionally rendered. Rather, the contents of the midColumn are conditionally rendered based on whether data has been fetched. Also, some sections of the ObjectPage are conditionally rendered based on whether the ObjectPage is in display or edit mode.

[Lukas742]

@qscheitlin thanks a lot for testing and answering the questions!

I tried again to reproduce it, including routing, conditional rendering, etc. but still didn't get the app to throw.
However, I'm now pretty sure that the bug is related to the unstable reference within the useEffect, as this fuzzy behavior is typical for timing issues. Nevertheless, we will keep the safeguards in place just to be sure.

We will release the fix sometime next week and that is of course if @TinaAngeloski and @signavio-fghedina don't find that the bug is not fixed for them.

Here you can find the stackBlitz sandbox which I used trying to reproduce the issue in another env: https://stackblitz.com/edit/github-qshwnm?file=src%2FToDos.tsx

[signavio-fghedina]

thank you @Lukas742 and @qscheitlin ... actually yesterday I could not really dedicate more time to that, hope next time to be more enabled in the discussion.
have a nice day

[ui5-webcomponents-react-bot]

🎉 This issue has been resolved in version v1.26.1 🎉

The release is available on v1.26.1

Your semantic-release bot 📦🚀