Problem/Motivation

Drupal triggers behaviors on each Ajax changes. On the homepage, Drupal.behaviors.toggleEditMode.attach() called 13 times.

`Drupal.ajax.instances` defined inside the function and loop over on each call. It's about 70 loops in total on the homepage loading.

Proposed resolution

1. It doesn't reassign the value, we need not `filter` and then `forEach`. `filter` create a new ARRAY which uses extra memory.

2. Ideally, it should only execution after contextual links fully loaded or after Top left Toggle button is clicked.

3. Ideally, "fullscreen" should be returned from Ajax. (might be hack Drupal.Ajax to support data-progress-type)

4. Drupal has no 2-way data binding. If we can't do #3, I'd suggest changing the setting when you clicking the target edit area (here's what the #0 patch does)

Remaining tasks

- Review Patch

User interface changes

- NO

API changes

- NO

Data model changes

- NO

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

droplet created an issue. See original summary.

droplet’s picture

** Note: I don't familiar with OutSide_In :) And here's a quick patch in #0. I hope there's some better workaround :P

droplet’s picture

well. It can even more clean code

droplet’s picture

Priority: Normal » Major
Issue tags: +frontend performance

I raise the priority since `Drupal.attachBehaviors` reattached 9 times on homepage (default installation profile). A huge performance impact

Wim Leers’s picture

A huge performance impact

Can you quantify this?

tedbow’s picture

Status: Needs review » Needs work
+++ b/core/modules/outside_in/js/outside_in.es6.js
@@ -212,21 +213,6 @@
-          instance.options.data.dialogOptions.outsideInActiveEditableId = $(instance.element).parents('.outside-in-editable').attr('id');
-          instance.progress = { type: 'fullscreen' };

These 2 lines are not being replaced anywhere. We still need the behavior so I don't think this block of code can removed at all.

I manually tested and confirm that "active" block is not highlight with this patch. Which means we need a test for that since the patch passed.

drpal’s picture

  1. +++ b/core/modules/outside_in/js/outside_in.es6.js
    @@ -212,21 +213,6 @@
    -        .filter(instance => instance && $(instance.element).attr('data-dialog-renderer') === 'off_canvas')
    

    Can we also check for the existence of instance.options.data.dialogOptions.outsideInActiveEditableId to we can more strictly filter elements to avoid looking at potential duplicates?

  2. +++ b/core/modules/outside_in/js/outside_in.es6.js
    @@ -212,21 +213,6 @@
    -          instance.progress = { type: 'fullscreen' };
    

    I think we need these.

droplet’s picture

Issue tags: +Needs JS tests