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

Retitling to focus on the goal. An iframe might or might not be the best solution for it.

I've added the MR 3471978-try-using-admin-theme-without-fence which does not iframe or otherwise fence anything. It renders the contents of the dialog (but not the dialog itself) with the admin theme. It also provides a fairly straightforward way of configuring other dialogs to be rendered by the admin theme. I'm hesitant to go with an iframe solution unless there's evidence of specific problems that can only be addressed this way, especially given that (as mentioned in #17) Site Studio had to abandon this approach due to problems they were unable to solve despite quality talent and time being thrown at it.

Anyway, Here's what it looks like with Gin (with the fix from this Gin issue: 🐛 Dialog styles are not loading correctly in Experience Builder Active )

Note this will only load the libraries that are specifically requested by the dialog contents, so it might not necessarily load base styles of the admin theme unless they are explicitly set as dependencies of the libraries requested by the dialog contents. I'm not sure this is a bad thing - with the themes I tried it seems like the UI-essential styling is reliably provided by this approach while also giving XB some agency in the overall visual experience.

So I think the main areas of potential concern with the MR might be

• Are there concerns about the media library css contaminating other aspects of the XB UI? Risk seems minimal since the previews are already iframed and Media Library styles are typically "namespaced" via distinct classnames
• Base styles aren't necessarily loaded, as it only adds libraries explicitly added in the Media Library render array. How bad do we need this? Without the base styles, contamination concerns are significantly reduced. This also gives XB more opportunity to style this while keeping the ML-essential styling present
• Were any of the concerns that motivated this issue geared towards styling the dialog itself, as opposed to its contents? This is a different challenge...

I don't think #38 looks quite like the Media Library is supposed to look like in Gin. The goal is to provide a consistent experience for the media library regardless of whether it's being used in XB or outside of XB. I'm not keen to a specific solution, but it's hard to see how we could load the base styles in XB without them leaking, at least without modifying the CSS either in JS or PHP. Do you have thoughts on how we can solve that without hard coding the styles in XB?

I don't think #38 looks quite like the Media Library is supposed to look like in Gin. The goal is to provide a consistent experience for the media library regardless of whether it's being used in XB or outside of XB. I'm not keen to a specific solution, but it's hard to see how we could load the base styles in XB without them leaking, at least without modifying the CSS either in JS or PHP. Do you have thoughts on how we can solve that without hard coding the styles in XB?

If it's important that ML visual consistency inside/outside of XB is the goal here, as opposed to just ensuring it's usable, then additional steps certainly need to be taken.

Keeping in mind that Site Studio stopped iframing due to issues that could not be fixed in a reasonable timeframe 📌 Media Library dialog styling Active , I think we can provide this consistent-visual + low-leaking if we only guarantee it with Gin (and perhaps Claro). The experience would still likely be acceptable with other admin themes, but the full visual consistency would only be available with these most commonly used admin themes. Most admin themes outside of those two already have their share of weird styling as it's very difficult to account for every theme edge case.

This could be achieved either by dynamically prefxing the admin theme CSS, or maybe just updating Gin directly.

Maybe that's controversial because it's not quite as holistic as a typical Drupal implementation, but it seems preferable to an option that broke some parts of the Media Library functionality.

Keeping in mind that Site Studio stopped iframing due to issues that could not be fixed in a reasonable timeframe

And what are those issues? Are they present in the MR I wrote?

The PoC I wrote seemed to work fine. There are problems with the integration e.g removing media but that is not related to the iframe solution at all.

What we want the solution to provide at minimum:

1. Consistent experience for using Media Library between XB and the rest of the admin UI (at minimum with Claro and Gin)
2. No duplication of styles so that Claro and Gin can keep updating their designs without our involvement
3. No explicit styling for XB in Claro and Gin

I think we should choose the solution that provides us the easiest way to accomplish this. If we don't have a solution that allows us to achieve all of these, these are in prioritized order so we should sacrifice from the end of the list first.

Re #41

The PoC I wrote seemed to work fine. There are problems with the integration e.g removing media but that is not related to the iframe solution at all.

That's different that what I encountered and perhaps mistakenly assumed the MR was incomplete.
I just checked again and rebased with the current 0.x and I'm running into the same thing

• The expected dialog behavior of moving submit buttons from the dialog contents to the dialog button pane does not occur. (Which is also evident in the screenshot provided in #22)
• The "insert selected" button does not work. When clicked, an AJAX spinner shows up momentarily but nothing beyond that occurs

For me as an outsider it’s hard to be part of the discussion since issues that are not in this issue is being raised unfortunately.

I believe anything I referenced was specific to the information provided in #17 which was linked to. To the best of my knowledge that's the extent of what I referenced, but if there's things beyond that I failed to properly contextualize it was not intentional and happy to fill in any blanks you were subjected to.

I've tested the MR before around when I commented #33 and uploading images and inserting images to the page was working back then. I can confirm that in the latest state it isn't working though.

I don't know if this is helpful but here's a mention of the Site Studio change in their release notes: https://github.com/acquia/cohesion/blob/8.0.x/RELEASE_NOTES.md#media-lib....

Here are the key files that are relevant from there before they reverted to a different workaround:

1. https://github.com/acquia/cohesion/blob/7.3.x/js/media-iframe.js
2. https://github.com/acquia/cohesion/blob/7.3.x/src/Render/MainContent/Ifr...
3. https://github.com/acquia/cohesion/blob/7.3.x/src/Ajax/OpenIframeCommand...

I spent few minutes looking at the iframe MR earlier today just to see why it wasn't working since it was working earlier. Seems like the problem was with the vite integration module which is essentially writing over the library that is adding the JavaScript to integrate with the modal. With that change, the submit button inside the iframe is triggering outside of it.

We'd still have to figure out how to solve the problem of the buttons not being loaded in the jQuery UI modals bottom panel. Wouldn't this require moving those buttons outside of the iframe which would then mean custom styling or loading admin theme styling again? We'd also have to change how the events work I guess because now the event would be triggered outside of the iframe.

Not saying we should proceed with this approach, but just wanted to see how the MR worked to better understand what is happening.

With my most recent approach, Claro is looking pretty good with the non-iframe approach, and the styles are scoped so no leak. The scoping-selector can be specified, too.

There's a library override currently in place to get it working that arguably shouldn't be needed, I'll need to investigate that further.

It also has a few problems with Gin, but those seem to be on the theme side and will likely be addressed in this issue 🐛 Dialog styles are not loading correctly in Experience Builder Active

re #45

We'd still have to figure out how to solve the problem of the buttons not being loaded in the jQuery UI modals bottom panel. Wouldn't this require moving those buttons outside of the iframe which would then mean custom styling or loading admin theme styling again? We'd also have to change how the events work I guess because now the event would be triggered outside of the iframe.

This is among the reasons I'm hesitant about the Iframe. I've had to deal with this buttonpane communication in other issues and it's pretty delicate. In this case we'd need to copy the submit buttons from inside the iframe, set them to hidden, then add the copies to the out-of-iframe buttonpane, set up handlers that communicate clicks to the hidden buttons inside the iframe, then the end result of the in-iframe clicks then need to be translated back out to the regular DOM again. It's a bunch of new potential points of failure in an already complex UI and if possible I want to avoid that 🤓.

It looks like what's inside the media library starts to look pretty close but the modal dialog itself looks like the default styling from jQuery UI. Is #46 still missing the modal dialog styles from Claro?

Is #46 still missing the modal dialog styles from Claro?

They are not included, the approach thus far has targeted Media Library styling (i.e. the dialog contents) but not the dialog itself. Since it sounds like the dialog itself should be styled, it's probably worth clarifying in the issue summary whether this dialog styling should apply to all core dialogs triggered from the XB UI, or specifically Media Library.

This is also the case for the iframe approach in the other MR - the iframe is inside the dialog so any assets added to the iframe will not impact the styling of the dialog it is enclosed in.

Styling the dialog itself introduces a bit of additional complexity because it's common for dialog styles to use selectors at the very top of the dialog markup (such as using .ui-dialog) - and these are the same selectors we use to wrap the dialog contents. So this introduces a scenario where we can't simply nest, but in some cases have to &. (and at the risk of excessive reiteration, iframing would not solve this because the iframe is inside the dialog).

Two paths come to mind to get the dialog styles consistent with the admin theme

• Find a way to leak-proof the dialog styles. I think this will boost the complexity quite a bit but I might be missing something simple. I'm sure we can figure it out, though.
• Just load the admin theme's dialog styling in the primary document. There's a risk of leaks if the dialog stylesheets are also styling non-dialog things, but all the core/Gin styles I looked are already scoped to dialog-only.

I went ahead with the option "Just load the admin theme's dialog styling in the primary document. There's a risk of leaks if the dialog stylesheets are also styling non-dialog things, but all the core/Gin styles I looked are already scoped to dialog-only. "

The end result is awfully close but not yet identical to using it outside of XB.

The spots that look off are ones that can be impacted by the height calculation that takes place when the dialog opens. I'll investigate further.

And with the most recent push...

That looks really nice 🤩

Does that also work with Gin after 🐛 Dialog styles are not loading correctly in Experience Builder Active has been fixed? 😊

Does that also work with Gin after #3497793: Dialog styles are not loading correctly in Experience Builder has been fixed?

I tried it out with Gin and any problems encountered were due to the the Gin accent library implementation that is being discussed as part of that issue. See comments 🐛 Dialog styles are not loading correctly in Experience Builder Active , 🐛 Dialog styles are not loading correctly in Experience Builder Active .

It is adding CSS variables that are scoped to selectors provided by preprocess_html - a hook that doesn't run when a media library dialog is created. I suspect they can make adjustments in Gin so it works, but XB could also make it easier if we were willing to run the main controller through the admin theme's preprocess_html()... something best scoped to another issue if we were to do it.

Very late to this party, but just adding a +1 to @bnjmnm's comment in #46 about being hesitant to use an iFrame. I think iFrame really should only be the approach of last resort. Looking at the progress since that comment, I'd say @bnjmnm is on to a winner as that's looking great and avoids the fragility of iFrame back and forth.

Seeing the progress so far I'm optimistic the CSS scoping will work as a robust enough solution here. Nice 👏

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.

For some reason Media Library looks like this with the latest changes in the MR:

I wonder if I'm doing something wrong? 🤔

For some reason, I'm also unable to type to the alt field after uploading an image. The field shows up and I'm able to focus it but if I type, nothing shows up on the form.

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

Up until now, the XB team has been following a pseudo-scrum/pseudo-kanban process, but we're now shifting into more conventional scrum. We started a new 2-week sprint last Thursday (Jan 16). I'm tagging our current sprint's issues for visibility.

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.

What's in the MR isn't still fully working:

1. Font family is incorrect; for some reason Media Library is loaded with font-family: Arial, Helvetica, sans-serif; in XB whereas outside it is loaded with font-family: "system-ui", -apple-system, "Segoe UI", Roboto, Oxygen-Sans, Ubuntu, Cantarell, "Helvetica Neue", sans-serif;
2. The media library items are not rendered entirely correctly.

   In XB:
   

   Outside of XB:
   

3. Styles for focusing the select element is broken:
4. Styles for the insert for media library button are broken for the non-default state:
   
5. On the table mode, links are black instead of blue: