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

Can we get a screenshot? Does this happen with Claro theme?

It helps to relate the issues. It may have been better to have moved the issue rather than to create another one.

Is this a Big Pipe bug or does Big Pipe expose a theme bug?

Claro not exhibiting the bug is strong evidence.

I'm not sure, but I just noticed that when BigPipe is activated, the javascript function "Drupal.attachBehaviors" is executed twice.
A first time by

domReady(function () {
    Drupal.attachBehaviors(document, drupalSettings);
  });

And a second time by

function bigPipeProcessDocument(context) {
    if (!context.querySelector('script[data-big-pipe-event="start"]')) {
      return false;
    }
    once('big-pipe-early-behaviors', 'body', context).forEach(function (el) {
      Drupal.attachBehaviors(el);
    });

To check it, it is enough to load a page, then in the console javascript of the navigator to seek "Drupal.attachBehaviors", put a breakpoint, then reload the page with F5

I'm having the same issue on Drupal 10.0.0 as olivierh65.

That piece of code that runs attachBehaviors on big pipe is useless since it runs after code attachBehaviors...

I'm 99% certain that this is another occurrence of #3337995: Big Pipe calls attachBehaviors twice — 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 #3314515: Double rendering select all checkboxes

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 #3337995: Big Pipe calls attachBehaviors twice.

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 maps for the web page, and processing those settings/Leaflet maps accordingly.
We deliberately didn't want/need to use the once() pattern there ...
The Leaflet module already faced an issue related to Big Pipe causing double attachments, here: https://www.drupal.org/project/leaflet/issues/3337537
and this following comment explained how in more details: https://www.drupal.org/project/leaflet/issues/3337537#comment-14898998

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, and properly reacting to it.
I also had evidence that disabling BIG Pipe stopped the double double Drupal.attachBehaviours() calls.

Leaflet module (since its 10.0.8 version) stopped loading its maps twice with Big Pipe thanks to this commit: https://git.drupalcode.org/project/leaflet/-/commit/a3caa11cb81d736971b4...

Please send me here better evidence that all this is indeed caused by the Leaflet module (that I couldn't see yet with your comment),
otherwise I will close this as "works as designed" ... or move this back to its original location (Drupal 9.5 Core)

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 😊

P.S.: the demo at https://www.geodemocracy.com/drupal_geofield_stack_demo/web/geoplaces-ma... is REALLY cool! 🤩

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.13 release with this.
Thanks again @Wim Leers for guiding me to this, that I am confident is a proper fix for this.