This fixes the problem twice-reported in the following issues
- 🐛 "There are no users matching '' error message appears after reloading the editor page Active
- 📌 Get Options as buttons in Page Data form working Active
I started the branch so I can't provide any codeowner approvals, despite this largely turning into something authored by @larowlan
Otherwise, I'd be the approver for Redux-integrated field widgets so if @larowlan wants to be my proxy for that I think that is fine.
Knew the tabledrag targeting in ajax.hyperscriptify.js wasn't great - adding a few attributes per @larowlan suggestion does a much better job of it.
The not-yet-addressed part of this is addressed by 📌 Get Options as buttons in Page Data form working Active , so that should be prioritized
@tedbow ran into what is likely this issue and narrowed it down to the changes made in 📌 Media Library dialogs triggered from page data do not have buttons yet Active
The work done so far in the MR is probably worth keeping, even though they were not related to #3506467 (autocomplete should be adding value, not label, and details contents should be in the DOM).
The parts still not working are probabl related to the #3506467 changes so now we have a place to target our efforts
The problem reported in #67 may already be represented by 🐛 "There are no users matching '' error message appears after reloading the editor page Active where some work has taken place already
I think this is a duplicate of 🐛 "There are no users matching '' error message appears after reloading the editor page Active but just unsure enough that I'm not closing this as duplicate. There's some changes there that are worthwhile, but I'm not sure any of them touch on the specific things caused by 📌 Media Library dialogs triggered from page data do not have buttons yet Active
I noticed there is activity from several people on this issue after a fix was committed and the issue closed.
If anyone above are hoping their comments result in additional changes to Drupal core, the chances of that happening are infinitely higher if it's reported in a new issue.
It is very rare that an issue is truly "reopened", and in those rare instances it's pretty much always within a few days of it being closed.
bnjmnm → changed the visibility of the branch 3484395-support-props-that to hidden.
bnjmnm → changed the visibility of the branch 3484395-wysiwyg-ckeditor5-widget to hidden.
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
I expected way more of the fields added in xb_test_article_fields to either "just work", or not work in a manner that didn't adversely impact the form if they weren't fields that had value changes.
In the case of language, options-as-buttons (radios), and possibly others, it's clear this isn't the case - they are enough to deserve their own issues IMO. In particular, if the problems impact the ability to test any fields in the form, a separate issue might be good. This issue could then be specific to adding tests for fields that either work already, or are broken in a way that doesn't contaminate the entire form.
It looks like the branch here and the MR in 📌 Get Options as buttons in Page Data form working Active are tackling the options-as-buttons issue in different places and I suspect they might be more complementary than conflicting. My preference is to see the work on option buttons here brought over to that issue. BTW I describe a wall I hit in comment 4 → but perhaps the approach here gets around that.
MR has a few bits that are necessary to get this working, but nothing touching on comment #2 yet. Some way of calling EntityAutocomplete::valueCallback or doing the equivalent will likely be needed.
- On initial load, the node author field has a value formatted like this:
username (USER_ID) - After the first autosave, the field value becomes just the user id.
- Now, when the form reloads (such as a refresh that uses autosave data) there is just a 2 in the field and when the validators are called as part of the build,
\Drupal\Core\Entity\Element\EntityAutocomplete::validateEntityAutocompletewill call$match = static::matchEntityByTitle($handler, $input, $element, $form_state, !$autocreate);and there is no title to match, because$inputis just the user ID, not the name.
I suspect this will be addressed if we address the process that converts the field value from username (USER_ID) to just the id.
The initial POST to https://drupal.test/xb/api/layout/node/{nid} fails due to the radio option being an array instead of the expected string
The option is being sent as a string on the FE
The last XB line of code before the error occurs is in \Drupal\experience_builder\ClientDataToEntityConverter::setEntityFields
$form = $this->formBuilder->buildForm($form_object, $form_state);
And during that build it eventually culminates in The submitted value type array in the XB Options (Buttons) element is not allowed. and a few other type validation issues. The $entity_form_fields sent to the build process has many similarly structured fields -> values so at first glance there doesn't appear to be anything wrong with the structure itself, so I suspect it's an issue with setting the correct validation expectations... but that is just speculation ATM. Will explore further.
This isn't a bug - it's a feature request.
The issue summary is still inaccurate. It should be presented as a desire to have a different icon present depending on open/closed state.
@julio_retkwa appreciate the attempt but the changes to the summary are not at all what was reported, the steps to reproduce and proposed solution seem like they are from an entirely different issue that happens to be media library related. Among other things you added "Added a js behaviour to scroll up on field media modal open." as the Proposed Solution despite not touching any JS in your MR or the issue having anything to do with scrolling.
I updated the IS to address this.
This also needed a manual run of the tests currently skipped for headless: prop-types.cy.js and autocomplete-props.cy.js:
This seems fine - AFAIK the only motivations behind not doing this initially is no longer relevant at this stage in the project. After merging 0.x with this I did encounter some problems that I addressed largely in XBTemplateRenderer, and also extended one of the e2e timeouts so it spanned the wait of the autosave interval + a little additional time.
Needs ui codeowner approval.
Got this working by querying from the available ref
const hiddenSelect =
selectRef.current.parentElement?.querySelector<HTMLSelectElement>(
'select[aria-hidden]',
);Doesn't feel the most solid to me, but maybe it's OK. So far seems to work.
My next step was to see if changing editors worked, and ran into a situation that will likely require nontrivial refactoring. The underlying cause may cause problems for more than just this use case.
Radix <Select> does result in a <select> tag in the UI. It is instead a button that triggers the appearance of selectable options.
Drupal.behaviors.editor very much expects a <select>, in some cases not adding the listeners unless that is the element type.
if (editor.tagName === 'SELECT') {
$this.on('change.editorAttach', { field }, onTextFormatChange);
}
The Radix select does have a hidden <select>, which is not exposed as a ref in @radix-ui/themes, but looks like it miiight be accessible if we access it more directly instead of the theme. I did some exploring and this doesn't look like something that can be done quickly, but there is a forwarded ref in the Select primitive, so it should be possible somehow. However, even if the <select> can be accessed via ref, we may still run into a limitation with some part of Drupal due to the select in the UI not being an actual <select>
Does CKEditor 5 itself also work?
The CKEditor 5 editor works - you can format the text using the various buttons like one might expect.
Do the contents of the edited-by-CKEdtor5 field get saved? That part does not appear to work. It that can be tackled in a separate issue once this lands.
It looks like this is likely related to a Chrome extension. I need to try one by one to see which. It probably means this is less urgent than I initially thought, but some of that might depend on which extension(s).
MR I just opened has logic that gets CKEditor5 to appear. It's not ready for prime-time yet but the underlying issue has been identified and there is a solution.
Ty @jessebaker
Merged and noise reduced.
Did a review, which was largely manually testing different things and confirming they are in fact fine. Added a docs request to an existing thread on the MR and updated a function name in the Media Library Cypress test.
bnjmnm → created an issue.
Merged. Thanks for the reviews everybody!
After a few days of e2e tests passing reliably, the autocomplete test introduced here began encountering the unpredictable+ambiguous "We detected that the Electron Renderer process just crashed.". So we can continue working without false-red-x's popping up, the autocomplete e2e test is bypassed on headless and the screenshot below shows that it is passing fine on headed.
bnjmnm → created an issue.
@balintbrews approved this but it looks like we still need a semi-coupled owner to approve too> @hooroomoo seems like the best candidate for that
bnjmnm → created an issue. See original summary → .
Start from the MR which adds a default value to the language select field in the test module so it validates properly.
I've added an approach that solves the issue, but my concern is on how to test it widely to make sure it works on all instance
If anyone could help me with that would be great :)
Testing it widely could be difficult, but there's a way to approach this where this wouldn't be necessary.
Instead of changing padding and max-width, the least disruptive way to do this would be to preserve the styling that is known to already work. If you make the enclosing <article> tag have position: relative;, but use a selector that isn't dependent on the .contextual-region class being present, this fixes it in a way that only requires a few moments of testing.
Bring on the tsunami of issues that will use this
Made a module that adds this stuff
bnjmnm → created an issue.
bnjmnm → created an issue.
Re #2. So 📌 [later phase] Support matching `{type: array, …}` prop shapes Postponed should definitely be blocked on this. We'll scope this at the widget JS functionality (controls show up, it is sortable, 'add' and 'remove' add and remove the correct things, etc), which should free up #3467870 to focus on what is attempting to be built there.
This test runs fine locally but not here, and I can see from the test artifacts the XB UI at least partially loads, which confirms the JS is present.
One thing I"m not sure of, though, is if the routing in the XB UI can handle sites running from sub-directories. The final test artifact is the page reporting "An unexpected error has occurred in a route." and the first thing that comes to mind is the router not being able to handle the additional /web/ in the local test site.
I was the un-rtbc'r and since then a few approvals happened. Given that this is also soft blocking some other issues I'm gonna bring it in.
Wish we didn't have to resort to this, but this should help keep things moving at least.
Assigning to Wim since they provided the initial RTBC, so it should only require looking at the changes since then.
Remove the test skipping stuff so anyone can review now.
Current solution is able to prevent the duplicate loading of assets by AJAX requests made by the UI app, but does not prevent the duplicate loading of assets if the duplication prone library requests comes from elsewhere, which is what I'm running into manually atm.
Looking for some @wim.leers help on why the test isn't passing on CI
This was addressed in 📌 Dialog + Media Library refinements to support Gin Active
Item 1 in the IS Buttons in the button panel are not visible (but can be clicked if you know where they are)
is fixed by applying
this patch →
to Gin, which seems to be (similar to but) different from what is in
🐛
Dialog styles are not loading correctly in Experience Builder
Active
. I added a comment that includes that specific patch.
It's possible this addresses Item 2, but I'm not 100% what to look for to confirm this is the case.
I am using this patch → so Gin works with the recent changes made to experience builder. This appears to take care of everything and is a change made direclty to Gin vs gin toolbar.
Confirmed that landing 🐛 "Add media" button doesn't always open the media library Active will get the new e2e test to pass
Assigning to @wim leers to help determine if these tests that check the criteria is met pass... should this go in as is and address other serializing in additional issues?
Re #92
#89 works
Patches are no longer used for updating drupal core → . If this is ready then it should be applied to the current merge request (which also needs rerolling) or create a new merge request.
The patch also doesn't have tests. Its possible the MR does but the diff is currently too large for that to be clear.
Feedback addressed and the Cypress memory issue was seemingly addressed by making the test module name shorter (???)
The Cause
Fairly recently, Experience Builder added several custom libraries that are largely identical to core libraries but with small differences to address things like CSS leakage or applying Admin theme overrides despite the XB UI not using the admin theme. We need to make them distinct libraries instead of instead of alter() ing them as the alters should only apply in XB contexts, something the library cache isn't easily aware of.
This approach works well, but it also results in multiple libraries potentially having the same assets. Drupal's AJAX system does an excellent job of preventing libraries from being added multiple times, but nothing to prevent assets from being re-added if they belong to a not-yet-added library. The specific problem reported was occurring because misc/ajax.js was being re-loaded.
There was already logic in Experience builder to prevent core libraries with existing XB equivalents from being directly added, but this was not being prevented if the library was a dependency. In this case, core/drupal.ajax was being re-added as a dependency of media_library/ui
The Solution
There might be something cleaner... but for now: drupalSettings now keeps track of all JS assets added by XB-specific libraries that load by default with the XB UI. processResponseAssets checks the list in drupalSettings and skips loading any JS already listed there - regardless of which library requested it.
To Review
This is hard to reproduce with a plain XB install, but the issue is immediately apparent in Drupal CMS when attempting to change the image in the card component.
Note that with Drupal CMS, the dialog buttons might not appear visible due to not having certain styles. This is unrelated to anything within the scope of this issue... it just happens to be in Media Library. Getting the buttons visible requires patching Gin with this → , so it properly includes all the stylesheets necessary to display dialogs.
Yep, the things needed for this to work better were taken care of as part of other issues.
Note that with Drupal CMS, the dialog buttons might not appear visible due to not having certain styles. This is unrelated to anything discussed here and requires patching Gin with this → .
I snuck this fix for this into 📌 Media Library dialog styling Active because otherwise the majority of reviews there would switch it back to NW despite being unrelated. I'm reopening because MR here includes a test, which was the only thing stopping this from landing 3 months ago. I'm not quite good enough at gitlab CI config to know how to get the compiled XB app available to Functional Javascript tests (which I used because unlike e2e they have a solid way to test file uploads)
Interesting. We had to leave aggregation off in the early days pre-Barcelona
With that being mentioned I think it's important to share I've already found the underlying cause and it's a very different set of factors than what is mentioned above. MR on the way.
I installed the Drupal CMS demo and reproduced the issue has well
I also confirmed that disabling JS aggregation makes the issue go away, which helpfully narrows down what the underlying cause might be. Will investigate further.
This is most easily tested with a Gin-integrated setup like DrupalCMS, the change is fairly easy to paste in and might be the simplest way to manually review.
Current MR gets us closer to fixing it. Currently works best when the UI is loaded and the image prop already has media.
It is still buggy when new media is added and then removed. I assume it's because the component is now removed-element aware, but there isn't yet a full equivalent for additions
There's some trickiness here (surprise!) because all of the current redux-updating is based on changes to input values, but when an item is removed via media library, this is reflected in the form/ui by removing the correspnding hidden <input /> that represents the selection.
I have some ideas on how to deal with this...