Problem/Motivation

The first Drupal.attachBehaviors() call for the document is executed on DOMContentLoaded event (drupal.init.js) which only happens after the whole body is loaded.
Since Big Pipe inserts its chunks before the closing </body> tag, this means that Big Pipe practically pauses normal page load/initialization until all its chunks are loaded, leading to a possible situation where user's browser already has most DOM content but the user can't interact with it since JS is not initialized yet.

However, at the same time, the bigPipeProcessDocument method at big_pipe.js may still attach JS behaviors for the chunks in processes, even if it happens before DOMContentLoaded, leading to a situation when JS behaviors for dynamic page parts are attached before the rest of the document.
This effectively means that cached page content parts may stay unprocessed by JS code while uncached content gets processed earlier.

Steps to reproduce

1. Install Drupal core.
2. Add any block that is to be rendered with Big Pipe (i.e. use big_pipe_demo module).
3. Load the page.
4. Use JS debugger, add a breakpoint at Drupal.attachBehaviors, reload the page.
Expected:

  • the debugger stops as soon as main page content is loaded.

Actual:

  • it's not being hit until all Big Pipe chunks are rendered by PHP.

Proposed resolution

Execute Drupal.attachBehaviors once the first script[data-big-pipe-event="start"] tag is sent to a browser.

Remaining tasks

Review, automated testing, regression testing.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

Not sure about that?..

CommentFileSizeAuthor
#18 3294720-18-D10.patch1.11 KBabramm
#12 interdiff-3-12.txt1.07 KBabramm
#12 3294720-12-D9.patch1.11 KBabramm
#3 3294720-3-D9.patch1.12 KBabramm

Comments

abramm created an issue. See original summary.

abramm’s picture

abramm
Primary language Ukrainian
Location Lutsk
commented
Issue summary: View changes
abramm’s picture

abramm
Primary language Ukrainian
Location Lutsk
commented
StatusFileSize
new 3294720-3-D9.patch1.12 KB

Here's a patch adding early Drupal.attachBehaviors() call as proposed.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Active » Needs work

That makes sense to me, +1

+++ b/core/modules/big_pipe/js/big_pipe.es6.js
@@ -93,6 +93,11 @@
+    once('big-pipe-early-behaviours', 'body', context).forEach((el) => {
+      Drupal.attachBehaviors(el);
+    });

can be simplified to once(…).forEach(Drupal.attachBehaviors) Or I would even call Drupal.attachBehaviors() without the once. Behaviors are safe to execute several times.

abramm’s picture

abramm
Primary language Ukrainian
Location Lutsk
commented
Status: Needs work » Needs review

Hi @nod_,
That won't work since forEach() would pass the array element index as a second argument to the callback:

forEach((element, index, array) => { /* ... */ })

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global...

This would cause an issue since second argument for Drupal.attachBehaviors is drupalSettings.

abramm’s picture

abramm
Primary language Ukrainian
Location Lutsk
commented

As for once(), while behaviors are safe to execute multiple times, there could be still a performance penalty so I'd stick with once() here.

nod_’s picture

nod_
Primary language French
Location Lille
commented

right, forgot about those extra parameters :)

As for the once, yeah missed that the code was in the bigPipeProcessDocument function. I expected that code to be in the top-level of the file (at the same level as the initial call to bigPipeProcess();) so that the once is not required.

That would work since big_pipe.js is amongst the last js file loaded (right before the <script type="application/vnd.drupal-ajax" data-big-pipe-event="start"></script> element) that would work too.

cilefen’s picture

cilefen commented

I am wondering if #3294168: BigPipe blocking custom module libraries after upgrade to 9.3. is reporting the same.

abramm’s picture

abramm
Primary language Ukrainian
Location Lutsk
commented

@cilefen I don't think it the same issue; what I'm trying to do here is to improve page loading / ready time while the issue you're linking to is related to libraries loading failure.

@nod_ I've put the code to the bigPipeProcessDocument() method for a purpose.
While the big_pipe.js file is currently loaded before the big pipe start marker, it is not guaranteed that the same loading order would be preserved forever and I'm scarred thinking of what fantastic side effects could happen if attachBehaviors be executed before DOM content is fully loaded :-).

I'm also thinking about some potential issues (advagg?) for people changing libraries loading orders/weights.
The safest option I could think about is "attach behaviors as soon as big pipe start marker is loaded in browser". The big_pipe.js already has mechanism for detecting that so I've just put it there.

Hope that makes sense.

cilefen’s picture

cilefen commented

Sometimes I go too far trying to spot connections.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Needs work

That's fair enough, thanks for the explanations.

Just need to fix the spelling so that commit checks pass and that'd be RTBC for me.

abramm’s picture

abramm
Primary language Ukrainian
Location Lutsk
commented
Status: Needs work » Needs review
StatusFileSize
new 3294720-12-D9.patch1.11 KB
new interdiff-3-12.txt1.07 KB

Updated patch with fixed spelling.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community

Works for me!

Thanks for the patch and the quick reroll :)

catch’s picture

catch
he/him
Primary language English
commented

I'm not sure if/how we'd actually be able to add tests this, but... should we be trying to test this?

nod_’s picture

nod_
Primary language French
Location Lille
commented

umm I guess we could try to test that the first call to attachbehavior is made with document.body as the context of the behaviors the first time behaviors are called.

But I'm not sure of the benefit of having a test for it.

abramm’s picture

abramm
Primary language Ukrainian
Location Lutsk
commented

As for the test logic, we could put a JS behavior which'd check if it's called before DOMContentLoaded.

However I'm not even sure this is reliably reproducible; the test should invoke the situation when the start marker is already sent to a browser but the first chunk is at least 50ms late. Surely we could reproduce this by putting sleep() in a lazy builder callback but in order for that to work, the webserver should support disabling output buffering so would it work reliably across different test environments?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

We need a 10.x version of the patch. I.e. we to do 

cd core
yarn run build

after applying the patch on Drupal 10.x as the JS is different on 10.x since it only supports more modern browsers.

abramm’s picture

abramm
Primary language Ukrainian
Location Lutsk
commented
Status: Needs work » Needs review
StatusFileSize
new 3294720-18-D10.patch1.11 KB

Here's a patch for D10.

omkar-pd’s picture

omkar-pd commented
Status: Needs review » Needs work

As the tests are failed, moving this to needs work.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Reviewed & tested by the community

Patch needed to be tested against 10.x, but was tested against 9.5.x.

RTBC, TestBot will strike me down if it fails tests.

  • catch committed 0c57711 on 9.5.x 
    Issue #3294720 by abramm, nod_, alexpott: The attachBehaviors() for...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 10.1.x/10.0.x, and 9.5.x respectively, thanks!

  • catch committed a3c9d9d on 10.0.x 
    Issue #3294720 by abramm: The attachBehaviors() for document is only...
  • catch committed 798800c on 10.1.x
    Issue #3294720 by abramm: The attachBehaviors() for document is only...

Status: Fixed » Closed (fixed)

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

cilefen’s picture

cilefen commented
Related issues: +#3337995: Big Pipe calls attachBehaviors twice

There is a report of this causing a regression.

gurunathan’s picture

gurunathan commented

The problem still exists with Drupal 9.5.3.
How to fix it?

herved’s picture

herved commented

Just a quick note as I also stumbled on this after debugging an issue for quite a while.
As #3337995: Big Pipe calls attachBehaviors twice mentions, once is a requirement, but in my case the issue was that the class used in the once() selector was being applied to another nested element. Definitely an issue from that JS behavior.

once('foo', '.bar .baz', context).forEach(function () {
  ...
  $(this).find(...).addClass('baz');
}
dgtlmoon’s picture

dgtlmoon commented

Same over at https://www.drupal.org/project/drupal/issues/3347144 , `#states` fails to run properly on multi-level/complex state setups due to states handler binding multiple times when bigpipe is involved

leopathu’s picture

leopathu commented
Version: 9.5.x-dev » 10.3.x-dev

Why this code not present in the 10.3 ?

and this issue is happening 10.3 now.