fix(elements): Ensure ForOF out of bound row gets deleted before moving to opposite side.#17199
fix(elements): Ensure ForOF out of bound row gets deleted before moving to opposite side.#17199
Conversation
skrustev
added
❌ status: awaiting-test
There was a problem hiding this comment.
Pull request overview
Fixes a virtualization edge-case in IgxForOfDirective that can lead to incorrect DOM/view recycling when scrolling (notably reproducible in the “Elements” scenario described in #17179), resulting in incorrect scrollbar sizing/empty space.
Changes:
- Add null-safety for recycled embedded views when shifting/popping from the internal view cache.
- Force a view refresh (
detectChanges()) after updating template context and before reinserting the recycled view into theViewContainerRef.
skrustev
changed the title
|
|
||
| // Because in Elements the whole parent div (containing data-index) gets removed (possibly due to being disconnected). In Angular it just gets moved. | ||
| // This ensures to update it with the new context and remove it first from DOM because of detach action before inserting it manually. | ||
| view.detectChanges(); |
There was a problem hiding this comment.
Note: In order to minimize this change since it is problematic so far only in Elements, this can be an option that can be enabled for the ForOf and be enabled only for the HGrid on Elements.
There was a problem hiding this comment.
IMO from a performance perspective this is fine, since this is only for small scrolls where a single view is moved and re-rendered. So an additional check should not cause a performance degradation.
Closes #17179
Additional information (check all that apply):
Checklist:
feature/README.MDupdates for the feature docsREADME.MDCHANGELOG.MDupdates for newly added functionalityng updatemigrations for the breaking changes (migrations guidelines)Guide to reproducing it consistently:
--
Performance shouldn't be affected since the update of context now happens before moving the node instead of after. Quick test for HGrid with all rows expanded. The first test is longer due to scrolling to unknown expanded rows:
Old:
Long Tasks:16, Average Long Task Time: 1177.3125
Long Tasks:31, Average Long Task Time: 544.0645161290323
Long Tasks:24, Average Long Task Time: 559.375
With fix:
Long Tasks:16, Average Long Task Time: 1161.4375
Long Tasks:33, Average Long Task Time: 556.6363636363636
Long Tasks:21, Average Long Task Time: 527.0952380952381