🇺🇸United States @bnjmnm

avpaderno → credited bnjmnm → .

Took care of the merge conflict and it looks like all other reviews were good with the changes so I'll go ahead and commit.

bnjmnm → created an issue.

Radix / 0.x

Native / this MR

Despite adding a bunch of additional testing the end result is fewer LOC! +341 −345

Thanks for the reviews @hooroomoo & @balintbrews

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

bnjmnm → created an issue.

Between this and some other recent changes, these can even be unskipped. Nice work @penyaskito

The is always unchecked part is addressed by
📌 Confirm Semi-coupled form elements can work with State API visibility Active , the other part will likely be fixed with 🐛 Autosave publish process does not acknowledge pathauto deactivation Postponed as mentioned in #4

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

bnjmnm → created an issue.

I had this postponed on the State API checkboxes issue becausei I can’t manually reproduce the problem without the State API working with that widget. Based on the above it sounds like this or something else representative of the situation can be reproduce me - could the issue summary be updated with these steps?

It's possible I duplicated this issue with 🐛 Autosave publish process does not acknowledge pathauto deactivation Postponed , but not fully sure as the IS here specifies the issue is due to pathauto creating an alias when a pattern isn't defined for the entity type, while #3526130 is specifically about the publish process not acknowledging the "Generate automatic URL alias" being unchecked.

If they are in fact the same issue reported differently please note the issue summary in #3526130 already provides a 3 line solution that works, but notes there's probably a cleaner way to implement it (perhaps in a hook that specifies module: 'pathauto'

or maybe there are two separate problems?

At least as-reported this issue is different. The problem reported here is specifically about the publish process not respecting the "Generate automatic URL alias" setting, while issue #3525070 states the the problem is a failure to do the desired "Pathauto should skip generating a new path when there is no pattern"

I wouldn't be surprised if the solution to #3525070 occurs in the same place as the solution to the problem reported here, but there's not yet enough info to consider these issues duplicates.

Approved the MR, still needs approval from /src/ and /src/Entity/

Also noticed #15.2 was already discussed but probably worth conclusively answering 1 and 3 even if the answer is sort of implied already just to be sure we're properly acknowledging every review point

Yep the moving / scrolling seems to be one of the more common intermittent issues here. good find.

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

@gábor hojtsy the scope of this issue includes getting the pathauto widget functional. With this MR. The form elements in that widget work as expected and send the correct data for submission. The autosave data is aware of pathauto being enabled/disabled and the value of the custom alias.

However, the publish process is not processing these values correctly. This.is out of scope for this widget-targeted issue. Even if it's out of scope, I figured it can't hurt to offer an additional draft MR 3481719-states-api-with-targeted-pathauto that takes the in-scope MR and adds a few lines to ApiAutoSaveController to get pathauto working

 if (get_class($entity->path) === 'Drupal\pathauto\PathautoFieldItemList' && $auto_save['data']['entity_form_fields']['path[0][pathauto]'] === FALSE) {
          $entity->path->__set('pathauto', 0);
        }

I believe this is necessary because the publish process updates fields, but path is being updated as an entity property.
I suspect there are more holistic solutions available so the 3481719-states-api-with-targeted-pathauto branch should probably not be part of this issue's commit, so I created 🐛 Autosave publish process does not acknowledge pathauto deactivation Postponed , which can use the solution I already found as a starting point.

bnjmnm → created an issue.

The Radix version on 0.x

The native version in this MR

I have some MR feedback. I think we can reduce the total renders of the Path widget component by using refs for some items managed by state, but I don't have time ATM to confirm that yet. Will come back to that shortly.

• The MR capabilities are a variantion Pathauto does, and as a result conflicts. It that module is enabled, it will ultimately override whatever is created by this, but confusingly ly, even if the "Generate automatic URL alias" checkbox is filled, the disabled URL alias field will populated with the autogenerated string. (note that this checkbox does not yet work for everyone - I tested this with a branch from issue #3481719 which gets the State API working). I believe pathauto might be bundled with some systems that use experience builder, so we'd need to account for this
• Re #15.2 "And what about SDC props that use type: string, format: uri-reference" is this a concern here? I believe those formats use the widget for link fields, not path fields.

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

This looks good and there are enough related followups happening to clean up any loose ends in MRs with a more manageable number of files changed.

Thanks @quietone (and apologies to @geoffreyr ) I may have spoke too soon in #7 . I'm glad you responded as it motivated me to double check my findings and it turns out my IDE was delivering incomplete info.

There is in fact a Core API and it's fine to have this as part of the drupal-apis → guide.

Something to consider adding to the intro page → is an explanation of what this API does vs the Token module. More often than not, I wouldn't want this kind of pre-emptive clarification in a core API guide, but in this case it's an incredibly popular module that makes use of this identically-named API so it might be beneficial.

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

Everything else on the APIs page → is a core api - it works without any additional modules. I'd prefer a module API not be on a core documentation page, but if there are strong feelings to the contrary it should ad least be incredibly clear, probably on all pages, that installing the contrib Token module is necessary for any of this to work.

Feedback addressed

Moved from root section to documentation

Moved from root section to documentation

Moved from root section to documentation

Moved from root section to documentation

Moved from root section to documentation

Moved from root section to documentation

Moved from root section to documentation

Moved from root section to documentation

Moved from root section to documentation

Moved from root section to documentation

Moved from root section to documentation

Moved from root section to documentation

Moved from root section to documentation

Good news
It looks like changes in 0.x that happened since I came up with the solution ~3 months ago means there's way less backend hacky stuff needed.

Bad news
Getting the e2e tests to pass reliably - despite the IRL version working fine - is not going smoothly at all. I'll just set this to needs work until I get that slop more agreeable.

bnjmnm → changed the visibility of the branch 3481719-confirm-semi-coupled-form to hidden.

Thi

Yep the component-operations.cy.js fail is something I've seen elsewhere and unlikely to be related to the changes here.

How much of the current MR is due to 📌 Allow 6.x version of justinrainbow/json-schema Active not yet being in? It's mentioned in a few areas
but I'm not 100% what would be different once this is in.

The main thing I'm wondering is, once that lands, would we still need to define the format in the front end? If that's the case there should be explicit explanations of when that's necessary vs the established approach of using the schema from the BE-returned component prop definition. The BE was previously the source of schema truth and I'd like to know when/why that should be deviated from. It's possible this is info is implicit in the docs already in the MR, but I couldn't parse with certainty.

Some FB in MR

The MR enables force refetch which initially loads the fast cached version that was satisfying but didn't work with Drupal Behaviors -- once the freshly requested version is available, it quietly replaces that the instant-but-useless version providing of it feeling fast and, well, actually working.

This introduces a bit of theoretical e2e trickiness as it means selectors can be available before the JS in the form works - but this is only an issue on repeat visits to a component and there's currently little to zero representation of that in e2e.

To anyone reviewing, be aware that - on a given page load - the first time opening the component instance form for a given component still takes time. That has always been the case as the Redux cache is warmed by that initial visit. Those revisits should be quick as the era prior to 🐛 #ajax config not reliably implemented for consecutively added components Active as this brings back to the behavior of getting the form it from the Redux cache when available... but with the key difference of theatcached form being replaced by a functional one ready.

Clarified in MR what I was requesting with the new describe()

Over the past few months there were a few discussions where concerns expressed about having JSON schema validation occur on both the back and front ends, because even though they are evaluating the exact same schema, there is the potential for a difference because the validation is performed by different validators. For that specific situation I think there's far more benefit to using the two validators than the risks it introduces. For this MR, however, I'm not sure yet if the tradeoff is worth it.

What I'm seeing here is significantly more bespoke than the above, and could be more difficult to defend. In this case, it's hand-replicating a property constraint already defined in \Drupal\path_alias\Entity\PathAlias::baseFieldDefinitions, and it's doing it based on field name vs something more generalized. In the case of path this might be fine

A more scalable approach might be converting regex property constraints like this to HTML5 pattern attributes and possibly using setCustomValidity for the messages. If such an approach doesn't happen, at the very least there should be some documented justification why both the replication of logic and the name based implementation are acceptable over a more scalable/abstracted approach to avoid unwanted precedence setting.

TLDR

• Ideally we'd find a way to translate existing constraints to html attributes
• If the more bespoke approach is used , there should at least be justification comments accompanying the code

Requested a few changes in the MR.

bnjmnm → created an issue.

Sorry, dupe

bnjmnm → created an issue.

With @larowlan approval this can be merged.

Merged.

Feedback addressed in a way better than I could have hoped for

Merged!

from 🐛 #ajax config not reliably implemented for consecutively added components Active

The problem is due to Redux createAPI() caching requests to xb/api/form/component-instance -
Any component form visited the first time will work.
If the component form is revisited in the same page visit, it will return the cached version which does not have the same selectors as those in drupalSettings.ajax and the ajaxification of elements such as "Add media" does

In other words, the forms loading quickly was a side effect of the problem that resulted AJAX being broken on repeat visits. By skipping some of the work necessary for the form to function, it was able to appear faster.

Even prior to #3520971 the first visit to any component instance form had this loading process, but it probably didn't feel like much of a performance lag because it usually occurred when the component was added to the layout.

If we want to have repeat visits to the same form load at speeds comparable to when they were not being fully initialized, some approaches come to mind, none of which seem like slam dunks. Note that the perceivable wait is not due to the AJAX initialization in JS, but the back end rendering a form with the expected selectors.

• Component instance forms are not destroyed if a new one is loaded, but instead hidden, and then un-hidden if their component is selected. The risk of duplicate IDs alone makes this risky, but there could be ways around it.
• Override the AJAX system so previous selectors are cached and perhaps indexed by form build id so if the form returns the selectors in the Redux-cached version are used instead of the fresh ones calculated by the form build process
• During the render process, identify component forms that have selectors targeted for processing by drupalSettings.ajax and flag these as the only ones to not be cached by Redux. This wouldn't eliminate the form-reloading altogether, but it would make it so it only happens on forms that require that additional processing.

Added 3519734-ref-approach, first commit is an intentionally failing test and the second commit is a version of the ref approach suggested by @omkar-pd in #14

bnjmnm → changed the visibility of the branch 3519734-blur-on-the to hidden.

Small request in the MR - but basically this looks good.

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

But I found a bug when testing with a JS component that has both the image prop and a text prop. Here in this GIF, I removed the media image, but then the text prop content also disappears from the preview, but it still exists in the form. And then when you type in the text prop, it does return to the preview.

I expanded the two e2e tests at the end of media-library.cy.js to include this scenario and they are passing. Perhaps the issue was resolved by a recent commit - many bugfixes recently.

wim leers → credited bnjmnm → .

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

Now that the issue summary has been updated and matches that I took to reproduce the same symptoms, pay special attention to the steps I've bolded

1. Create content in Experience Builder that contains a required image field
2. Leave the image field empty (do not upload an image)
3. Publish the content
4. Refresh the page
5. Attempt to upload an image to the image field
6. Observe that image is not showing up in the preview and is lost after refreshing the page

In other words, this is a problem that only occurs when we publish content that has an empty required field, which IMO shouldn't be possible in the first place. For simple-shaped fields, client side validation already prevents empty values from being written to autosave, so the publish process doesn't have to enforce it in those instances (but presumably can as it uses validation constraints). For fields such as image with more complex shapes, it looks like there might be more needed to enforce them being required.

If I add cache busting strings to the getCodeComponents and getComponents endpoint requests, the problem goes away. The underlying problem might only be with one of the two, but that narrowed things down pretty darn quick. It looks like the BE is equipped to provide us what we need, and the FE needs to change how it asks for it.

I spotted one small thing mentioned in an MR comment that I think I can just remove, but I wanted to confirm with the original author. If it's as simple as that, removing and re-rtbcing seems fine to me and I can commit.

Or we could use useRef to fix the issue

Good idea. Lets do that to minimize useEffect, even if it means a bit of extra logic.

Test fail appears due to the change that removed the The content has either been modified by another user..." issue see comment

Note: include a mention of function xb_stark_preprocess_field_multiple_value_form which should be there after 📌 Add e2e tests for multi-value textfield widget in page data form Active as that is some extra lifting that wouldn't be needed without Radix & the templates were more analagous to our Twig ones.

Added two followups

• 📌 CKEditor 5 - expand format select functionality Postponed (this is more relevant to page data forms and is a refinement that can be done later while the significant improvements go in here)
• 📌 Document use cases where XB's field widgets don't 1:1 match how they would work in a twig theme Active (@lauriii requested this be done in a separate scope)

bnjmnm → created an issue.

bnjmnm → created an issue.

Only test fails are a common random failer. Will click retry and it may be green by the time anyone sees it.

Spotted a few little things mentioned in the MR

Upon discovering that much of the implementation I was fighting against was done due to lack of designs vs something intentional, I gutted it and replaced it with something that required fewer hacks & workarounds. It's not suddenly simple, but all the steps are very clearly geared towards the implementation the CK team's CKEditor 5 React component, instead of wiring disparate parts together.

Functionally this is basically ready to go (I'm sure there will be some changes requested in review) but the following need to be considered

Current Limitations

• The dialog that appears when the text format changes is not currently implemented. For the component instance form this isn't an issue since there's only one format. It could be taken care of in a followup
• The text format info below the field does not change based on the format changing. This is nothing new, just more noticeable with a fully working editor. This could be done in a followup

Documentation Needs
This could take a while for a few reasons

• Documentation that properly addresses the concerns expressed in #24 and #26 needs to provide context beyond CKEditor5 implementation. For example, there's arguably more complexity due to using Radix instead of fully authoring our own templates. Highlighting one example without sufficient context can lead to overattribution and misunderstanding. This is information worth documenting, but it's a big topic that needs to be addressed thoughtfully. So this could take some time especially because....
• It's likely it will require several rounds of feedback much like this documentation issue did: 🐛 Experiments in rendering Twig as React Active

Does this need review from me, @bnjmnm? 😇 Would like to land this one! :D

Not yet. #24 & #26 opened a can of worms and this will be another few days at least, potentially longer depending on how many rounds of revisions are requested on the docs.

Also, with the current solution in the MR any removed image reverts to the example when one is available, otherwise it is removed from the preview. The IS suggests there might be interest in different logic depending on whether or not the image prop is required. That should be a feasible expansion of the current logic if needed.

The component used for the review in #18 is a custom code component. I think it's still worth reviewing with the included-with-experience-builder components the MR was tested with as it would be good to get feedback on the solution approach sooner than later.

Once I have access to & can successfully import the custom component from #18 I can see why that is running into problems, but that probably should preclude getting eyes on the solution that has solved the reported symptom for the components that are available to anyone with the Experience Builder codebase.

@lauriii Id like to try and reproduce your findings to narrow down what is going on, but it looks like the component you are testing with is called "Image with text", which is not one available in the experience builder codebase.

The MR was tested with the various image-including components provided by the xb_test_sdc module, as well as the standard image component. I'd like to see what is different about the one in #18 and figure out which side things need to be addressed.

If I perform the steps as written, such as with the image-optional-with-example component from the xb_test_sdc module I can't reproduce the problem reported. Upload works fine.

If I perform the steps per the gif in the issue summary, which appears to use the image sdc from Experience Builder's default components which has a required image, I do run into problems such as being able to publish despite a missing required field, and the preview is then completely empty which is different from what was reported but perhaps the same underlying issue.

Top of Image SDC defintion

$schema: https://git.drupalcode.org/project/drupal/-/raw/HEAD/core/assets/schemas/v1/metadata.schema.json
name: Image
props:
  type: object
  required:
    - image

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

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

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