Support props that can use wysiwyg widgets

Not much point working on this until we can get CKEditor 5 loading on textareas 📌 CKeditor not loading on formatted text fields in the content entity + component instance forms Active so postponing on that

The current MR is outdated to the point that it's probably not worth continuing, and will likely be less relevant after #3512867 so I'm going to hide that one and recommend a new MR starts once we have CKEditor 5 working in the fields that are configured to use it. #3512867 already has it the editor properly loading with formatted text fields in the Page Data form

bnjmnm → changed the visibility of the branch 3484395-wysiwyg-ckeditor5-widget to hidden.

bnjmnm → changed the visibility of the branch 3484395-support-props-that to hidden.

#5++ — thanks so much for assessing and summarizing this issue's status, @bnjmnm! 🙏😊

Per #3512867-11: CKEditor 5 not loading on formatted text Field Widgets in the content entity form → , retitling this to more precisely reflect this issue's scope.

📌 `StringSemanticsConstraint::MARKUP`: agree how SDC prop JSON schema can convey it should be markup, and Needs review is in.

I reviewed #3512867 to help it land: #3512867-14: CKEditor 5 not loading on formatted text Field Widgets in the content entity form → .

This will

📌 CKeditor not loading on formatted text fields in the content entity + component instance forms Active is in!

MR has it working, still need to add tests.

#13: exciting! 😄🤩

Note: the "inline" ones don't need to work, as described in ✨ [later phase] Make `x-formatting-context: inline` work Postponed . Sorry for making that not more clear, @bnjmnm :(

There are PHP tests failing due to there being commented-out props in the all-props sdc. These are props unrelated to the work happening here, but their presence results an error occurring any time a value changes on any prop in all-props. It is being worked on in this issue: 📌 `StringSemanticsConstraint::MARKUP`: agree how SDC prop JSON schema can convey it should be markup, and Needs review

They're commented for now so the work being done here can be reviewed manually and the CK5 e2e tests pass. That should be enough to get this reviewed, but we shouldn't commit until #3467959 lands and we un-comment those props.

🏓 Review posted!

It is being worked on in this issue: […]

That issue is closed, so did you perhaps intend to link to another issue?

I did some manual testing of this and noticed after the first update of the preview (in response to an edit in a CKE field) there is a flicker as the form is rebuilt and then CKE is reattached. And from that point editing the field value doesn't trigger an update anymore. Here's an example showing I've put 'Hi dog what's up more edits' in the field but it doesn't trigger an update in the preview.

\

This turned out to be a pretty big thing to address, but it's good this came up and I have the underlying issue fixed. Instead of relying on Drupal behaviors, this is now using the Ckeditor5 React Integration.

Now the editors work without flickering and the preview updates as you'd expect. However, I've been at this for far too long and the code is a bit scary looking so it's switched to draft. There's some cleanup + the format switching will need to be refactored a bit to work with the React-rendered CKEditor5 components. I think this is for the best as CKEditor5 is probably too complex to graft the Vanilla JS version onto our React forms if there's an official React solution available.

I'm unassigned myself in case someone wants to jump into the chaos, but I'll be back at it tomorrow.

Sorry @ anjali your name is right next to "Unassigned"

The flickering reported earlier wound up being a pretty big deal and I switched out the drupalBehaviors grafting to instead use the React solution from CKEditor themselves.

The only failing e2e test is the recently addedfield_xbt_entity_ref_tags which was added to day and should(?) be unrelated to anything here.

The MR should probably have gotten another once over from me before switching to NR, but I gotta leave the computer + this is high priority and probably good to get some eyes on it

Pausing the NR - spotted something locally that could be improved

@larowlan already did a detailed review, so I won't :)

I want to zoom out and look at what our intent/hope is with ("keep existing widgets working without significant changes") versus the reality here:

I switched out the drupalBehaviors grafting to instead use the React solution from CKEditor themselves

• On the one hand: 👏 for pragmatism!
• On the other hand: 🤔 for this sounding like we'll have to do one-off things like this for more widgets.

Then again, CKEditor 5 is a massively complex application on its own, which Drupal's been embedding, and now we're embedding it into XB, another complex app on its own!

So: can we be confident that this deep special-casing will be only necessary for very complex widgets like this one? Is it extra complex because we're only going to allow these 2 specific locked text formats/editors in component instances? IOW: can we document why this is so complex and special? 🙏 A new section in docs/redux-integrated-field-widgets.md perhaps?

I want to zoom out and look at what our intent/hope is with "Redux-integrated field widgets" ("keep existing widgets working without significant changes") versus the reality here

Maybe this should be a separate issue? This is higher level than just CKEditor5 , and our decision to use Radix has necessitated far more complexity than anything in this issue.

I also think this is based on an assumption that this introduces unprecedented special-casing which I don't really think it does.

So: can we be confident that this deep special-casing will be only necessary for very complex widgets like this one?

• It's not particularly deep special casing IMO. The majority of what might be seen as complex is code copied from core's ckeditor5.js. If the code wasn't encapsulated we could have called it directly and reduced the MR size significantly
• What is largely happening in the MR is I opted to use an already-available React component recommended by the CKEditor5 team. It's an existing React solution for exactly what we need. The CK5 implementation has required less bending-to-our-will than the decision to use Radix has required.

Is it extra complex because we're only going to allow these 2 specific locked text formats/editors in component instances?

No, nothing in the React or semi coupled code was any more/less complex because of the that. It just works with whatever is made available to it.

There was additional complexity due to the design decision to hide the format select element behind a popup and render that element with Radix instead of making it a standard select.

OW: can we document why this is so complex and special? 🙏 A new section in docs/redux-integrated-field-widgets.md perhaps?

I still don't think this stands out as novel in its complexity

If documenting additional complexity in the Render Array -> React layer seems beneficial, justifying the complexity introduced by Radix should probably get the emphasis, which would be out of scope here. I think this could be worthwhile to do in a separate issue.

This is higher level than just CKEditor5

It is a higher-level question, indeed, but it is prompted by this MR adding a lot of extra JS code to make the "formatted text" Field Widget work within XB. That's why I'm asking for documentation in docs/redux-integrated-field-widgets.md so that somebody who joins the Experience Builder project might be able to learn/understand how all this fits together, and why for certain field widgets extra work/complexity is justified.

and our decision to use Radix has necessitated far more complexity than anything in this issue.

😞 I did not know that. I don't see anything about that in either docs/redux-integrated-field-widgets.md nor docs/shape-matching-into-field-types.md.

due to the design decision to hide the format select element behind a popup and render that element with Radix instead of making it a standard select.

IIRC this was not really a design decision, but just a choice @balintbrews made many months ago because we didn't have designs!

I'll review the back-end pieces of this MR. But my concerns above WRT FE pieces still exist. I do fully acknowledge most of this is a black box to me already, but I've been able to conceptually reason about it. This MR makes
If @larowlan, @effulgentsia and @balintbrews are comfortable with this landing without docs, then great!

Per #26. Back-end review completed.

Manual testing: this works beautifully! 🤩

Self-addressed all my remarks, except for two: 1, 2.

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

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.

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

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)

Aiming for RTBC since it was so very close in #27 already!

Removing tag, that's now going to be handled in 📌 Document use cases where XB's field widgets don't 1:1 match how they would work in a twig theme Active .

daterange_default detour

@bnjmnm must've been testing with the (optional) daterange module installed and hence ran into a one-line logic bug introduced by 🐛 Component transforms need to be per sourceType, not per component prop Active . To get past that, he defined an empty client-side transform for the daterange_default widget, which then resulted in it being claimed to be working in \Drupal\Tests\experience_builder\Kernel\EcosystemSupport\FieldWidgetSupportTest, which then undermined the value/meaningfulness of that test … because it doesn't actually work (and I spent ~30 minutes debugging and trying to get it to work, which is why there's now a detailed analysis of where it goes wrong) 😅
To avoid that information to go to waste: ✨ [later phase] Support `daterange_default` field widget in component instance form Postponed .

Manual testing

Works great! 🤩

wim leers → credited larowlan → .

Argh, can't merge because package-lock.json was just updated in 📌 Compile Tailwind CSS globally for code components Active . Can you do that, @bnjmnm? 🙏

Making #31's follow-ups findable.

🥳