Leaflet not using once() ⇒ Leaflet + BigPipe causes double checkboxes

I'm 99% certain that this is another occurrence of 🐛 Big Pipe calls attachBehaviors twice Closed: works as designed — i.e. a bug in Gin's Drupal behaviors not using once() correctly.

Is it? @Joao Sausen & oliverh65 is this happening with Gin or another theme?

If so we can close it as a duplicate in favor of 🐛 Double rendering select all checkboxes Closed: cannot reproduce

This behavior is not Gin specific.
I think it's a big pipe problem, as I've had this problem in leaflet module.

Then let's move this back to core 😉

It's starting to sound like a bug in the Leaflet module then actually 😅 Can you provide concrete steps to reproduce?

It's normal that attachBehaviors is called multiple times. Also see the discussion in 🐛 Big Pipe calls attachBehaviors twice Closed: works as designed .

Just checked https://git.drupalcode.org/project/leaflet/-/blob/10.0.x/js/leaflet.drup... and I see:

  Drupal.behaviors.leaflet = {
    attach: function(context, settings) {

      // For each Leaflet Map/id defined process with Leaflet Map and Features
      // generation.
      $.each(settings.leaflet, function(m, data) {

        $('#' + data['mapid'], context).each(function () {

… this is definitely incorrect. It should be using once(). It doesn't use that anywhere in the JS file.

It was correctly updated in https://git.drupalcode.org/project/leaflet/-/commit/73757ffd5e5565a606f6.... Looks like there's a history of breaking this: https://www.drupal.org/project/issues/leaflet?text=attachbehaviors&statu... →

Thank @Wim Leers for pointing that out,
but I believe that code in the Leaflet module is correct (and at least is not wrong).
The Leaflet module js is handling all the each defined Leaflet map for the web page, and processing those settings (and related Leaflet maps) accordingly. We deliberately didn't want/need to use the once() pattern there ... and the Leaflet module already faced an issue related to Big Pipe causing double attachments, here: https://www.drupal.org/project/leaflet/issues/3337537 🐛 Big Pipe changes causes multiple map attachments Fixed
and this following comment explained how in more details: https://www.drupal.org/project/leaflet/issues/3337537#comment-14898998 🐛 Big Pipe changes causes multiple map attachments Fixed

In the end, if the Big Pipe triggers twice the Drupal.behaviour ... well it could be right not to stop its second call.
What we need to stop is the re-initialisation of a Leaflet Map inside a container that already have an initialised one.

I believe that is not the Leaflet module triggering double Drupal.attachBehaviours() execution ... but it is just facing it.

Please send me here better evidence that all this is caused by the Leaflet module indeed (that I couldn't see yet with your comment),
otherwise I will close this as "works as designed" soon ...

Note that none of this is BigPipe-specific. It's just the Drupal AJAX system.

believe that is not the Leaflet module triggering double Drupal.attachBehaviours() execution

Correct, it's the AJAX system, which BigPipe uses.

... but it is just facing, and properly reacting to it.

Leaflet is indeed invoked to attach behaviors. But it's not reacting to this properly.

Quoting Drupal.attachBehaviors from core/misc/drupal.js:

   * Behaviors should use `var elements =
   * once('behavior-name', selector, context);` to ensure the behavior is
   * attached only once to a given element. (Doing so enables the reprocessing
   * of given elements, which may be needed on occasion despite the ability to
   * limit behavior attachment to a particular element.)

👆 It's that reprocessing that Leaflet does not handle correctly.

We deliberately didn't want/need to use the once() pattern there ...

Interesting … 🤔

#3337537-4: Big Pipe changes causes multiple map attachments → says:

With the above solution the once() condition will be triggered only for the all document load ("html"), and that is not good.

Well, that's true, one should never use once() on the entire document. Then this is indeed a logical consequence:

It won't trigger the Leaflet Map initialisation in case of Leaflet Map inside Ajax requests and contexts

You need to use the proper selector — see https://www.npmjs.com/package/@drupal/once, which says:

once(id, selector, [context]) ⇒ Array.<Element>
Ensures a JavaScript callback is only executed once on a set of elements.

So for example Drupal.behaviors.contextual does this:

  Drupal.behaviors.contextual = {
    attach(context) {
      const $context = $(context);

      // Find all contextual links placeholders, if any.
      let $placeholders = $(
        once('contextual-render', '[data-contextual-id]', context),
      );

That means 'contextual-render' is the identifier of the behavior/processing you want to do only once, '[data-contextual-id]' is the selector to find the relevant elements and contextis the root of the tree where you want to find elements matching that selector.

The patch at #3337537-2: Big Pipe changes causes multiple map attachments → did this though:

once('myLeafeltBehavior', 'html').forEach(function (element) {

… which means it was selecting the html element … which selects the entire HTML document, so it's clearly not a selector matching every individual leaflet map!

Hope it makes sense now 😊

cool! ... and super thanks for your great mentoring @Wim 😊
ket me test the attached patch, on some advance Leaflet use case (multiple leaflet maps in the same page, and added through IEF) and I will commit into dev and deploy a new release with this ...

Ok. I successfully tested and QAed what has been committed into dev. I am going to deploy a new Leaflet 10.0.12 release with this.
Thanks again @Wim Leers for guiding me to this, that I am confident is a proper fix for this.

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