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.
Comments
Comment #2
tedbowI 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
Comment #3
grandmaglassesropeman- remove unused event binding.
Comment #4
grandmaglassesropemanComment #5
droplet commentedI 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)Comment #6
grandmaglassesropemanI 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.
Comment #7
tedbowLooks good. RTBC 🏅
Comment #8
wim leers@tedbow Did you do manual testing like @drpal said was necessary?
Comment #9
tedbow@Wim Leers thanks for checking. I did do manual testing but also doubled checked and did manual testing again.
Still looks good!
Comment #11
grandmaglassesropeman- reroll
Comment #12
grandmaglassesropemanComment #13
grandmaglassesropemanComment #14
droplet commentedSAME PATCHING.
Comment #15
tedbowComment #19
larowlanCommitted as 98f4e57 and pushed to 8.5.x
Cherry-picked as c3a11a8 and pushed to 8.4.x
Credited @tedbow for manual testing.
Comment #20
larowlannote: backported to 8.4.x because this is experimental module