🇺🇸United States @bnjmnm

larowlan → credited bnjmnm → .

FB Addressed, but in a new MR 3537146-error-links-cleaner-diff

bnjmnm → created an issue.

I tested a theory in the branch 3536040-ensure-unique-changed and it may have fixed it. I can't be 100% sure as I've only run it ~10 times so far and there's only one e2e running to make things faster, but it has yet to fail.

Although I couldn't reproduce locally, I ran logs on UnpublishedChanges.tsx and the manually created changed value was sometimes only 1 different from the existing one. This had me wondering if there are instances where, the value set could be identical to the prior one - if that were the case then $form_updated_changed_field = $changed_timestamp_int !== ((int) $entity_form_fields['changed']); might be FALSE and an unworthy changed does not get the desired continue; treatment.

  if ($field_name === 'changed' && $form_updated_changed_field) {
        continue;
      }

bnjmnm → made their first commit to this issue’s fork.

Created 📌 Nit followups to #3491047 Active w/MR.

bnjmnm → created an issue.

This shouldn't be a problem once [##3534490] lands

Setting to NR. e2e fails seem to be random / intermittent / unrelated. Will continue to stop back to rerun until green, but review should be fine unless you see publish-validation.cy.js failing.

The change in Jesse's commit looks good - it's essentially the same logic as what it replaces, but uses information that was already in the component instead of having to perform additional steps to retrieve it. It seems OK if Jesse reviews the remainder of the MR.

Looks like #11 was happening due to tailwind using colons in classnames. This has been addressed and a test was modified to include it as a use case.

Is this when applying the recipes? Because since #3515646: Add automated generation the recipes no longer enable xb_stark - I wonder if that's related?

We fixed the attribute addition in #3534601: Add a Video prop type to the Code Component editor; requires adding `file` as default `StaticPropSource`, `FileWidget` support and various infrastructure improvements - so this might be solved - can you retest and advise?

I have a working fix in 📌 Support Image Widget fully in page data form Active , which is not specific to recipes. Rather, any element that is added via a #process callback does not get run through FormIdPreRender::addAjaxAttribute and as a result does not have the form-identifying attribute needed for the inputs to get an onChange listener added. Without that, they can't receive input.

bnjmnm → created an issue.

bnjmnm → created an issue.

This has been pretty thoroughly FE / BE reviewed but still needs MR approval for

Redux-integrated field widgets /ui/src/components/form/inputBehaviors.tsx 

MR 1318 looks to address it, and maybe it's OK to stop the propagation at the wrapper. I'm gonna RTBC but it probably can't hurt to have someone with the AI service activated to make sure it didn't disrupt anything ( think if we can confirm that submitting via keyboard is unchanged it's probably fine.

If the fragment issues from #4 are out of scope, those can get their own issue. I went through this solution with the steps to reproduce and it definitely fixes the issue that I can now easily reproduce on 0.x

bnjmnm → made their first commit to this issue’s fork.

I'm running into some situations where the problem still exists:

Scenario 1

I tested with this as the slot-containing component

const Slotto = ({
  title = "Has Slots",
  one, 
  two,
  three
}) => {
  return (
    <>
      {/* Text Prop */}
      <h3>{title} </h3>
      {/* Slot */}
      {one}
      {/* Slot */}
      {two}
      {/* Slot */}
      {three}
    </>
  );
};

And the outline is only around the <h3>, not the entire component including the slots

Scenario 2

Largely the same component as above, but change

      <h3>{title} </h3>

to {title}

In this use case, there is no outline or nametag at all

Scenario 3

Take the component from scenario 2 but replace the fragment with a div, and it works great

Found a way to handle any VH value, not just 100.

I have an MR that will work for h-screen and any other 100VH situation, but > 100 VH settings would still be an issue. On the plus side, this covers far more use cases than HEAD, even if some linger.

bnjmnm → made their first commit to this issue’s fork.

Switching to needs review, but specifically a back end person should look at my MR + Code comments to make sure the things I'm removing are OK to remove. All the BE changes needed to get this working required removing stuff that I assume was added for a good reason, but since the tests pass maybe it's fine? LMK!

The positioning over the select element itself is how the Radix selects we used worked so it's been that way since it was added early December 2024, and if it's taken this long to be reported as a problem perhaps it should not be categorized as such.

When we switched to native we attempted to match the Radix styling as much as possible, but Firefox does not yet support the selectors necessary to do that. I had anticipated the Firefox deviation possibly showing up in the issue queue, but not so much the opposite.

Given that this was part of the initial designs, and there's more thorough designs on the way if we're now finding this objectionable 8 months in, I'm inclined to close this as "works as designed".

ESM + import map approach gives contrib maintainers the ability to support both types of CKEditor5 builds with a single codebase.

I don't believe the UMD approach can accommodate that, which would mean maintainers having to maintain two separate versions, one that uses imports and the other pulling from the global CKEditor. If there's a way around that - great. If not, then I suppose we need to decide what tradeoffs are acceptable.

The validateConfigurationForm / submitConfigurationForm methods in ExperienceBuilder/ComponentSource/BlockComponent aren't called at all, so the additions in #4 unfortunately won't work. This isn't a huge shock as (like the Page Data form) there isn't a traditional submit that would invoke the validate/submit methods in the way we're accustomed to.

I attempted to submit the block form similar to how the entity form is programmatically validated/submitted in \Drupal\experience_builder\ClientDataToEntityConverter::setEntityFields and didn't have any luck - if elements in the form had #element_validate callbacks they weren't called. I suspect this is because of how complex it is to do this (a delicate balance of form state and form and input values) and not because it's an unworkable approach.

bnjmnm → made their first commit to this issue’s fork.

balintbrews → credited bnjmnm → .

bnjmnm → made their first commit to this issue’s fork.

bnjmnm → made their first commit to this issue’s fork.

The 3528384-image-widget-etc MR I just pushed has a fix for the alt text field not working. The underlying problem will occur on any form element added in a #process callback.

Diagnosis

For an input to be React-controlled, it needs to have the 'data-form-id' attribute, which is added recursively to everything in the form array in \Drupal\experience_builder\Hook\ModuleHooks::formAlter

However, inputs such as alt are not part of this array, as they are added later in a #process callback. This may be a simple as running FormIdPreRender::addAjaxAttribute at a later stage in the render process.

This is a duplicate (or perhaps could be called a subset) of 📌 Support Image Widget fully in page data form Active . That said, every issue I've closed recently that doesn't include a merge gets reopened so I'll leave the possible official closing to those with a better macro perspective on the project.

Details
For an input to be React-controlled, it needs to have the 'data-form-id' attribute, which is added recursively to everything in the form array in \Drupal\experience_builder\Hook\ModuleHooks::formAlter

However, inputs such as alt are not part of this array, as they are added later in a #process callback. This may be a simple as running FormIdPreRender::addAjaxAttribute at a later stage in the render process.

MR 1250 ready

One way to manually test is to add:

// PageDataForm.tsx
// update imports
import {
  selectPageData,
  setPageData,
  updatePageDataExternally,
} from '@/features/pageData/pageDataSlice';

// add a test button
/*
      <button
        style={{ marginTop: '20px', border: '3px solid black' }}
        onClick={() => {
          dispatch(
            updatePageDataExternally({
              'title[0][value]': 'stringForTesting title',
              'body[0][summary]': 'stringForTesting summary',
            }),
          );
        }}
      >
        TEMPORARY BUTTON THAT UPDATES PAGE DATA
      </button>
 */

I'm assuming tests for the modules that wind up using this will

bnjmnm → created an issue.

effulgentsia → credited bnjmnm → .

Quick fix is in, but we should still look into reconciling the eslint and prettier ignores.

bnjmnm → created an issue.

glad the solution wound up being so elegant! Merged.

I added 3533289-if-select-value-change-triggers-react-onchange as a draft MR which approaches the tabledrag issue differently - any programmatic changes to the select element are relayed to the onChange callback.

As noted in the MR, if my changes are accompanied by me commenting away the useEffects added to DummyPropsEditForm and PageDataForm, the widget-multivalue.cy.js test is able to get several scenarios further, though it still eventually fails. Although it's not a complete solution, my hopes it is helps surface info that helps us get there.

Also worth noting
I was running into other problems with widget-multivalue.cy.js until I removed the existing components from the layout. More info is in a comment within the test.

The new MR 1253 looks promising - if we can avoid more build_id headaches I'm all for it.

I was unable to test manually until I made a change in the getPathAlias function DrupalPathWidget.tsx that checked for the truthiness of titleValue before it calls string-specific methods.

The need to do that might be related to what I then ran into:

While testing manually, I ran into an issue that seems to only occur if no fields in the page data form have been interacted with yet.
To reproduce:

• Reload the UI and immediately click the "temporary button that updates page data"
• Note that this updates the form field, but not the preview.
• Once any further interaction with the form takes place - regardless of field - the title field will reverts to the stale value shown in the preview.

.

👏 to @larowlan for tracking that down. Assigning to @wim-leers here to reflect the existing assignment in the MR to get a +1 on that backend solution.

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

I could not get the original MR: 3533703-setpagedata-vs-form-values to work. I'm not sure what use cases it was able to work as the defaultValue (AFAIK) does not consult the pageData slice.

I tested that MR and my new one by installing the xb_test_article_fields module and adding a temporary button that calls setPageData

<button
        style={{ marginTop: '20px', border: '3px solid black' }}
        onClick={() => {
          dispatch(
            setPageData({
              ...formState,
              'body[0][value]': 'stringForTesting body field',
              'body[0][summary]': 'stringForTesting summary',
              'field_xbt_textarea[0][value]':
                '<p><b>stringForTesting bold</b></p>',
              'field_xbt_textarea_summary[0][value]':
                'stringForTesting textarea summary value',
              'field_xbt_textarea_summary[0][summary]':
                'stringForTesting textarea summary summary',
              field_xbt_options_buttons: 'option3',
              'path[0][langcode]': 'zxx',
              'field_xbt_textfield[0][value]':
                'stringForTesting textfield value',
              'field_xbt_unlimited_text[0][value]':
                'stringForTesting on the Coast',
            }),
          );
        }}
      >
        TEMPORARY BUTTON THAT UPDATES PAGE DATA
      </button>

In the 3533703-setpagedata-vs-form-values-refetch-on-update MR, this temporary button is also there but calls a newly added updatePageData action which is just setPageData but also triggers an event to inform the page data form it needs to refresh. I've confirmed this approach works with text fields, but not sure about radio/select etc yet.

It seems like there was some doubt about my approach so perhaps the ultimate solution is some combination of the two MRs?

Unassigning myself so a B.E. dev can have a look at #13

Not 100% sure on this, but it seems like the mechanism that accounts for an image not yet having a value hasn't been applied to video. I added an intentionally failing e2e test that reproduces the steps.

To Reproduce: Add video component to layout, then update the Display Width field before adding a video
(This doesn't have to be the display width field - it can be any field in a component instance form with a video prop)

• We get the Component failed to render, check logs for more detail. in the preview
• The logged error is
  

  An exception has  been thrown during the rendering of a template 
      ("[experience_builder:video/video.src] NULL value found, but a  
      string is required. This may be because the property is empty    
     instead of having data present.

• Here's the model being sent in the bad request

There's one scenario where this won't happen: If the prop is optional and a video has been added then removed. In this instance, the empty video will not result in an error because of the way removed media items are handled on optional props. The request no longer attempts to send the default value and instead sends empty ones

(The above is with a temporarily altered Video component so the video prop is not required)

#9.2

Question: what's the source for the 2 videos? Did you create them? Asking for licensing purposes: they're going to be licensed as GPL2+ upon commit (and in principle they already are by having them in the MR). Just hoping to confirm

I grabbed these videos from Pexels, which specifies they are free to use (although I should point out no specific license is mentioned). Also note I reduced the dimensions and frame rate of the videos before adding to provide a smaller file size.

Merged!

Talked to @balintbrews about me having a crack at this since I've been working in this corner of XB quite often lately.

I'm leaning towards creating an additional action that is very similar to setPageData but specifically for use cases where the store change needs to inform the form vs the other (more common) way around.

Setting to NW since the input loses styling with this fix (see MR comment). The MR is approved, though, as I assume the style fix won't require anything elaborate.

In the original issue summary for 🐛 Autosave publish process does not acknowledge pathauto deactivation Postponed I provided a special case snippet for Pathauto that when added to ApiAutosaveController it successfully got things working as seen here. I won't assume it's the solution, but cant hurt to have a reference from something already proven to work.

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

bnjmnm → created an issue.

This is because CSS scope() is still an experimental feature in Firefox that needs to be enabled. There was a decision that we should go ahead with scope() as its addition was inevitable, but it might not be standard until later this year (2025) according to the FF team. Given that XB might have users prior to scope() working with FF out-of-the-box, we might need to come up with a workaround.

@lauriii

wim leers → credited bnjmnm → .

The Pipeline fails are due to CI issues installing Cypress dependencies, nothing specific to this MR (the 2nd to most recent had been passing, and the only change in the most recent were stylelint related).

Gonna merge this in now since @hooroomoo already reviewed.

I'm not able to reproduce what was reported in #46 with the "All props" component from the sdc_test_all_props module. This component has multiple booleans (one default true, one default false), though not as many as the one in the provided video

There are also and the e2e tests using the "All props" component that would have failed had the symptoms in #46 been occurring.

Although this does not appear reproducible with the components available within the experience builder module, we should give this a try with the component used in #46 to see what we can surface. Could @heyyo direct us to where we could find that component?

After debugging a bit with the ref-stored transforms I'm more glad it's in there and it might spare us some future headaches.

I think the back and front end scrutiny has been duly applied, and the changes are good ones, so this shall be merged.

I'd incorrectly expected this to be addressed by the de-radixification but it finally occurred to me - this is specifically for a boolean widget, not a checkbox element. The values are handled slightly differently so I wired that up and it should work fine now.

bnjmnm → made their first commit to this issue’s fork.

@jessebaker reviewed my parts, I reviewed his and @hooroomoo did a pass as well so this is gonna go in.

I could only reproduce the symptoms as reported once, but I approved the code as it does address the possible cancelling of a callback we didn't actually want cancelled. However, because the issue is difficult for me (and @jessebaker) to consistently reproduce, I think it would be best if @neha_bawankar (the reporter) tried this out before this gets merged. That's just to ensure we don't need to re-open this - it doesn't appear to be the sort of change that would introduce a problem.

#9

(I found one bug that is also present in 0.x - if you have a media widget on the page data side bar, pick some images and then switch to a different sidebar and come back the selected images are no longer shown).

Good catch. I created 🐛 Page data form values-in-progress not retained for Media Library (and perhaps other) fields Active for this.

bnjmnm → created an issue.

bnjmnm → created an issue.

This was filed before form elements were refactored to use native instead of Radix and I believe this is no longer an issue, and I was unable to reproduce the problem as reported.

Looks awesome in Chrome! But in Firefox (my default), the styling gets a little messed up

🤔 I checked this out in Firefox and spotted one thing in the screenshot more (the media titles were wrapping instead of elipsising...) but I'm not entirely sure that would explain how herky jerky that screenshot was... This is how it looks for me now in FF

If you're still having problems, would you be OK looking in the FF inspector and seeing if there are any styles (or lack of) that seem to be making the most nuisance?

Yes, I think we need to revisit this. I'm actually surprised we've managed to get this far with XB with this existing order of operations. It seems fundamentally incorrect. We're using AJV to validate, which means our validation is based on the json schema, which means the thing we need to validate is the prop value, but we don't have a prop value until we run the transforms. Prior to running the transforms, we don't have a prop value, we only have a form input value, and form input values are allowed to be anything so long as the transforms turn them into a valid prop value.

The transformed versions are getting validated, but they're still valid. After digging into the implementations, the format requirements are more relaxed than I'd assumed.

If it is validating entity:node/2

• ✅format: uri passes /^(?:[a-z][a-z0-9+\-.]*:)(?:\/?\/)?[^\s]*$/i (the colon indicates a scheme and the slash indicates a path)
• ✅format: uri-reference passes /^(?:(?:[a-z][a-z0-9+\-.]*:)?\/?\/)?(?:[^\\\s#][^\s#]*)?(?:#[^\\\s]*)?$/i (uri-reference is even more forgiving)

This could be made more robust with the use of pattern: but that alone would result in the scenario mentioned by @effulgentsia where the AJV validation does fail

I'm good with this approach, I think it's more accurate to see this as a stale @todo vs functionality that needs to be changed.

wim leers → credited bnjmnm → .

wim leers → credited bnjmnm → .

Updated issue summary to reflect that is this not referring to a Dropdown list (it is not a list of select menu options) nor is the issue necessarily related to scrollability (there are several ways this can be addressed)

I'm also postponing this as it might be redundant in light of 📌 Style Autocomplete suggestions Active - based on what was reported, this should only require setting the autocomplete collision logic Drupal.autocomplete.options.position = {collision: 'flip'}; which I've already done in that issue's MR. I'll keep this issue postponed instead of closed in case #3529623 has trouble getting in, in which case we can do a targeted MR here with the Drupal.autocomplete.options.position = {collision: 'flip'}; that allows the suggestions to show up regardless of the input's proximity to the bottom of the viewport.

Further, I can't imagine not showing a hidden dependency, that would be a huge point of confusion.

I'm inclined to agree. This issue was filed because

• there were comments in core indicating hidden modules exclusion from "depends on" was the intended functionality, but the code was failing to achieve that.
• Giving the above finding it's own issue make it possible to unblock progress on a the more important issue where the above happened to be found

I was leaning towards keeping things as-is even when this issue was initially filed, and considering 5 additional years have since passed since I'm not sure there's anything that needs changing aside from possibly pruning the @todos

Ideally, the component should not fail to render when a required field is left blank.

I disagree, if a component has an invalid prop, it shouldn't be rendering. That said, the. component should do a better job informing the user why it isn't rendering, which we will address here:

📌 Invalid prop errors should be detailed in Component preview Active

bnjmnm → created an issue.

wim leers → credited bnjmnm → .

Re #11 There were styles for this but it looks like there were some conflicts with focus-visible vs visible but this should be good now.

FB is addressed as far as I can tell so I'm merging to get _none working properly again. If there's lingering bits happy to address in a followup.

avpaderno → credited bnjmnm → .

smustgrave → credited bnjmnm → .

griffynh → credited bnjmnm → .

The two PATCH requests to component-instance is due to the endpoint set to forceRefresh: true, which was added to fix the issue of vanilla Drupal behaviors not re-running if a component form is revisited. (the most common symptom was leaving then re-loading a component with a media library widget would result in the widget JS not working)

Setting forceRefresh: true provides a redux-cached version of the form until the new version is available, then the updated data (including selectors necessary for Drupal Ajax to work) replaces it.. Apparently this setting also makes additional, unnecessary requests. In the MR I updated the criteria to force a refresh if

• The query string changed (which is very expected)
• The last query string came from a different component.

Some manual testing is needed to ensure there aren't scenarios where a re-opening of a props form doesn't force a refetch.

bnjmnm → made their first commit to this issue’s fork.

bnjmnm → created an issue.