Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug(drag-drop): Layout thrashing (forced layout reflow) when dragging between dropzones with 400+ complexed drag refs #25990

Open
1 task
perjerz opened this issue Nov 16, 2022 · 4 comments
Labels
area: cdk/drag-drop needs investigation A member of the team needs to do further investigation to determine the root cause P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent perf This issue is related to performance

Comments

@perjerz
Copy link

perjerz commented Nov 16, 2022

Is this a regression?

  • Yes, this behavior used to work in the previous version

The previous version in which this bug was not present was

No response

Description

Problem

The layout thrashing problem occurs when the user drag the complex element between drop zones.

image

image

There are 42 drop zones.

image

There are 400+ drag refs.

image

The drag refs are quite a bit complex. We have several kind of drag Refs. For example, we have a text component which including Text Editor (Quill) inside.

image

However, we think the number of components and drag ref complexity of root element are not the main reason for layout thrashing.

In the investigation, there are two cases make layout thrashing when dragging between drop zones.

The first is in extendStyles in _toggleNativeDragInteractions in _startDragSequence, and then call _cacheItemPositions for loop each item in a drop zone which call getMutableClientRect which call getBoundingClientRect.

image

image

image

It extends styles from toggleVisibility (WRITE), insert placeholder, in body this._document.body.appendChild(parent.replaceChild(placeholder, element)); Line no. 826 (WRITE) and getBoundingClientRect in _cacheItemPositions (READ).

Suggested Solution

  1. _cacheItemsPositions, getBoundingClientRect (READ) first, appendChild, extend Styles (WRITE) later. Not sure is it possible.
    (I read it from avoid-layout-thrashing thrashing)

  2. Same code, but batching read write DOM i.e. FastDOM

  3. The drag drop library mandatory insert placeholder at _startDragSequence and _endDragSequence which is not optimum. I think placeholder should be optional, and also optional extend styles.

That's all for the first layout thrashing.

The second is almost the same as the first but a little difference. This always happen when dragging item changing active container. This layout thrashing always makes Janky user experience.

image

It first starts from _updateActiveDropContainer function.

In the investigation, the enter function in SingleAxisSortStrategy class has insertBefore placeholder. (WRITE)

    enter(item: T, pointerX: number, pointerY: number, index?: number): void {
    ...
     // Don't use items that are being dragged as a reference, because
        // their element has been moved down to the bottom of the body.
        if (newPositionReference && !this._dragDropRegistry.isDragging(newPositionReference)) {
            const element = newPositionReference.getRootElement();
            element.parentElement.insertBefore(placeholder, element); // <------ Here WRITE
            activeDraggables.splice(newIndex, 0, item);
        }
        else {
            coerceElement(this._element).appendChild(placeholder); // <------ Here WRITE
            activeDraggables.push(item);
        }
          // The transform needs to be cleared so it doesn't throw off the measurements.
        placeholder.style.transform = '';
        // Note that usually `start` is called together with `enter` when an item goes into a new
        // container. This will cache item positions, but we need to refresh them since the amount
        // of items has changed.
        this._cacheItemPositions(); // <----- HERE READ
    }

And then this._cacheItemPositions(); which call getMutableClientRect call getBoundingClientRect
image

Again layout thrashing.

Suggested Solution

  1. let users provide their own implementation DropList Sort Strategy Item extends DropListSortStrategyItem
    implements DropListSortStrategy

Then, dropListRef sortStrategy has to be configured instead of fixed SingleAxisSortStrategy.

this._sortStrategy = new SingleAxisSortStrategy(this.element, _dragDropRegistry); // https://github.com/angular/components/blob/main/src/cdk/drag-drop/drop-list-ref.ts#L196
  1. _cacheItemsPositions, getBoundingClientRect (READ) first, appendChild, extend Styles (WRITE) later. Not sure is it possible.
    (I read from avoid-layout-thrashing thrashing)

  2. Same code, but batching read write DOM i.e. FastDOM

  3. The drag drop library mandatory insert placeholder at startDragSequence which is not optimum. I think placeholder should be optional, and also optional extend styles.

Here is similar Performance profiling in json not same as images above but same problem.
Profile-20221117T201453.json.zip

Reproduction

Steps to reproduce:

  1. Production Link to be sent privately

Expected Behavior

It should have no layout thrashing (no forced reflow).
This image below shown there is no layout thrashing when I deleted the code insertBefore, extendStyles, but the behavior of dragging is bug.

image

Actual Behavior

There are layout thrashing every time, the placeholder has been inserted.

image

Environment

  • Angular: 14.2.9
  • CDK/Material: CDK
  • Browser(s): Chromium (Edge, Chrome), Firefox
  • Operating System (e.g. Windows, macOS, Ubuntu): Windows, macOS, Ubuntu

Angular CLI: 14.2.9
Node: 16.13.2
Package Manager: npm 8.5.3
OS: darwin arm64

Angular: 14.2.7
... animations, common, compiler, compiler-cli, core, forms
... google-maps, language-service, platform-browser
... platform-browser-dynamic, router, service-worker

Package Version

@angular-devkit/architect 0.1402.6
@angular-devkit/build-angular 14.2.6
@angular-devkit/core 14.2.9
@angular-devkit/schematics 14.2.9
@angular/cdk 14.2.5
@angular/cli 14.2.9
@angular/fire 7.4.1
@angular/material 14.2.5
@angular/material-moment-adapter 14.2.5
@schematics/angular 14.2.9
rxjs 7.5.7
typescript 4.8.4
webpack 5.75.0

Related issues

angular/angular#20471
#13372

@perjerz perjerz added the needs triage This issue needs to be triaged by the team label Nov 16, 2022
@perjerz
Copy link
Author

perjerz commented Nov 17, 2022

This Stackblitz also show the same behavior of layout thrashing when dragging item with several DOMs in several drag refs.

https://stackblitz.com/edit/angular-sxsjem

image

Here is Performance profiling in json.
Profile-20221117T201056.json.zip

@nuttaponkhamkul123
Copy link

I made the huge amount of dragrefs and happend to me too. any solution for this?

@perjerz
Copy link
Author

perjerz commented Dec 16, 2022

@e-oz
Copy link

e-oz commented Dec 26, 2022

This Stackblitz also show the same behavior of layout thrashing when dragging item with several DOMs in several drag refs.

I don't understand how exactly this demo should work, but switching changeDetectionStrategy to "OnPush" helped to make it usable.

@crisbeto crisbeto added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent perf This issue is related to performance needs investigation A member of the team needs to do further investigation to determine the root cause area: cdk/drag-drop and removed needs triage This issue needs to be triaged by the team labels May 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: cdk/drag-drop needs investigation A member of the team needs to do further investigation to determine the root cause P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent perf This issue is related to performance
Projects
None yet
Development

No branches or pull requests

4 participants