Media Library dialog styling

I personally think #3 is the better choice, because:

• there may very well be other Drupal-defined dialogs that we will need/want to load inside XB, and those will need to look good too, then
• #2 likely needs some of the alterations applied that claro.theme is making
• #2 likely then needs postprocessing of the Claro CSS to ensure it is scoped to only target the Media Library dialog

I defer to @lauriii.

wim leers → credited lauriii → .

Closed 📌 [PP-1] Media library should use styles from Claro Postponed (opened by @lauriiii in parallel with this one!) as a duplicate, since it contains less detail.

The media library should look like Gin because that's what Drupal CMS is likely going with so we definitely should not style it like Claro at least. Ideally we could outsource the styling completely to the admin theme so that we don't have to own this within the XB. I don't understand why are we loading the media library within the XB application instead of loading it inside an iframe that would have all the required styles from the admin theme?

Ideally we could outsource the styling completely to the admin theme so that we don't have to own this within the XB.

I don't see how this is realistic/feasible, because those admin themes will understandably style dialogs on the back-end only, not in the front-end (system.theme:default) theme.

That's an unsolved problem in Drupal core too: #2195695: Admin UIs on the front-end are difficult to theme → .

I don't understand why are we loading the media library within the XB application instead of loading it inside an iframe that would have all the required styles from the admin theme?

No idea — that's the part of #3454173 that @bnjmnm handled, so assigning to him for answering that :) I suspect it's simply "because getting it to work at all was the only goal for #3454173" 🤞

It's unsolved for Drupal core but there are several solutions in contrib for this. For example Layout Builder iFrame Modal → solves a similar problem within Layout Builder.

Great! TIL about https://www.drupal.org/project/layout_builder_iframe_modal → 😄

Then the next steps are pretty clear — updated issue summary accordingly.

Assigning to @bnjmnm for review/confirmation of @lauriii's proposal.

I'm not sure how feasible iframing the media library widget is, but I also don't want to dismiss something entirely until I've had a chance to dig further.

Layout builder's architecture seems more welcoming to this kind of solution
In layout builder:

1. edit form opens in a dialog or frame
2. submit the edit form and the changes are saved server side
3. The entire #layout-builder is rebuilt based on the updated server side values saved in the prior step. The methods in \Drupal\layout_builder\Controller\LayoutRebuildTrait

The Media Library widget, otoh, does not work like this. It isn't updating anything in the DB that will then be available after a refresh. Instead, it is updating the DOM directly. Specifically, it is updating fields in the entity form with values that will not be represented in the back end until the entity is saved.

So to make this work I believe it would be necessary to create a translation layer that allows the iframed Media library widget to update the DOM of the main document. We know that is possible because the DnD layout does exactly that. Given the implementation hurdles of the initial Media Library implementation, I suspect implementing this would be more time consuming & complex than the frame <-> document communication used for preview.

I'm happy to find out I'm wrong about the anticipated complexity - but if it turns out I'm correct it may be worth evaluating if doing it the iframe way is important enough to devote 1-2 sprints to implementing.

I was thinking that we would only iframe the modal dialog which users use to browser, select, and upload media, not the widget. The widget itself has a different design in Experience Builder, but the modal should have the same design as the admin theme. Does that make this more feasible?

Does that make this more feasible?

Unfortunately no, I was not using the correct terminology. I will edit my comment to make it clear I'm referencing the Media Library modal, not the widget in the entity form

Needs a new round of feedback from @lauriii.

We've already said that option 1 won't work, and to me it seems that options 2 & 3 have the downside that they add quite a bit of long term maintenance burden. Whenever there are changes to the backend markup / CSS, we need to make sure that it works in the context of XB. FYI, I changed Claro to Gin since that's what Drupal CMS is most likely going with.

Because of that, unless we have other ideas, I think we should do a spike on the iframe approach to better understand how it could work.

So to make this work I believe it would be necessary to create a translation layer that allows the iframed Media library modal to update the DOM of the main document.

Do we need the iframed media library to modify the markup directly? Couldn't we do this in a more structured way where we tell the media library to call a function that returns what was selected in the media library? Then in the XB app we translate that into how the media library form element / widget should be updated.

Do we need the iframed media library to modify the markup directly? Couldn't we do this in a more structured way where we tell the media library to call a function that returns what was selected in the media library? Then in the XB app we translate that into how the media library form element / widget should be updated.

This is possible, but it should be taken into consideration that the dev resources necessary for this approach this are likely nontrivial. If there are time-consuming surprises similar to the initial ML implementation, this approach might take 1-2 sprints to complete and would have to be done by someone with a skillset >= staff engineer. There's a good chance doing this approach would noticeably slow down other efforts, but perhaps that tradeoff is warranted. While I can't really make that call I at least want to provide the info to help the call be made confidently.

I've done some digging and Site Studio originally used to load the Media Library in an iFrame but had to resort to loading it directly on the page with their own custom CSS because it didn't play nicely when in an iFrame. I think there were several issues but the last of many smaller issues was an bug where after using the filter controls while Media Library was in an iFrame it became impossible to select an image.

This is not to say loading it in an iFrame is not possible or not the correct solution in the long term, but more to add a caveat that loading it in an iFrame is certainly not a quick win - and certainly not something I suspect we can achieve in time for the demo unless someone outside of Acquia is able to pick this up and bring it to completion.

Based on the recommendations from the experts in this area, @bnjmnm and @jessebaker, I think the only viable recommendation here to achieve this by DrupalCon is:

• port relevant templates/preprocessing from Gin
• port relevant CSS/JS from Gin

The proposed workaround seems okay for the meanwhile. I did move this issue down on the list in 🌱 Milestone 0.1.0: Experience Builder Demo Active because of this. Polishing the workarounds for some of the other workarounds seems like a higher priority than implementing this workaround.

We're not doing #18 anymore, and should do @bnjmnm's original proposal at #10 here, to solve this fully.

See #3454094-77: Milestone 0.1.0: Experience Builder Demo → for why.

Spent some time looking into this tonight, and I think I have a solution on how this can be done. Here's how:

1. We need to form alter the MediaLibraryWidget, and override the ajax callback for the open_button

2. In said ajax callback, we need to build a new MediaLibraryState so that we can change the opener ID, which allows us to use our own Media opener. Here we will also construct the iframe URL, and then we'll return an AJAX command that needs to render said iframe.

3. We build a Media opener very similar to MediaLibraryFieldWidgetOpener, but we need to forward the commands from the iframe so that we can run them where the media field is actually rendered.

Leaving this unassigned, but I might pick it up again next weekend if someone else doesn't before me :)

I pushed some more changes and now it looks like this:

@johnwebdev Very interesting approach! 🤓😄

I think it'd be valuable for @bnjmnm to provide feedback on this, because this does seem like a much simpler alternative to his proposal in #10! So if he agrees this is viable, then that'd save us a lot of time! 🤞

re #24 I'm not seeing where the suggestions in #21 + #22 simplify anything that's been discussed so far. I see that goes into greater detail (and does so nicely) about how the approach mentioned in #10 would be implemented, but I'm not seeing any time savers here there's still the "but we need to forward the commands from the iframe so that we can run them where the media field is actually rendered." mentioned which was my largest concern regarding effort, and something that Site Studio was never able to get running reliably.

While here, I also wonder how much we need to prioritize "without affecting the main document". Maybe we can just add the admin theme's CSS to the main doc?

We're already pulling in any libraries #attached in a form - including Media Library libraries provided by the module. If we want to fence CSS entirely we'll need to solve it for more than just the media library widget. The media library widget selectors are pretty specifically named and are probably less risky than other core CSS that is already being added via #attached in props forms. The Preview is already wrapped in an iframe so the CSS-contaminateable area is smaller than something like Layout Builder, and largely UI related elements in our control.

One question and possible problem with "Maybe we can just add the admin theme's CSS to the main doc?"

The question: Does that mean we add the "global-styling" or ALL libraries defined for the Admin theme?

Possible problem: I don't think just adding the CSS is enough, I can see for e.g. Claro there is a lot of preprocessing of e.g claro_preprocess_input, claro_form_media_library_add_form_alter, claro_preprocess_item_list__media_library_add_form_media_list that will affect things like Media library, e.g. if we didn't run claro_form_media_library_add_form_alter we would never even add the Media library CSS specifically for Claro theme! (Or add the classes it needs)
And if we're using a different Admin theme then Claro we have no idea what kind of preprocessing it does.

The question: Does that mean we add the "global-styling" or ALL libraries defined for the Admin theme?

That shouldn't be necessary. There's a risk of it if the libraries we hope to add have it somewhere in there dependency tree, but it's not something fixed.

Possible problem: I don't think just adding the CSS is enough, I can see for e.g. Claro there is a lot of preprocessing of e.g claro_preprocess_input,

Oh! That's a really good point. That will need to be looked at before a just-CSS approach could be recommended.

Continuing on #22

1. Currently the dialog height is set to be 700px fixed. In Media Library UI its set to 75%. If the content of the dialog is less than configured height, it's changed to auto. I think that's a buggy behaviour since height is basically working as maxHeight, but makes it hard for use cases like this when we have iframes. I opened https://www.drupal.org/project/drupal/issues/3480282 🐛 The configured dialog height is overrided to be auto Active , but maybe someone have a better workaround in mind then just a fixed height.

2. Currently the modal is not closed when clicking Insert selected, we can solve that by just adding:

new CloseDialogCommand(),

to the $commands defined MediaLibraryXbPropOpener.php.

This is basically what MediaLibrarySelectForm and AddFormBase does:

    return $this->openerResolver->get($state)
      ->getSelectionResponse($state, $media_ids)
      ->addCommand(new CloseDialogCommand());

but we're only forwarding the commands from the opener and I think that's fine.

3. Instead of overriding media_library.ui, we can create a new "wrapper" route and do the overrides on them, this will allows us to only override in XB context.

So I basically suggest creating this route:

experience_builder.media_library.ui:
  path: '/xb/media-library'
  defaults:
    _controller: 'media_library.ui_builder:buildUi'
  requirements:
    _custom_access: 'media_library.ui_builder:checkAccess'
  options:
   _admin_route: 'TRUE'

... that simply uses the same controller. And then we just update experience_builder_preprocess_html as well to target that XB specific route.

4. experience_builder_preprocess_html()

Not sure yet, but either we remove all regions except 'content' and then remove any elements in said region except the Media library UI, or we create a new renderer that does above, which perhaps is a bit more efficient.

Hi,

I had to synchronise 0.x and this branch before working on this issue (they moved many files and HOOKs and they switched to xb_stark).

I commited the changes, and added an up-to-date screenshot to this comment

1. Currently the dialog height is set to be 700px fixed. In Media Library UI its set to 75%. If the content of the dialog is less than configured height, it's changed to auto. I think that's a buggy behaviour since height is basically working as maxHeight, but makes it hard for use cases like this when we have iframes. I opened https://www.drupal.org/project/drupal/issues/3480282 🐛 The configured dialog height is overrided to be auto Active , but maybe someone have a better workaround in mind then just a fixed height.

Media Library UI is by default set to 75%, the parent iFrame is not which then result in Dialog using the bar minimum height. Instead of playing with Media Library UI configuration, we should set the parent iFrame to a fixed height and width (I only fixed the height for the moment) to the best possible value.

3. Instead of overriding media_library.ui, we can create a new "wrapper" route and do the overrides on them, this will allows us to only override in XB context.

I created a route as suggested by John, this help reduce commited changes.

It would be good to PoC that we can get the insert media to work before we further optimize the solution. This is the concern raised by @bnjmnm in #25. It seems that #28 outlines some steps that we know need to happen before we can get the insertion to work. Are there other steps that we know need to happen?

#33 insertion already works with current code

We are still facing the unable to modify issue : once I quit the media settings, I can no longer view the active Image in the sidebar again and all I have is a field with "Add media" button