[PP-1] Handle media image fields on page data form

Feel free to ping me when you dig into this; @bnjmnm is the real expert in that area, but happy to assist when @bnjmnm has more important things to tackle!

No longer relevant as this was addressed by other work. There is confirmation of this in the test 'Can open the media library widget in an xb_page props form' in media-library.cy.js (this test does more than just open, it adds / removes etc.)

Maybe there's something more specific that's wrong but this doesn't seem to be working for me? Attached video to show what I'm seeing.

Clearly a problem is occurring in #7 but it isn't one I'm running into (see this video) + this and the e2e test mentioned in #5 demonstrate that "Handl[ing] media image fields on page data form" currently works.

The error in #6 looks like it's coming from OpenAPI validation, which should obviously be addressed. If anyone currently experiencing it can either update this issue summary or create a new issue targeting that specific bug

@lauriii

For me I can reproduce this regardless of how long I wait for things to initialize.
However, when I uninstall xb_vite module it no longer occurs.
Cypress tests don't use xb_vite.
Can this be reproduced without xb_vite? i.e. in a production setting?

#12: I'm seeing something very similar in #3533703-3: Calling the `setPageData` action creator directly doesn't update values in the page data form → where I wrote a fix, but the double rendering done by <StrictMode> breaks it, which is what happens when you run the app via xb_vite and Vite's dev server. I'm a bit afraid this is exposing a bug in <a href="https://git.drupalcode.org/project/experience_builder/-/blob/272794d73a28320fb9989850dcb3d60150b0ce41/ui/src/components/form/inputBehaviors.tsx">inputBehaviors</a>.

Saw this happen without xb_vite

When this happens, no amount of waiting helps, which seems to point to a race condition.
In the failure case the data-once=drupal-ajax attribute is missing.

This comes from Drupal.behaviors.AJAX which we load as the experience_builder/xb.drupal.ajax library

However, our custom ajax commands don't have a dependency on this, so could load before the base ajax has.

I added that dependency and in my testing this seems to work.

I also added some subtle CSS changes so that the user can't click the button until the behavior has been attached, including a greyed out state

Was able to still hit this, so will dig further into the race condition

The behavior not attached is a red-herring - here you can see the available Drupal.behaviors and AJAX is there but the button stays greyed out - meaning data-once="drupal-ajax" didn't get applied

Debugging this further I can only get this to occur on a hard-reload which does point towards JS files being fully loaded

When this happens, the jQuery selector for the ajax element in drupalSettings.ajax doesn't find the element, which probably indicates it is re-rendering

I was able to get this to a point where I could no-longer reproduce it, even with a hard-reload.

The tl;dr was that we were calling useDrupalBehaviors once we had HTML for the form and the outer div (ref) had been rendered. But this didn't mean the inner form had rendered (just that we had the HTML) and as a result the this selector in Drupal's AJAX was unable to find the element to attach the behavior too.

This change ensures the behaviors are attached once the form HTML has been rendered.

Looks like @larowlan is confident this was a race condition between our logic and the AJAX system (independently) executing. That seems very plausible given all other comments in this issue!

I like the elegance of the solution, but I know not a single thing about "refs", so deferring to @bnjmnm.

I would like to see this crucial comment on the MR moved into the code, and ideally, generalized to also apply to the component instance form, where AJAX behaviors are also a thing.

Would be a shame to solve the same problem two times.

Changing status / assigned to reflect the changes @jessebaker requested in the MR

wim leers → credited jessebaker → .

Thanks, both!

If I make the same change to component inputs form, it stops working there the same way as the original report here 😭
If I change the way useDrupalBehaviors works to pass ref.current as a dependency instead of just refit fixes the component inputs form but breaks the page form

(╯°□°）╯︵ ┻━┻

OK I think I've found a solution that works for both forms.

I think the issue w.r.t race conditions comes down to how useEffect works - there is no guarantee that the browser has painted by the time the effect is called - which is why we were using setTimeout in the useDrupalBehaviors hook.

But we were also setting the HTML inside a useEffect hook but I don't think we need that extra layer of reactivity, because it is already provided by RTKquery and hyperscriptify and parseHyperscriptifyTemplate are synchronous.

Removing the setState/useState to track the form HTML and just relying on the reactivity provided by redux seems to fix the issue reported here and in a way that I can apply the same fix to the component inputs form without breaking it..

Ok, 32 test fails tells me that idea won't fly

Woot got to the bottom of why this only impacts page data form

We don't render inputs on the page data form until page data exists

This was added in 🐛 Page data form inputs should not render until page data exists Active

So this is why we were attaching behaviors but jQuery wasn't finding the elements.

In PageDataForm the condition that the jsx form data exists was satisfied, but the elements weren't being rendered because of that guard in inputBehaviors and hence jQuery wasn't finding them.

So the race condition is as follows:

• HTML for the page data form has been loaded and jsxFormContent exists
• The layout has not been, so pageData does not

The fix is much simpler now - was glad I could work that one out 🤯 - basically we hoist that guard out of each individual element and only check it on the outer form renderer. It achieves the same result but ensures that when the parent form ref renders, the children inside it actually render as well and therefore jQuery can attach the behaviors.

I also added some timeout clean up which was missing and retained the CSS changes because I think they're useful still.

Sounds like this is definitely ready for a new @jessebaker review!

That solution makes sense, to move the page data loaded check to a more sensible place. Nice.

Hi Team, I have verified below scenarios and those are working as expected :

Thanks! :)