Problem/Motivation

After the off-canvas dialog is created, it executes the following function:

afterCreate({ $element, settings }) {
  const eventData = { settings, $element, offCanvasDialog: this };

  $element
    .on('dialogresize.off-canvas', eventData, debounce(Drupal.offCanvas.bodyPadding, 100))
    .on('dialogContentResize.off-canvas', eventData, Drupal.offCanvas.handleDialogResize)
    .on('dialogContentResize.off-canvas', eventData, debounce(Drupal.offCanvas.bodyPadding, 100))
    .trigger('dialogresize.off-canvas');

  Drupal.offCanvas.getContainer($element).attr(`data-offset-${Drupal.offCanvas.getEdge()}`, '');

  $(window)
    .on('resize.off-canvas scroll.off-canvas', eventData, debounce(Drupal.offCanvas.resetSize, 100))
    .trigger('resize.off-canvas');
}

Two resize events are fired in rapidly,

// Firstly, the script triggered:
.trigger('dialogresize.off-canvas');

// And at the end of script, it triggerred:
.trigger('resize.off-canvas');

While the off-canvas is also designed to work with window.reseize events, I don't see any reason we need to fire dialogresize.off-canvas at the early stage. Two events caused Drupal.offCanvas.bodyPadding to be executed twice even with debounce.

These event loops might cause random test fails.

Proposed resolution

Remove excess event triggering.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

droplet created an issue. See original summary.

tedbow’s picture

I manually tested and tried to see if was any difference from removing the event.

I don't think it affects the behavior at all. So removing the triggered event seems like a good idea.

+1 to RTBC

GrandmaGlassesRopeMan’s picture

- remove unused event binding.

GrandmaGlassesRopeMan’s picture

droplet’s picture

I thought to do the same thing as @drapl but just unsure at the time..

and little bit more:

the resizing debounced already, so further actions need not extra debounce.
debounce(Drupal.offCanvas.resetSize, 100)

GrandmaGlassesRopeMan’s picture

Issue tags: +Needs manual testing

I think this looks good. I am a bit worried about this breaking some interactions we don't expect or can't test for with Phantom.

tedbow’s picture

Title: [Performance] Remove extra `resize.off-canvas` event » [Performance] Remove unused event triggers and listeners
Status: Needs review » Reviewed & tested by the community

Looks good. RTBC 🏅

Wim Leers’s picture

@tedbow Did you do manual testing like @drpal said was necessary?

tedbow’s picture

Issue tags: -Needs manual testing

@Wim Leers thanks for checking. I did do manual testing but also doubled checked and did manual testing again.

Still looks good!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 2903297-5.patch, failed testing. View results

GrandmaGlassesRopeMan’s picture

Issue summary: View changes
FileSize
1.81 KB

- reroll

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan’s picture

Status: Needs work » Needs review
droplet’s picture

Status: Needs review » Reviewed & tested by the community

SAME PATCHING.

tedbow’s picture

Component: outside_in.module » settings_tray.module

  • larowlan committed 98f4e57 on 8.5.x
    Issue #2903297 by drpal, droplet, tedbow: [Performance] Remove unused...

  • larowlan committed c3a11a8 on 8.4.x
    Issue #2903297 by drpal, droplet, tedbow: [Performance] Remove unused...

larowlan credited larowlan.

larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed as 98f4e57 and pushed to 8.5.x

Cherry-picked as c3a11a8 and pushed to 8.4.x

Credited @tedbow for manual testing.

larowlan’s picture

note: backported to 8.4.x because this is experimental module

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.