🇺🇸United States @bnjmnm

Question 1

Step 1 in the Libraries instructions includes
Merge the listing of Components and Dynamic components under the label Components.

Dynamic components are already organized into collapsible subsections sections based on their block_category:, which would mean two different forms of collection next to each other:

1. Folders (ui configurable)
2. Block categories (hard coded)

Perhaps this is fine and is why there's no additional explanation, but I'd like to confirm that.

Question 2

Is adding a new folder done via a dialog with a single text field for the dialog name, or was a different type of interaction envisioned? Are there any format requirements of the name provided?

Question 3

In the screenshot for Manage Library the root level shows only folders. Item 4 mentions "Keep support for displaying items at the root level." which I believe means un-foldered items can be displayed alongside the folders.

• Could I get confirmation of this and (if capacity allows) a visual example?
• Assuming this is the case, should folders and components be listed interchangeably as alphabetical order dictates, or should they be grouped together and alphabetically sorted within those groups? Is this different depending on if we're in Library or Manage Library?

Question 4

In the screenshot for Manage Library I don't see any controls where one might be able to rename or delete existing folders. Even if this is something to be added later, knowing where the controls might be will inform how this gets implemented.

Question 5

I could not find a folder icon in Radix. Could the svg used in the designs be provided in this issue?

Question 6

Do folder weights have any impact at this time? Based on the current requirements it looks like they're all being set to 0 and sorting is alphabetical - which is fine of course but I wanted to be sure I wasn't overlooking somewhere they get used.

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

This approach doesn't seem ideal for screenreader users, as each component being conveyed will now begins/ends with drop zone labels in all situations - not just when they are adding/moving components in the layout

The Github blog has a good writeup on how they approached Drag and Drop. It's not something we'd need to follow exactly, but it showcases several things we should consider.

One part that stands out is the form used to add/move items without drag and drop:

XB probably needs something like this as an accessible alternative for users unable to Drag and Drop. Although that should be done in another issue, it's implementation would likely require drop targets to have attributes with information similar to what was suggested here.
That's a long way of saying:
Lets not overwhelm users by using aria-label, but instead use custom attributes, and perhaps have dedicated before and after attributes that can be used by a future "position this component in layout" form

I could only reproduce #14 in Firefox though it mentioned it occurred in all browsers. The MR does fix it, and the approach makes sense, so this is going in.

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

I made a small change that (at least in my manual testing) addresses the Webform Block problem mentioned in #19. 🤞 this doesn't result in a bunch of exploding tests, but easy enough to revert if it does.

So: IMHO we should close this issue — client-side validation for block component instances just doesn't make sense.

I'm 90% in agreement here. It's arguably essential that the Block Component Instance form use form API based submit and validation callbacks like the page data form does, and that provides the necessary validation for these forms.

However (the annoying 10%), I think there's a level of clientside validation that would be nice to have & feasible. Since block requires constraints to qualify as a component-block + some settings like required might be easily mapped, there are some use cases where we might be able to associate a JSON schema specification with a given input and leverage the existing logic for SDC settings. It's not critical to add this, but it could provide faster feedback for content authors.

Might sound like splitting hairs, but IMO an important distinction to figuring this out:

What @gábor hojtsy is encountering in #19 is something introduced by the MR. In other words it's not part of the underlying problem that this MR failed to account for.

We're now invoking the submit/validate callbacks on the block form. That's overall a good thing, it's needed to massage values to the expected end result.

BUT... The problem is due to logic in \Drupal\webform\Plugin\Block\WebformBlock::blockSubmit, which is now invoked as early as adding the Webform Block to the layout.

public function blockSubmit($form, FormStateInterface $form_state) {
    $values = $form_state->getValues();
    $this->configuration['webform_id'] = $values['webform_id']; // 👈HERE IS THE PROBLEM!
    $this->configuration['default_data'] = $values['settings']['default_data'];
    $this->configuration['redirect'] = $values['settings']['redirect'];
    $this->configuration['lazy'] = $values['settings']['lazy'];
  }

It's looking for $values['webform_id'], which is NULL, instead of the default value which is an empty string.

NULL will fail the !is_null() assertion in \Drupal\Core\Entity\EntityStorageBase::load, but an empty string is handled fine.

We need to test #3513742: Machine name JS does not work in Page Data/Component Instance form after this because per @bnjmnm, this issue should address that too

To clarify: the issue as reported (which has to do with the JS logic in machine-name.js) is not something that would be fixed here, but it likely already works due to other recent changes.

There was an unrelated bug mentioned in #3513742 that is what #21 is referencing. That bug is related to problems with webform block autocomplete values that expect a machine name. It's also is what @gábor hojtsy tested in #19 - so it's already part of this evaluation - not something additional.

Ah, #11 I think this is additional confusion regarding machine name JS vs autocomplete

• the logic in machine-name.js, which is why I filed this issue, is likely addressed in #3537872, which was mentioned in #10
• Entity Reference value conversion, which is the symptom mentioned in #6 (and in the case of webform is working with machine name values) should be solved in #3523496.

I think I contributed to this initial mixup, but fortunately all of these problems are nearly or already fixed.

It's possible this works after 📌 Redux-aware form input components should be aware of direct element manipulation Active landed, which adds support for our inputs directly changing value via DOM or jQuery methods and triggering a redux/preview update.

bnjmnm → created an issue.

The solution I was already happy with, and the new tests look good 👍,.

I've updated the issue summary to make it clear this is more of a feature request.

Re #16 The term Formalize in the issue title is misleading. There's a need to support the specific functionality mentioned in the IS asap, but we agreed at standup (@balintbrews in particular) that this should not be mistaken for formalization, which is something that should be tackled far more holistically. There was a preference that this remain informal.

do we want to take the opportunity here to make the validation messages nicer and match the Figma designs?

I'm going to vote no on doing this, although I think it was completely reasonable to suggest. The areas being changed in the MR are happening at an earlier stage than anything that would impact the appearance of those messages. Keeping it in its own issue avoids this being delayed by surprise test failures due to the changes, and gives us an issue where there's the opportunity for community members to discuss concerns about the designs (however unlikely those are to occur - the designs seem pretty good)

addNewComponentToLayout was updated to address the use case that led to this issue being filed. This makes it relatively easy to programatically add new components with custom values.

This still needs a way to update the values of existing components - which is on the way...

bnjmnm → changed the visibility of the branch 3538576-stage-1-split-input-behaviors to hidden.

bnjmnm → changed the visibility of the branch 3538576-programatically-update-model-values to hidden.

Looks good, merged!

bnjmnm → changed the visibility of the branch 3538506-preview-canvas-content to hidden.

With the 3537946-page-data-only MR:

1. HTML5 validation is only used by the page data form - the component instance form is already using JSON schema. However, as you can see here, when the HTML5 validation is triggered, the messages are rendered by our application, not the browser native. This makes it visually consistient with the component instance form.
2. We have maintained the requested functionality of letting empty strings through, even when a validation warning is present, as seen by the request here. (note that when that was implemented, the focus was on component instances.
3. Notice that despite the empty field, the title in the preview is the most recent non-empty value. This may require some discussion regarding how to address (if at all) as it's different from components, but it shouldn't happen here as This behavior was not caused by this MR. I mention this as it's seems like the kind of thing that might surface during manual testing and be mis-attributed to these changes. Also note the Front End request includes the empty string, so if we did want the preview to allow an empty title, it probably requires a back end change.

bnjmnm → changed the visibility of the branch 3537946-lift-html5-validation to hidden.

Having the form display / enforce HTML5 Errors was implemented very early on - in a panic - to prevent preview updates that include preview-crashing values. Since then, we've added JSON schema validation that serves this purpose more robustly. This has me wondering if the html5 validation is necessary at all.
On a surface level, it can be confusing to have a mixture of schema and HTML5 validation on the same form.

We've also already had to create a workaround due to HTML5 and JSON schema disagreeing about what constitutes a required field violation, opting to go with JSON schema's definition.

If there's something that HTML5 validation enforces that the AJV schema checking is not, then perhaps this issue is correct to promote it's role within the form. If the HTML5 validation is at best replicating what AJV already does, then we're probably better off removing the HTML5 validation logic from onChange.

// 👀 DO WE REALLY NEED THIS 👇
// Check if the input is valid based on HTML5 attributes before continuing.
      if (e.target instanceof HTMLInputElement && !e.target.reportValidity()) {
        const inputElement = e.target;
        const requiredAndOnlyProblemIsEmpty =
          inputElement.required &&
          Object.keys(inputElement.validity).every(
            (validityProperty: string) =>
              ['valid', 'valueMissing'].includes(validityProperty)
                ? inputElement.validity[validityProperty as keyof ValidityState]
                : !inputElement.validity[
                    validityProperty as keyof ValidityState
                  ],
          );
        // We will return early unless the only problem caught by native
        // validation is a required field that is empty.
        if (!requiredAndOnlyProblemIsEmpty) {
          return;
        }
      }
// // 👀 IF THIS PART THAT WAS ADDED LATER VALIDATES AGAINST THE WHOLE SCHEMA? 👇
// Check if the input is valid based on JSON Schema before continuing.

      if (
        fieldName &&
        newValue &&
        e.target instanceof HTMLInputElement &&
        e.target.form instanceof HTMLFormElement
      ) {
       // RUN JSON SCHEMA VALIDATION
        if (!validateNewValue(e, newValue).valid) {
          return;
        }
      }

There is one bit we'd need to change in this condition, though:

if (
        fieldName &&
        newValue &&
        e.target instanceof HTMLInputElement &&
        e.target.form instanceof HTMLFormElement
      ) 

Here, newValue should be (newValue || newValue === '') so empty strings get the schema validation, too.

Re #7 discussed this with some folks including @balintbrews / @effulgentsia / @hooroomoo and it was determined that the scenario that needs to be tested here is better implemented as an integration test, as the unit would require a degree of mocking that would limit the test's ability to catch actual problems. Currently, integration tests aren't set up at all and would significantly expand the scope of this issue. The discussion concluded that we should prioritize getting the fix in to address some of the immediate needs of other projects, and focus on integration test implementation in a separate (and likely much larger) issue.

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

Got a +1 from @hooroomoo to merge the 3538576-stage-1-split-input-behaviors branch. With that in, the MR that actually implements the requested functionality should be easier to review. (it will definitely be easier to implement)

MR 3538576-stage-1-split-input-behaviors makes no functionality changes - it's just splitting inputBehaviors so the form-specific parts are in dedicated files. It should make the next step easier to review, so perhaps that can get in quickly and we'll focus on the next functionality-impacting MR.

Neat approach. Can we get a kernel test added for the authentication checker?

Yep,! There's a bunch of other tests that need updating too - I just didn't want to do all that refactoring until I got a +1 but now that I have one I'll hop on that.

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

bnjmnm → created an issue.

I'm running into the same scenario as #2 when I followed the steps to reduce, though it's not using the exact components in the report so perhaps there's something specific that can unlock the reproducibility?

Based on the error message, this could be in a few places where it should be safe to assume the resolved property exists. I suspect there's a timing issue with selectedModel being undefined when we don't expect it, but hopefully we can confirm that with a little more info.

Ran npm audit in every directory with a package.json and updated where needed.

bnjmnm → created an issue.

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

Postponing as this is likely addressed as part of the changes in 🐛 Position of some components is miscalculated frequently Active but we should hold off on closing this until that other issue officially lands and there's a final verification that the reported issue is resolved as a result.

I left two more comments on the MR that might be asking for more than two changes. None of them are functionality impacting, though, so I'm happy to approve the MR code-wise and once the doc additions and (possible) formatting adjustments this can probably go in.

bnjmnm → changed the visibility of the branch 3532618-after-session-times to hidden.

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

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

Fixed the multiple-add issue, back to MR

Current MR does not allow creating multiple components, will get that fixed

Screen recording in #14 addresses the need that was keeping this at "needs work" status.

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

It looks like, work in this previous issue wasn't enough

I wasn't aware that was in there, nice! I've run into a few instances where there was a lack of xb_stark due to recipes issues - not a manual change. It made it appear as if the UI had bugs, so it seems better to make the xb_stark dependent parts of the UI unavailable when xb_stark isn't around.

Using these recipes to install XB doesn't invoke the install hook that enables the xb_stark theme, which is evident in the tests for 📌 Provide salient warning when xb_stark not enabled Active . I'd like to be sure this is addressed before (or as part of) removing xb_dev_standard, or we at least do the steps necessary for #3538562 to land. Currently when xb_stark isn't present, it could be seen as any number of triage-burden-amplifying bugs.

I approved MR 1371 - it's a solid approach that doesn't require DOM vigilance. It's also more comprehensive as it covers keyboard interaction and submits. However, the issue is NR as this expanded functionality should probably get some e2e representation. You should be able to use the Cypress jQuery functionality to add a link to the preview iframe after it loads which would reproduce the scenario this fixes without having to deal with making a component do it.
jQuery('iframe[title="Page preview"]').contents().find("body").append(YOUR LINK[s] )

tim.plunkett → credited bnjmnm → .

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

What am I doing wrong? The cypress test seems to validate it works.

If I were to follow the steps you provided to reproduce verbatim, I'd get the same symptoms because those recipes don't invoke the experience builder install hooks that enable the xb_stark theme that is needed to render the Forms with React and get the preview-updating functionality listed. If I then enable xb_stark everything works fine in the Firefox and Chrome browsers I tested it in.

Based on the screenshot, it looks like xb_stark is enabled, but given that the symptoms reported are exactly what I encountered when following the steps to reproduce, it's hard to not to wonder if a lack of xb_stark has something to do with it.

Lets rule out the recipes and see if you run into the same issue if XB is enabled directly.

bnjmnm → created an issue.

Pushed an MR that needs some tweaking to pass CI, but this addresses the issue. Looks like if the try block is caught with anything that isn't a Validation Constraint violation, it throws the error but does not make the FE aware of it. It kinda seemed like that difference was intentional but at worst the MR can motivate us to better explain why.

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

Some of the broader concerns expressed in comment #5 are being addressed in 📌 Redux-aware form input components should be aware of direct element manipulation Active

Adding reference to 🐛 Reordering items in a multi-value field doesn't update auto-save unless the user makes another edit Active which needs this.

This will also likely address the concerns expressed in this comment: 🐛 XB UI doesn't save the media/image or fields that involve Drupal javascript. Active (the issue was closed based on the summary & title vs the broader concerns found in that comment)

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".