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

[5.0.0-beta.4]: Editing nested elements causes JS violation / Forced reflows #14510

Closed
thupsi opened this issue Mar 1, 2024 · 13 comments · Fixed by #14911
Closed

[5.0.0-beta.4]: Editing nested elements causes JS violation / Forced reflows #14510

thupsi opened this issue Mar 1, 2024 · 13 comments · Fixed by #14911
Assignees
Labels

Comments

@thupsi
Copy link

thupsi commented Mar 1, 2024

What happened?

When editing nested elements, especially more than 3 levels deep (but can be less), it's easy to encounter a JS bug that causes a large number of reccuring "forced reflows", introducing heavy lag in the UI.

I have attached a video of a specific workflow, but I've triggered this bug in a variety of situations:

forced_reflow.mp4

I think if you play around with nested elements it's a pretty common bug to run into, but let me know if you can't reprocuce.

Craft CMS version

5.0.0-beta.4

PHP version

No response

Operating system and version

No response

Database type and version

No response

Image driver and version

No response

Installed plugins and versions

@thupsi thupsi added the bug label Mar 1, 2024
@thupsi thupsi changed the title [5.0.0-beta.4]: Editing nested elements causes JS violation [5.0.0-beta.4]: Editing nested elements causes JS violation / Forced reflows Mar 1, 2024
@brandonkelly
Copy link
Member

I’m seeing a slightly different error, using Edge: [Violation] 'setTimeout' handler took <N>ms. (Can’t get any similar violations reported in Chrome or Firefox.) Will test further on a PC…

@thupsi
Copy link
Author

thupsi commented Mar 1, 2024

Yep, I've seen that error also, but at some point something occurs that causes the reccuring reflows. Maybe the root problem is common for both errors.

@thupsi
Copy link
Author

thupsi commented Mar 4, 2024

Hello again! I think the reflow error is related to the toggling of the slideout details sidebar.

I'm including some specific steps so that you can replicate more easily in a clean Craft 5 install:

Important note: Check this on a screen size where the slideout sidebar is opened by default

  1. Create a section with an entry type (A)
  2. In entry type (A) create a matrix field with another entry type (B)
  3. Go to the section settings and edit the entry type (A) settings in a slideout
  4. Edit the matrix field in a slideout
  5. Edit the entry type (B) in a slideout. Notice that now you should have 3 slideouts opened.
  6. Click cancel to close the last slideout and return to the matrix settings.
  7. Reopen the entry type (B) via slideout.
  8. Toggle the slideout details sidebar. You should see recursive errors in the console, that keep increasing until you close the slideout, with a message along the lines of [Violation] Forced reflow while executing JavaScript took <N>ms

Can you replicate the error with the steps above?

@thupsi
Copy link
Author

thupsi commented Mar 4, 2024

Uh-oh! Sorry, but I have a further update! Even without the sidebar toggling you can trigger the error.

Follow the steps above but when you go to step 5 open the title settings of entry type (B), click the checkbox to hide the title label and click apply. Same error.

@thupsi
Copy link
Author

thupsi commented Mar 4, 2024

It almost seems that some javascript is running recursively with each slideout and it just keeps pilling on, and at some point it crosses a threshold and goes mental 🤪

I wouldn't have a problem if this happened after you open a crazy amount of slideouts, but with the new edit workflows it's pretty easy to trigger. My system ram is 64GB so I wouldn't think this could be an issue.

@thupsi
Copy link
Author

thupsi commented Apr 29, 2024

Hi again @brandonkelly. I have been trying to get to the bottom of this, since it causes severe lag when configuring sections and fields in nested slideouts. I'm not a js expert but I managed to track it down to the resize object in Garnish.

I then added a debounce function in Garnish like this:

function debounce(func, wait) {
    let timeout;
    return function(...args) {
        const context = this;
        clearTimeout(timeout);
        timeout = setTimeout(() => func.apply(context, args), wait);
    };
}

and used it inside resize like this:

resize: {
    setup: debounce(function(t, e, i) {
        if (this === window)
            return !1;
        n()("> :last-child", this).addClass("last"),
        X().listenTo(this, z)
    }, 300), // Debounce with a 300ms delay
    teardown: function() {
        if (this === window)
            return !1;
        X().removeListener(this, z)
    }
}

This seems to solve the problem on my end. Not sure if it's a solution that suits you, but I would be gratefull if you can take a look and maybe improve or adapt it however you like. Thanks!

@brandonkelly
Copy link
Member

This should be fixed for the next Craft 4 and 5 releases, via #14911.

@thupsi
Copy link
Author

thupsi commented May 1, 2024

Perfect! I did some quick testing and it seems the issue is fixed! 🙂

@brandonkelly
Copy link
Member

Thanks for testing @thupsi! Craft 4.9.1 and 5.1.1 are out with that fix now.

@thupsi
Copy link
Author

thupsi commented Jun 17, 2024

@brianjhanson Hello again. Alas, it seems I was too quick to mark this issue as solved. That's partly because I haven't found a clean way to reproduce and partly because I had limited time to test thoroughly. In any case, here's some more info:

  • The issue seems to manifest only in Windows Chrome (and Edge)
  • From my testing I can seem to trigger it when editing entry types and opening slideouts containing FLDs. I go about just adding fields, selecting entry types in Matrix fields etc, pretty standard stuff.
  • At some point the application enters a loop where it repeatedly triggers resize events, causing forced reflows that significantly degrade performance.
  • If I leave it open, the reflows go into the thousands and they only stop increasing when I close all slideouts.
  • In the Perfomance Panel in Chrome I detected a high percentage of time spent on the resize event, with calls to element-resize-detector.js
  • Using a MutationObserver, repeated mutations were detected in elements with the class fld-tab last

MutationObserver Log Example:

Mutation detected: 
MutationRecord {type: 'attributes', target: div.fld-tab.last, addedNodes: NodeList [], removedNodes: NodeList [], previousSibling: null, nextSibling: null, attributeName: 'style', oldValue: null}
Attribute style modified in element: <div class="fld-tab last" style="height: 216px;">

Screenshot from the Performance tab:
image

@brandonkelly
Copy link
Member

Thanks for the info! That helped me finally get to the bottom of this. I realized that it’s only triggerable when a major window resize change happens, such as the browser tools opening/closing, or the window being resized in some automated way (e.g. window snapping).

Tracked down the culprit and got this fixed for the next release 🎉

@thupsi
Copy link
Author

thupsi commented Jun 18, 2024

Awesome! Thanks 🙂

@brandonkelly
Copy link
Member

Craft 4.10.2 and 5.2.2 are out with that fix. Thanks again for all the help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants