openui5 icon indicating copy to clipboard operation
openui5 copied to clipboard

Published 20 hours ago • verified publisher icon SAP

sap.m.HeaderContainer: onAfterRendering is triggered twice

Open boghyon opened this issue 4 years ago • 5 comments

  • Sample 1 (JSBin): https://jsbin.com/luforov/edit?html,js,console We can see that the HeaderContainer is being rendered twice. Replace that HeaderContainer with a different control (e.g. sap.m.Text). The control is rendered once as expected.
  • Sample 2 (More realistically): https://embed.plnkr.co/ZhSMIE9qPna831Us?show=view/Home.view.xml,view/Next.view.xml,preview:?sap-ui-xx-componentPreload=off Press Navigate >. There, unlike the first page ("Home") which also contains the HeaderContainer, the HeaderContainer in the "Next" view bubbles invalidation up to the view, causing the entire view to render twice.

Any other information? (attach screenshot if possible)

I assume this is caused by invalidating _prevButton and _nextButton aggregations within onBeforeRendering of the HeaderContainer: https://github.com/SAP/openui5/blob/e9145a4edfe57b4125d712d067b99743ab03ccb6/src/sap.m/src/sap/m/HeaderContainer.js#L342-L356

I remember reading somewhere that controls shouldn't do anything in onBeforeRendering that would trigger invalidation. But that's exactly what is happening here.

Update: due to https://github.com/SAP/openui5/commit/cc9349495feaff9c2d9a8bf8a26631cf87a9b57a, the issue is no longer reproducible in the latest UI5 release. The samples are updated to run with the previous OpenUI5 version 1.91.0 in order to reproduce the issue again.

boghyon avatar May 12 '21 19:05 boghyon

This PR would fix this issue: https://github.com/SAP/openui5/pull/3251.

boghyon avatar May 12 '21 19:05 boghyon

It is explicitly allowed to modify composite parts in onBeforeRendering, that's one of the main purposes of that hook. When the control and its parts are finally rendered, RenderManager notifies UIArea about the rendering of the parts and the invalidation flag is cleared for them -> no 2nd rendering should be needed.

The problem in the snippet rather is twofold

  • the HeaderContainer is added as a direct child of the UIArea. This marks the whole UIArea as invalid (as its content aggregation was modified). The invalidation bookkeeping in this case is quite coarse granular and the mentioned notification does not work as intended
  • the document body is used as UIArea. Not sure why this plays a role, but when I tried to address point one, still 2 renderings happened. Only when moving the UIArea one level down into a separate div, then the notification worked and only one rendering happened - despite the invalidation in onBeforeRendering

I leave it up to the control owners whether they accept the PR #3251. But in general I tend to get rid of any calls that use the bSuppressInvalidate flag in favour of the DOM patching.

@"dispatcher of the week" can you please create two incidents, one for this issue here (and we'll check whether the notification between RenderManager and UIArea can be made working in this special case) and a second issue for the PR #3251 ?

codeworrior avatar May 12 '21 20:05 codeworrior

Hello @boghyon @codeworrior ,

Thank you for sharing this finding. I've created an internal incident 2180175524. The status of the issue will be updated here in GitHub.

Regards, Tsanislav

tsanislavgatev avatar May 25 '21 08:05 tsanislavgatev

So... What do we do with this issue now? The change at https://github.com/SAP/openui5/commit/cc9349495feaff9c2d9a8bf8a26631cf87a9b57a copied my PR and "fixed" the issue. But the actual issue is not related to sap.m.HeaderContainer only as mentioned at https://github.com/SAP/openui5/issues/3250#issuecomment-840080020.

Here is sample 3 using a simple notepad control: https://embed.plnkr.co/eHaCt7OwVhhCDBzj?show=control%2FSquare.js,preview. But again, the UIArea there is also the <body>.

@codeworrior

Only when moving the UIArea one level down into a separate div, then the notification worked and only one rendering happened

In the more-realistic sample 2, after clicking Navigate >, the navigated view is rendered twice due to invalidation in the HeaderContainer, even though its UIArea is neither the <body> nor HeaderContainer's direct parent. I haven't dug enough to find the cause yet.

Any progress so far in the internal incident(s)?

boghyon avatar Jun 27 '21 11:06 boghyon

I've created another internal incident 2270050972 covering the general part of the UIArea

flovogt avatar Mar 31 '22 06:03 flovogt