🇺🇸United States @bnjmnm

I also noted that if I tried to the link the "revision log" to the sub heading this did not work. I think it might be because the revision log was empty. If I updated the revision log so it was not empty it worked

In \Drupal\canvas\Plugin\Canvas\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::clientModelToInput
There's a bit that reconciles the host entity values
$evaluated = $source->evaluate($host_entity, $is_required_prop);

If the host entity value is empty, this returns NULL, and it results in it not being added to the model that is ised to inform \Drupal\canvas\Plugin\Canvas\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::buildComponentInstanceForm that the prop is dynamic.

With a little more xdebugging I can probably figure it out, but someone who spends more time in there might be able to do it quicker.

There is a FE bug right now where if u

add a heading component
link the 'text' prop to a field,
change the 'style' prop to secondary

Then it reverts the 'text' prop to its original static value.

After some debugging I found that after doing step #2 of linking a prop to a field, the selectedModel.source looks to be correct but the selectedModel.resolved value is wrong and is the static value. So I think because the resolved value is wrong, the FE sends a request with the wrong values to the BE and gets the wrong model back.

But also I'm not familiar with how source and resolved values are handled in the backend so hoping someone with more knowledge there knows how to fix this.

Fixed this - it required the layoutModel AND the formState stores be updated when a prop is linked

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

Setting to NR - the playwright tests have yet to fully pass, but each run has a different combination of specs+browsers failing. In the many times I've re-run Playwright, every test with every browser has passed multiple times, so this appears to be random and unrelated to the changes in the MR. I'll keep re-running in hopes of it hitting that confidence-inspiring green.

I'm setting to NR now that 1.x has the component form + template.

Did not have much of an opportunity to manually test things now that 1.x is merged - lost a few hrs trying to narrow down Playwright flakiness but it's different spec+browser combos failing on each run so I'm guessing it's bad timing more than actual regressions.

This is covered in 📌 Manage Library / Code Component UI refinements Active which was NR in experience builder but needs a bit of attention now that it is Canvas.

setting as postponed instead of duplicate in case there's a greater affinity for this issue

bnjmnm → changed the visibility of the branch 3543804-manage-library- to hidden.

This is covered as part of 📌 Have "Add Folder" form use @/components/Dialog Active which had a working MR with experience_builder and should have a canvas version shortly. Will set this to postponed instead of duplicated so this remains invisible if it's decided to address that without the Dialog support.

This is the current state. Linking works. Need to confirm if unlinking should be part of this too, and there's undoubtedly other things to fine tune

bnjmnm → changed the visibility of the branch 3541037-allow-linking-a to hidden.

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

This looks fine and now I better understand why we didn't bother with some of the helpers I suggested in the routing issue that preceded this

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

bnjmnm → changed the visibility of the branch 3538562-provide-salient-warning to hidden.

MR 1 is committed, MR 2 is in progress, the add template dialog works but needs some loading spinners for the view modes request, and the form isn't quite what I'd expect if I choose a content type that already has a template assigned to the "full" view mode.

It should be jump-onable by folks in other timezones - it all works and the TypeScript is sorted out, but needs some refining. The dialog props were set up knowing it will evenually be opened by a bundle's contextual menus, too.

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

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

Followups created as needed:

• Already addressed in this issue
• Appearance of empty folders need to be adjusted, see #note_575777.
• Components and patterns should not be draggable to the canvas from Manage library, see #note_575781
• ✨ Broaden Manage Library -> Code Tab scope Active Covers "Display both internal and exposed code components in Manage library under the Code tab. See #16". I know it was stated above that this was already addressed and discussed in #20 but I'm not seeing exposed components in the Code tab, just internal - if I missed something here sorry about the noise
• 📌 Folder style updates Postponed Covers "Appearance of hovered folders need to be adjusted, see #note_575778."
• 📌 Have "Add Folder" form use @/components/Dialog Active Covers "Adding new folders should use @/components/Dialog and handle errors, see #note_575780."
• 📌 Manage Library / Code Component UI refinements Active Covers
  • When a new code component is created from Library, the Code tab of Manage library should be opened.
  • When a code component is clicked under Manage Library, and the current route is not the code editor, the Code tab should remain selected.

bnjmnm → created an issue.

bnjmnm → created an issue.

bnjmnm → created an issue.

bnjmnm → created an issue.

media-library-component-instance has a fix snuck into an unrelated not-yet-merged MR, but moving iterations inside the testMediaLibraryInComponentInstanceForm should take care of it. (has worked thus far at least)

There's a 2nd media library test that should get a similar change.

global-regions-interact.cy.js is the renderer crashing. This was a bigger problem with more tests before we parallelized, and this seems to be the only one continuing to suffer. It certainly can't hurt to make this the next playwright port, but a quick way that might reduce some pain here is to split it into it's own test group as it seemed like the renderer crashes were related to the number of tests in a run - but I'm not certain about this so it's only something to try if it is fast.

bnjmnm → created an issue.

Failing test is just a flaky Playwright

penyaskito → credited bnjmnm → .

bnjmnm → created an issue.

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

Setting to NR. e2e tests, particularly Playwright at the moment, are a bit flaky, so don't let fails there get in the way of a review.. there's plenty that needs looking at.

Library

Manage Library

wim leers → credited bnjmnm → .

The 3530795-validation-render branch has an approach that looks like this:

The screenshot was taken before we added support to gracefully handle empty required fields - mentally replace that message with any of the many possible render-crashing errors that can't be handled so smoothly. The design is just rough, the main things to note is the unrenderable component has a clear message in its place, and the guilty prop input is correctly highlighted as invalid.

Please find the attached video demonstrating the steps to reproduce the "An unexpected error has occurred while fetching the layout" issue

This is helpful, but just to make things clear, the above is just one of many ways trigger this message. The ""An unexpected error..." is a generic message in the UI that could be triggered by any number of different errors. Detailed, helpful info already exists in the JS console, too! This issue is about bringing those details to the UI

#40 TC doesn't seem particularly Front End to me - it looks like we're allowing invalid values to be published, which can result in errors when attempting to render.

Based on the description / video:

1. A label is manually entered into an entity autocomplete widget that doesn't have a corresponding entity
2. An error then occurs when attempting to publish the content with the no-corresponing-entity chosen

AssertionError: Cannot load the "webform" entity with NULL    
                                                       ID. in assert() (line 262 of                                  
                                                       /Users/ben.mullins/Sites/drupal/core/lib/Drupal/Core/Entity/  
                                                       EntityStorageBase.php). 

From the looks of it, the block is being published despite not meeting validation criteria. When we attempt to do this in OG Drupal, it's not possible to submit:

I suspect we need to do something like 📌 Publish checks validation constraints, but not form validation Active but targeted to blocks, so content with invalid field values aren't publishable.
Created 📌 Prevent publishing when block values do not #validate Active for what was spotted in #40
Created 📌 Provide better info than "an unexpected error has occurred" Active because it's unfortunate how much helpful info is literally just sitting there to make our lives easier, but we only get "an unexpected error has occurred"

bnjmnm → created an issue.

bnjmnm → created an issue.

bnjmnm → created an issue.

In case it's helpful for additional testsl:

I'm working on the F.E. implementation of this and added 3 test modules that create setup content

• xb_internal_code_components creates several internal code components
• xb_test_patterns creates several patterns (that are essentially the same but with different names)
• xb_test_folders creates at least two folders for patterns, library components and internal code components. It depends on the modules above + xb_test_sdc

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.