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:
   

I chatted with @bnjmnm about this. PropSourceEndpointTest failed for locally that same way it does on gitlab. It passes for me locally on 0.x.

I tried various things and the only thing that worked was reverting that changes to experience_builder.redux_integrated_field_widgets.inc

I restored and put some debug asserts locally to see if I could figure out what was happening

This is the assert after the 2nd request for xb/api/config/component

// Ensure we get the exact same response contents as the first time.
    $secondResponseDecoded = $debugGetDecodeResponse();
    $this->assertSame(array_keys($firstResponseDecoded), array_keys($secondResponseDecoded));
    $this->assertSame($firstResponseDecoded, $secondResponseDecoded);

The last assert fails. Since the first passes we know the same the components are returned in the list
It is just that differ

It turns out components for blocks like block.user_login_block have different default_markup
so

- <input data-drupal-selector="form-qre8a3p5fsuun59wo6jradkbpeceewp-2ez9swbej9m" type="hidden" name="form_build_id" value="form-qrE8A3p5FSuUn59WO6jrADkbPEcEEWp_2EZ9sWbEj9M" />\n
 +<input data-drupal-selector="form-69vyw7nxd-eczqvemkgxhhdjy43obmtmxbgbicdmtdc" type="hidden" name="form_build_id" value="form-69vYw7nxd-ECzqveMkGXHHdJY43obMTMXbGbiCdMtdc" />\n

so the identifier for the form. I guess since cache was not hit the form was regenerated. I guess maybe that is not very helpful 🤷

🎉Ok figured out which line makes PropSourceEndpointTest, experience_builder_library_info_build()

pushed up a commit with this. But will paste it here:

// 💡 Returning here will make PropSourceEndpointTest pass. If \Drupal\experience_builder\Asset\XBFriendlyLibraryDiscoveryParser::setActiveTheme
  // is called in the next line,and we still return [], the test will fail.
  // I guess this results in a call to \Drupal\Core\Theme\ThemeInitialization::loadActiveTheme().
  // Could it be that calling loadActiveTheme during the hook makes the uncachable?
  //return [];
  $library_parser->setActiveTheme($active_admin_theme);
  // If we comment out the return above and make it here PropSourceEndpointTest will fail.
  return [];

I am not familiar with \Drupal\Core\Theme\ThemeInitialization::loadActiveTheme(). but assume it usually done 1x a request. Maybe calling in the hook again with a different theme makes the pages uncachable?

With my latest commit PropSourceEndpointTest should pass. Other test for the new stuff in this MR will probably break but a least we know $library_parser->setActiveTheme($active_admin_theme); is likely messing with the caching

#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.

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.

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.

I'm not sure how to test this now that the image component is not working: 🐛 Adding the Image component results in a state considered invalid Active .

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

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.

I tried re-installing before submitting 🐛 Adding the Image component results in a state considered invalid Active . Re-installed again and still seeing the same error message.

1. There's still something off about the grid items because the label is appearing on top of the border:
2. This throbber should be using the full screen throbber styles
3. This throbber should be using the full screen throbber styles:
4. After adding media and I click "Remove", it opens Media Library with broken styles:

#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.

Thank you for tracking these down so quickly @bnjmnm 👏 I think from my perspective the experience is looking really good and ready to ship 🚀

Holding from moving to RTBC since phpcs is failing.

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.

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.

Added some questions/nits. Also trying to think of a cleaner way of altering the libraries than experience_builder_dialog_library_customize() as swapping the active theme during discovery feels kinda hacky, but not coming up with anything yet.

Excellent feedback from @longwave has been addressed.

Added another round of questions/nits but overall this is looking great!

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.

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 🤔

#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.

Some more nits/questions but this looks great and is nearly ready!

This looks ready to go!

Taking a final pass over this epic work, to make sure I understand it too! 😄

Epic work here! 🤯🤩

Fixed nits, moved 2 excellent @bnjmnm comments on the MR into the MR, did trivial refactoring in ExperienceBuilderController to simplify both the code and the diff and figured out the answer to @bnjmnm's question at https://git.drupalcode.org/project/experience_builder/-/merge_requests/5... (root cause: tiny regression introduced by 📌 Pages have images usable for SEO purposes Active ).

What's left:

• praise 😄
• a request to generalize the bits in experience_builder_field_widget_single_element_media_library_widget_form_alter(), although AFAICT that is not possible. Just confirmation is fine: https://git.drupalcode.org/project/experience_builder/-/merge_requests/5...
• 🙏 Renaming experience_builder/ajax.custom asset library name + behavior name to something more descriptive: https://git.drupalcode.org/project/experience_builder/-/merge_requests/5... + https://git.drupalcode.org/project/experience_builder/-/merge_requests/5...
• 🙏 Clarifying the .ui-dialog fallback: https://git.drupalcode.org/project/experience_builder/-/merge_requests/5...
• A GIF added to the "UI changes" issue summary, showing how this now beautifully uses the Claro styles! 😄

I hope the ~15 mins that'll take will make you feel really good about the work you've done here, @bnjmnm, because … very few people could've done what you've done here! 🧙‍♀️😵 It's complex, but it's necessary complexity 👍

wim leers → closed merge request !352

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

😿😿😿

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.

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.

I was just testing this new feature.
Not sure what is wrong.

Here what I see, and all links are unresponsive.

with those errors in console

This is also not working for me at least when launching from the "Page data" tab:

It also looks like some of the Claro CSS is leaking to the XB UI after opening the media library:

Not able to test with the image component because of 🐛 Adding the Image component results in a state considered invalid Active .

#96: Thanks for the GIF, @bnjmnm! 🤩

@heyyo in #97: how are you testing Experience Builder? Did you test using 0.x at daa7e31491c5f01c1518de544d7e4724f33e50ef and follow the full set of steps documented at the top of CONTRIBUTING.md? And clear the browser cache?

@lauriii in #98: Can you also reproduce that while editing an article node? I can reproduce the off-looking media library dialog that you demonstrated when editing a Page content entity. I bet there's some accidentally loaded asset library on node forms that does not exist on the Page content entity form. I prefer — and I suspect @bnjmnm will, too — to tackle the "Media Library Dialog does not look correct when editing Pages" problem in its own issue — when this issue started, Page did not even exist yet, and it definitely was not yet editable through the XB UI (that's since 📌 Adding or editing a Page brings user into Experience Builder not entity form Active ). Chances are there's something wrong on the 🌱 [META] Pages Active side, not here.

(Forgot to attach the screenshot in which I managed to reproduce @lauriii's observed problem, but only when editing a Page.)

Here's the same bug reproduced with an article.

Aha! You were testing using the media library widget for a field on the edited content entity type! (i.e. bundle/base field).

(You did say tab in #98)

So: can you reproduce this when opening the media library for an image component you just placed? 🙏 If so, we've narrowed it down to ComponentInputsForm vs Drupal\experience_builder\Controller\EntityFormController::form().

#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.

I tested both #101 and #98 with a fresh install.

Thanks, @bnjmnm, for creating 📌 Exapand Media Library admin theme rendering beyond props form. Active to tackle

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.

👍

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 🤓

@wim-leers sorry I missed the notification.

Yes I'm following contributing.md file, but I'm not using standard profile but minimal and I have existing config which installs for example Gin theme.
I tried to cleared the cache of browser, but it didn't helped.

On slack, Laurii pointed me to a Gin toolbar issue, and indeed switching to Claro, it worked as expected.
https://www.drupal.org/project/experience_builder/issues/3471978#comment... 📌 Media Library dialog styling Active

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

Automatically closed - issue fixed for 2 weeks with no activity.