Sorry, dupe
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.ajaxand 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.
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 → .
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
- Create content in Experience Builder that contains a required image field
- Leave the image field empty (do not upload an image)
- Publish the content
- Refresh the page
- Attempt to upload an image to the image field
- 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.
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
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.
The current MR is more unnecessary changes than necessary ones. It might be easier to start a new one.
Also, any e2e tests enabling Claro as the admin theme should have that line removed since it is now doing that by default.
(I set the Component to "Miscellaneous" because I'm unsure what this actually is. Closest seems to be autosave but this is about publishing - a separate process that is predicated on autosave, but is not autosave)
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.
wim leers → credited bnjmnm → .
@larowlan suggested reverting this due to intermittent reliability here . I opted to create a new branch instead 3517859-make-test-more-stable so we can build off what is there + there's a comment pointing out how the wait() seems to fix it. There will likely be a @todo pointing to this issue in 0.x shortly.
Wonder if this is still needed?
I'll make the call. These CSS moving efforts were worth pursuing when Drupal 8 was in development - but now that it's been around close to a decade such a change would be more disruptive than beneficial.
All set.
Pausing the NR - spotted something locally that could be improved
Did not mean to switch the priority
wim leers → credited bnjmnm → .
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
This is e2e testable but might be worth considering holding off on that since we might be switching testing libraries in the near future, and this is only a few lines in one file that can be manually tested pretty easily.
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
Sorry @ anjali your name is right next to "Unassigned"
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.
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.
MR has it working, still need to add tests.
@liquidcms A true revert would either be a patch or overriding the two JS files updated but the quickest way to go back to the earlier behavior is probably to disable the scroll lock immediately after it is activated.
in a custom module add a jQuery event listener on 'dialog:aftercreate', which is triggered immediately after the scroll lock is added.
In the callback, run the same code that is run when the modal dialog closes
const $scroll = $element.find('.scroll');
if ($scroll.length) {
bodyScrollLock.unlock($scroll.get(0));
}
This might not be it line-for-line, I didn't actually try it, but this approach should work - fortunately an event is dispatched right after the scroll lock so it's quite available for disabling.
👏
The followup MR was straightforward and welcome, added.
Went ahead and addressed the feedback from me & @longwave
Ideally someone else should at least look at the bits I changed.
This is what it looks like:
And I'll set to NR once the tests that pass locally can pass on CI 👹
I'm running into the same test failure locally , which is an existing test that also deals with the multi-value textfield widget. It failing on the portion of the test where the items in the multivalue field are reordered. The dragging of the lower item to the top no longer seems to register.
This still works manually, though, so hopefully it's just a matter of tweaking widget-multivalue.cy.js a bit. Unassigning myself for now.
Recategorizing - this was not a bug (i.e. something purported to work that doesn't) but the next step of a work in progress. Either way, this made it apparent that this was something that required higher prioritization.
If everything is ultimately based on the target id, then #17 is fine.
If not, then it would run the risk of missing images on case sensitive file systems. I'd need to debug around to say one way or the other, but perhaps someone else already knows whether or not this is the case.
It looks like there are changes in useCopyPasteComponents.ts that seem adajcent but not directly related to the reported issue. Could a comment or two be added in the MR explaining what the refactoring there is for?
The MR looks good, but could this be expanded to include a checkbox with a default value unchecked, in addition to the existing one with ['value' => 1],?
#41 provided screenshots of the status quo - there are * without the additional explanations requested in the issue summary.
Unless specified otherwise Needs screenshots is asking for screenshots of the implemented fix.
#14.1
Addressed in the MR.
#14.2-3
Will be addressed in
📌
Address CKEditor5 "more items" bar contents not being fully visible
Postponed
#14.4
Will be addressed in
📌
Dialogs opened by CKEditor5 need to be rendered with admin theme
Postponed
Assigning to @larowlan as this is familiar territory for him, but @wim.leers also offered to look into this if that isn't feasible.
If this is the case, why don't we make our client automatically convert every pattern to a regex, which then would allow to use the /…/i syntax PHP supports?
I don't think this would mitigate this particular situation. The issue is that Drupal Core allows adding media that has caps characters in file extensions. Uploading new media items like some-pic.JPG is not performed anywhere that ajv even runs.
We could modify the AJV logic so it sees caps-in-extensions as invalid and thus not making the back-end-breaking PATCH request, but it doesn't seem ideal to not support caps-in-extensions media at all in XB when it works everywhere else in Drupal. I could also see people being frustrated that such media is available to select but they're unaware it's not valid until choosing it and seeing a validation message in the input.
TY
This commit introduced ~12 files of unrelated changes. Since the actual fix is likely a single-file change, it may be worth starting a new MR vs trying to untangle all the unrelated changes.