Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#11 | 2903297-11.patch | 1.81 KB | GrandmaGlassesRopeMan |
#5 | 2903297-5.patch | 1.79 KB | droplet |
#3 | 2903297-2.patch | 1.81 KB | GrandmaGlassesRopeMan |
remove-extra-trigger.patch | 1.83 KB | droplet | |
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 CreditAttribution: 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 CreditAttribution: 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