🇺🇸United States @bnjmnm

Where I'm leaving off today:

• 😎 Media library widget/modal now works right in the page data tab. You can remove media and add new media etc.
• 😎 Media library value changes in the Page Data form DO get sent to the Autosave Controller
• 🤦 The values don't survive ClientDataToEntityConverter yet. I left a comment on the MR - might be an easy thing once @wim.leers has a look?
• 🤦 something in the changes done so far have broken the ability for props-form-ML-widgets-FOR_IMAGES to update the layout when they change. It's a media library widget being used unconventionally so I'm not shocked it might be impacted by getting things status-quoey. Not too worried about it but it's currently an issue.

It looks like the hidden build_id input in the form is initially getting the new build ID, but it changes back to the original. Gonna try adding a mutation observer + ref to ensure those changes stick.

re #16

Checked in drupal 11 claro theme, not able to reproduce the issue.
below is the screen shot of foucs highlighting the empty label field.

You literally reproduced the issue. In your screenshot, the Text (plain) option is focused (notice the green outline), not the label field. While it's true the label field is red, it is not the focused element.

Where this is currently at:
IN THE PAGE DATA FORM
The media library widget, if it's empty, will open fine, with buttons and it is possible to add media.
HOWEVER, if I am using the XB UI on a node where the ML field already has a value, I'm not able to open the Media Library dialog via "add media" if it is preceded by removing the media. I've narrowed this down to the AJAX rebuild not being aware of the media removal - it thinks the available slots are still full so it sets "Add media" to #disabled which prevents the dialog from opening due to #disabled not being a valid triggering element.

When these steps are performed outside of experience builder, the removed media is known because form/form_state are properly restored from key_value_expire on the AJAX rebuild. When this happens in XB, there is an attempt to do this, but the request has a build_id different than the one needed to retrieve the cache from key_value_expire.

There's some additional info in the MR explaining various changes.

After removing a media item, an AJAX refresh occurs, \Drupal\media_library\Plugin\Field\FieldWidget\MediaLibraryWidget::formElement checks $referenced_entities = $items->referencedEntities(); and correctly identifies the number of referenced entities as 0. This is evident in the UI because the "Add media" button is visible and accompanied by the following message which is only

Opening media library. One media item remaining.

HOWEVER

When "Add media" is clicked \Drupal\media_library\Plugin\Field\FieldWidget\MediaLibraryWidget::formElement runs again, including the call to $referenced_entities = $items->referencedEntities(); , but $referenced_entities is returning a value as if the Remove never took place, so the widget concludes there are no more $remaining, and sets the opener button to #disabled which makes it ineligible for being seen as the triggering element.

Assigning to @wim-leers without the expectation of pushing it the whole way through, but if he can dislodge the button triggering that's great (but if other priorities emerge I don't feel stuck on this so also happy to resume this after the US holiday .

This is a good fix! one small change to tests requested.

The buttons weren't appearing because we were setting #access to FALSE on any actions arrays in 'experience_builder.api.entity_form' adding a check so it only does that if the form is rendered in xb_stark allows the buttons to show up

NEW CHALLENGE
I'm running into problems when the following is true

• If the entity form has an image field AND a media library field
• The media library field has a value, so remove must be run before I can Add media

When 👆is true and I click Add media I get an exception from \Drupal\file\Element\ManagedFile that originates from a call in FormAjaxSubscriber because it thinks the triggering element is the image field's AJAX upload element instead of the Add media button. The subscriber runs a callback intended for a ManagedFile button, but the request contents are for opening the Media Library dialog and it errors out. It's pretty specific so I'm sure I can find a specific cause, but if this rings a bell for anyone LMK.

Was going to merge but I'm getting a Merging failed. when I attempt despite being current with 0.x will try later if someone doesn't swoop in and take care of it before that.

Optional is AOK, the field being required was just a reflex for when I make fields in test, but not fully necessary for the test.

jessebaker → credited bnjmnm → .

Ah rats, adding a media library field to the article content type set up by tests winds up messing with assumptions in some kernel tests.

bnjmnm → created an issue.

As I start to look into this, I'm running into an issue where if the

• Page Data form has been visible at any point
• That form has a media library widget (does not need to be interacted with - it just needs to have been rendered)

Then if we switch to the props form, the media library "Add media" button will not work

I logged the differences in the options used by ajax.$form.ajaxSubmit(ajax.options); between a working "Add media" and a broken one (some hashes decoded for your convenience) https://www.diffchecker.com/45TipW82/

They are largely the same, but ajax_page_state[libraries] is quite different
WORKS

ckeditor5/internal.drupal.ckeditor5
ckeditor5/internal.drupal.ckeditor5.codeBlock
ckeditor5/internal.drupal.ckeditor5.emphasis
ckeditor5/internal.drupal.ckeditor5.htmlEngine
ckeditor5/internal.drupal.ckeditor5.image
ckeditor5/internal.drupal.ckeditor5.table
comment/drupal.comment
core/ckeditor5.autoformat
core/ckeditor5.blockquote
core/ckeditor5.horizontalLine
core/ckeditor5.link
core/ckeditor5.list
core/ckeditor5.pasteFromOffice
core/ckeditor5.removeFormat
core/ckeditor5.sourceEditing
core/drupal.autocomplete
core/drupal.collapse
core/drupal.vertical-tabs
experience_builder/extensions
experience_builder/xb.transform.cast
experience_builder/xb.transform.dateTime
experience_builder/xb.transform.firstRecord
experience_builder/xb.transform.link
experience_builder/xb.transform.mainProperty
experience_builder/xb.transform.mediaSelection
file/drupal.file
filter/drupal.filter
media_library/widget
menu_ui/drupal.menu_ui
node/drupal.node
node/form
path/drupal.path
text/drupal.text

DOES NOT WORK

core/internal.jquery.form,
experience_builder/xb.drupal.ajax,
media_library/widget,

Given that the non-working one is much smaller, I suspect the AJAX request is reloading a JS asset that is actually already present. This has been the cause of the exact same symptoms in the past, so that's where I'll poke around FN

Just to clarify, what was spotted in #11 was mentioned in the summary.

I feel it's important to clarify it is something that props-form-triggered media library widgets have never been capable of doing, and not an issue introduced by any of the theming efforts, nor should it be within the scope of those. The need to get this working should be reflected in the issue queue, of course, but (at the risk of nitpicking) it's not a followup to this issue - it's just one of many things that need to happen as part of constructing Experience Builder.

But yep lets be sure it is documented and findable in the issue queue 📌 Media Library dialogs triggered from page data do not have buttons yet Active

bnjmnm → created an issue.

Tests added in 📌 Media library e2e should include multiple media replacements Active

bnjmnm → created an issue.

The "add media" button can get a little flaky if it is used multiple times, not related to the changes in this issue. There's an NR merge request that addresses that here: 🐛 Image can only be replaced one time before "add media" stops working Active

The e2e tests need to be refactored because the less aggressive variable replacement means I cant use CSSStyleSheets the same way in the test. This can be reviewed without waiting on those tests, though.

The issue spotted in #9 made it apparent that I needed some additional changes for it to work right with aggregation, which is fixed in d7e0ea26

The trickiness is due to single variables being assigned new values in the same css file that then uses those variables. It's no problem when they are separate files, but when aggregated together there needs to be additional logic to ensure the dialog scoped version manages the variable values correctly.

Thanks @hooroomoo! looks like #9 happens with aggregation on. Time to dig further.

Discussed with @lauriii that it would be OK to provide a non-disruptive fix that includes an arbitrary wait, and we can seek out the robust version in a followup: 📌 Replace arbitrary wait in attachBehaviorsAfterAjaxing with an occurence based one Active

Note the test failures in the pipeline are unrelated to the changes in this issue. You can see PHPUnit passing on the previous major runs

We discovered #7 was due to triggering the media library from somewhere other than the article props form, which is currently the only supported way to do this, but will soon be available to other uses 📌 Exapand Media Library admin theme rendering beyond props form. Active (that issue is ready, the test fails are unrelated to the MR)

bnjmnm → created an issue.

XB Page - Page Data (no buttons - as was the case pre-#3471978)

Article - Page Data (no buttons - as was the case pre-#3471978)

XB Page - Props (the "add media" button does not work reliably, which is not in this issue's scope. It was the case pre-#3471978 and will likely be fixed with the solution reported in 🐛 Image can only be replaced one time before "add media" stops working Active

Or, do you feel this needs no further review, @bnjmnm?

There should at least be one additional person manually confirming Gin seems good and Claro didn't regress. There's still the drop shadow mystery I need to figure out too, but could see the benefit of getting the significant improvements provided here merged (it goes from unusable to completely usable) and the fine tuning can happen with a bit less urgency and perhaps a good newer-contributor issue.

@niranjan_panem that is correct because, as the issue summary states Claro's file-link template always displays file_size

The template is where the issue lies

The preprocess in the issue summary was explaining how other themes addressed this in the past.

Gin targeted refinements are happening here 📌 Dialog + Media Library refinements to support Gin Active

Most recent changes to the MR addresses the buttonpane weirdness. The drop shadow still isn't getting in for some reason - I still need to figure that out - but I'm going to switch to NR so interested parties can start looking at it.

I have this addressed in 📌 Dialog + Media Library refinements to support Gin Active but I'll hold of on closing this issue just in case that one stagnates for some reason

I uploaded a patch → that Gin will need to make it work (and will file an issue for it - the closet issue is filed in Gin toolbar and not gin)

Most of the support can be taken care of on the Experience Builder end, and the MR is full of that

Experience Builder

Regular Gin

bnjmnm → created an issue.

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

bnjmnm → created an issue.

I found an additional problem and created 🐛 add_css override should support var replacement for variables declared within the same file Active for it- this is something we would have only run into if aggregation was enabled - and I'm guessing most of us have it turned off. The symptoms are fairly mild: visual differences that don't impact functionality, but the fix shouldn't be hard and the IS documents the underlying cause.

I wouldn't be surprised if there are a few more of these. This is a huge change, and it's among the reasons I documented the heck out of it 🤓

bnjmnm → created an issue.

bnjmnm → created an issue.

#97 is what it would look like if \Drupal\experience_builder\Theme\XBThemeNegotiator failed to see and/or process the 'use_admin_theme' query arg in the AJAX request, and loads with xb_stark instead. Any time I've run into this it is due to the cache being out of sync with the current branch and a cache clear took care of it all but a few times - and that was fixed by truncating the cache tables. I've never run into the issue on a new install.

#98 This functionality targets the media library widget within the props form, which was all that existed when these efforts began. It can certainly be expanded to cover other uses in a separate issue.

bnjmnm → changed the visibility of the branch 3471978-try-using-admin-theme-without-fence to hidden.

The problem I spotted in #92 does not happen on a fresh install. I'm guessing it's legacy junk but if it happens to occur again I can probably knock it out more effectively in a separate issue anyway.

😿😿😿

Recording the outcome for a nice gif and I'm running into Claro base assets being added to the whole page instead of just the dialog. This means the "add button" style will look like xb_stark at first, then the dialog is opened and then it looks like Claro. I'm guessing this is due to the recent change of making the admin theme base styles a dependency of the Media Library instead of adding it as part of the dialog open request.

bnjmnm → created an issue.

Given that I've had to reopen several issues incorrectly closed by @niranjan_panem and

I tested the issue, views-pages and exposed forms are working fine in drupal, but not able to reproduce the issue.

Doesn't provide enough detail to indicate the steps to reproduce were followed, this may not have received the due diligence required to justify setting this to Closed (cannot reproduce)

I was just able to reproduce it in 11.x using the steps described in #12

@niranjan_panem I have reopened multiple legitimate issues today that you had closed due to incorrectly identifying them as no longer occurring. This hurts the project. If you'd like some mentorship regarding how to better do this, please join Drupal Slack (I could not find you there otherwise I would have pinged this to you there), and you can get some mentoring on this.

dfaf6b18 takes care of the alt validation message. The form generated by setEntityFields was not taking access into account so it was validating fields that should be ignored as their access is false.

#36 Given the age of the issue, this may very well be outdated, but the rationale given is not valid. Drupal core has never had a "button field" so there not being one would not be evidence of this not being an issue. This is about how buttons are styled. Buttons have been and continue to be part of Drupal, but they aren't a field type.

#4 confirmed the presence of file-audio.html.twig & file-video.html.twig, but the issue is reporting on field-audio.html.twig & field-video.html.twig (note they begin with field, not file).

However, these files don't necessarily even need to be present, The specific concern is that Audio and Video field formatter classes are not called. This can be manually tested by going to the formatter settings for a video field and confirm changes to height/width/muted are reflected in the rendered page.

Re #3 @niranjan_panem in order to reproduce the issue as reported, the contextual module must be disabled.

Since you did not mention disabling the contextual module in your comment, anyone looking at this issue should assume the issue as reported is still valid unless #3 is clarified further.

#82 and #83 were specific to my dev environment.

MR feedback is addressed. Please note there that there a new intermittent fail in the media library E2E test on 0.x. If a reviewer is seeing a Cypress fail that matches what I reported in 🐛 Intermittent fail in media-library.cy.js Active , it is unrelated to the changes in this issue.

bnjmnm → created an issue.

Sometimes I'll get an error from the dummy props form, but it isn't consistent. Here's an example of what I might run into after clearing a cache and accessing a path with a component selected... It expects a JSON response and instead gets the contents of core/modules/ckeditor5/css/table.css. I'm pretty confused by this ATM, but I'm sure there's an entertaining answer to all this 🤔

After merging in the current 0.x, it's not possible to click any component (no image/media library needed) and get the props form . It just opens the panel but no form appears.

This works fine on 0x, so clearly there is something in this MR causing the problem. It's kind of difficult to isolate changes but I'll dig around and the DEBUG-3471978-try-using-admin-theme-without-fence branch is with the current 0.x - I didn't update the main MR yet with it.

A few small things to address in the MR. Should be able to RTBC when those are done.

wim leers → credited bnjmnm → .

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

Excellent feedback from @longwave has been addressed.

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

re #20

I'd like to make it @larowlan's top priority in the next 2 weeks to keep pushing forward on #3467954: META: Evolve XB UI's data model to allow non-SDC components' inputs, DynamicPropSource support, etc. and all its child issues/blockers to solve this at the root.

That issue seems like the right priority, When that work is done, would it still be necessary for every prop-example image to become a media entity? It's fine with our dev-collection of components, but as more custom components start happening I could see that causing issues, such as someone deleting that placeholder image would make the component un-addable. It might be helpful to have a placeholder image generating controller, similar to placehold.it, where the URL determines width, height, color, content, then the required src for preview won't be at risk of deletion and repos don't need to have as many image files just to placehold stuff.

Again, the data model work seems like the priority to me, but if the above seems worthwhile this issue could be the home for it as it would make the band-aid unnecessary.

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

bnjmnm → created an issue.

I got the go-ahead to use @scope by @lauriii, but I now realize that was done outside of this issue so that's bad messaging on my part. I also had @scope support enabled in FF long enough that I'd forgotten it's not yet available by default.

I could go with not scoping at all or, it's reasonably easy to fall back to regular selectors when @scope isn't selected and in most cases it should work fine but there might be a few specificity hiccups that will go away as soon as FF switches scope from experimental to default.

It can be done with a ref!

Updated MR on the way...

Screenshot from 10.4, does not seem to be a problem for me either

Perhaps the original reporter can specify what element / which style is being set to a higher Z-index that results in the existing .media-library-item__remove Z-index setting not working.

Added 🐛 Media Library item styles assume contextual module is present Active for the issue spotted in #72.1 where Claro expects contextual.module for styling media library items.

bnjmnm → created an issue.

Yep its good

#73.1

There's still something off about the grid items because the label is appearing on top of the border

Outside of XB, the label positioning relies on this style from contextual.module.css

.contextual-region {
    position: relative;
}

contextual/drupal.contextual-links isn't anywhere in the media library dependency chain, so that stylesheet isn't guaranteed to load, just very likely to be as pages it is used on tend to have that library present anyway. Seems like there should be a core issue since the appearance depends on an asset that is not explicitly summoned.

#73.2+.3

This throbber should be using the full screen throbber styles:

looks like the drupal.ajax library also needed to account for the admin theme's overrides and extensions. A side benefit of this is we no longer need the workaround that was implemented to avoid all the extra Olivero assets from 🐛 Many css files incorrectly being loaded on /xb/ pages Active

#73.4

After adding media and I click "Remove", it opens Media Library with broken styles:

The remove buttons needed similar ajax property customization to what was provided to the dialog open button.

It is working fine for me with what the install hook provides.

To narrow down what might be different it's probably worth checking admin/content/media to see if the media entity created via the install hook actually got created, and if so, is it accessing the image properly

There are two steps in the process that can be checked.

First Step

First, the image in experience_builder/images/600x400.png is added as a managed file and copied to the root of public://

  $file = \Drupal::service(FileRepositoryInterface::class)->writeData(\file_get_contents(__DIR__ . '/images/600x400.png') ?: '', 'public://600x400.png');

Questions:
Is the file being copied?
Where is it being copied to?

Second Step

A media entity is created referencing the managed file created in the prior step.

if (MediaType::load('image')) {
    Media::create([
      'bundle' => 'image',
      'name' => 'Default placeholder image',
      'field_media_image' => [
        [
          'target_id' => $file->id(),
          'alt' => 'Placeholder image',
          'title' => 'Placeholder image',
        ],
      ],
    ])->save();
  }

The "Default placeholder image" entity pictured below should exist and the preview should actually provide an image .

Questions:
Does this media entity exist?
Is it previewing a valid image/path?

If everything checks out above, please share path of public::// on the site that is failing on this.
The sdc has the preview image path configured as - src: /sites/default/files/600x400.png which seems like it's making some assumptions regarding the stream path so I'd like to rule that out if there's nothing earlier in the chain failing.

Re #69

This is not a problem on a fresh install. The recently changed hook_install needs to run to create the placeholder image and associated media entity, then it works fine.

This works fine on a fresh install hook_install needs to run to create the placeholder image and associated media entity.

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

I'm not sure how to test this now that the image component is not working

The image needed is located at experience_builder/images/600x400.png but the image component, as of a few days ago, has the image set to src: /sites/default/files/600x400.png

Obviously that should be fixed, but copying experience_builder/images/600x400.png to src: /sites/default/files/600x400.png should make this reviewable

I feel like I had commented here before, but perhaps not.
Is there merit in exploring the use of declarative shadowdom here?
I believe @jessebaker tried it for components instead of iframes but it was problematic having to replicate the style tags inside the template elements multiple times for each instance of a component.

But because we've got a fixed set of CSS here, it might be more feasible. I'd be happy to try and spike on that if folks (particularly @jessebaker who has already done some exploratory work on it) felt it was worth exploring.

Pretty much everyone front-end focused including @jessebaker is hoping for a solution where the dialog and its content remain in the primary dom, especially if any Drupal vanilla JS is expected to work with it. Also worth mentioning that the CSS scoping/isolation of the dialog contents of this issue wasn't that challenging. Most of the complexity was related to the dialog itself, not the contents.

I did a bit of exploration with the declarative shadowdom just to be sure I wasn't prematurely ruling it out. It required overriding the insert AJAX command to use setHTMLUnsafe to allow <template> tags and I ran into issues similar to the ones I encountered in another project where we tried to Shadow-or-iframe a Drupal dialog where we had to add all sorts of additional communication support to ensure the dialog trigger, the dialog contents, and the dialog itself worked properly despite those three parts not being in the same DOM - the end result was something that still had problems and was even more spaghetti-ish than the MR in this issue. I don't think jQuery UI was built for long distance relationships, especially with the level of customization the Drupal dialog API has.

That said, if there's an MR that nails this I'm not too proud to let that one be the chosen approach.

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

Just pushed a solution to #66👆 - creating the admin CSS defaults with a custom library instead of bringing in dependencies on the dialog open request.

#61.1 and #61.2 were addressed by adding logic to recognize top-level selectors like <body> and .js and adding the wrapping scope after that

#61.3 was addressed by better matching the load order of Claro. These aren't CSS files with weight explicitly set, but if they appear in a different order than listed in the library definition, some unwanted styles will get precedence.

#61.4 and #61.5 were addressed by ensuring the Claro overrides and extensions were not just applied to the Dialog library, but the entire dependency tree the dialog library

I'm going to set to needs review because theres a bunch that needs to be looked at, but there's one outstanding bit I'm still looking into: On a fresh install, the dialog contents will not be rendered by the admin theme like they are supposed to. Just a cache clear will not work - but it seems like if I visit the XB UI then clear the cache, the admin-theme-rendering works from that point forward.

Left comments on the MR bits relevant to me

Fortunately I was able to reproduce the flaky test locally by slowing things down a bit (throttling or enabling Xdebug worked). The 3500128-retry-typing-until-it-works branch has the solution, and I've re-run it several times. It being intermittent makes it hard to be 100% certain, but this fixing it on a local where it happened reliably

component-operations had two different random fail areas, one of which was recently addressed in 📌 Implement adding component to empty layout without dnd Active . The other, which happens to be the screenshot in the issue summary, has something pretty specific going on.

It fails when looking for a section called "The Entire Node 1". In the issue summary, that section is instead namede "Node 1"

In a separate instance of this failing, the section is named "e Node 1"

This is the code where it happens - the input is receptive enough to receive the clear() as there are no artifacts of the default section name "Two Column section", but it looks like the fails occur when some of the initial typing used to name the section fails to register.

   cy.findByLabelText('Section name').clear();
    cy.findByLabelText('Section name').type('The Entire Node 1');
    cy.findByText('Add to Library').click({ scrollBehavior: false });

It is a pretty specific way to fail so hopefully that means a fix is reasonably easy to achieve too.

Ah rats I should have been more thorough after bringing in 0.x. #56 is what it looks like if the theme negotiator fails to instruct the dialog to render with the admin theme and just uses xb_stark because it was triggered from an xb_stark inclined route.

I consolidated the negotiators into one and this seems to be OK now

This seems to be reliably passing with the changes I made in 📌 Implement adding component to empty layout without dnd Active . It's difficult to be 100% sure since it is intermittent but I re-ran the tests many times and it's green every time

This seems to be reliably passing with the changes I made in 📌 Implement adding component to empty layout without dnd Active . It's difficult to be 100% sure since it is intermittent but I re-ran the tests many times and it's green every time

I expanded the scope to more easily accommodate Gin by effectively running preprocess_html but it looks like some work on Gin's end is probably still needed here 🐛 Dialog styles are not loading correctly in Experience Builder Active , but this can be reviewed with Claro.

This brings back every skip added in 🐛 Temporarily skip intermittently failing e2e tests Active , and I've rerun the Cypress tests many times and they pass every time.

• components-operations, empty-canvas and publish-button now add a component to the empty layout by clicking the component instead of dragging it. (turns out the UI already does this! Just no affordance to make it evident)
• global-regions was unskipped but no changes as the drag and drop operation is never in the canvas area. Seems to be passing
• The skip added in components-slots earlier today was removed, but only because a skip is already present on the higher level describe() that was added last month. The scope of this issue is only to remove skips added in #3500549