πŸ‡ΊπŸ‡ΈUnited States @bnjmnm

Ann Arbor, MI
Account created on 3 November 2012, over 12 years ago
  • Staff Software Engineer at AcquiaΒ  …
#

Merge Requests

More

Recent comments

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

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

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

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

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

The template is where the issue lies

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Gin targeted refinements are happening here πŸ“Œ Dialog + Media Library refinements to support Gin Active

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

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

Experience Builder

Regular Gin

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

bnjmnm β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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 πŸ€“

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

😿😿😿

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.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI
πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

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

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

bnjmnm β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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 πŸ€”

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Excellent feedback from @longwave has been addressed.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

re #20

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

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

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

bnjmnm β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

It can be done with a ref!

Updated MR on the way...

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

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

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

First Step

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

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

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

Second Step

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

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

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

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

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Left comments on the MR bits relevant to me

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

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

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

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

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

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI
πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

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

bnjmnm β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

I have a branch in πŸ“Œ Media Library dialog styling Active (try-using-admin-theme-without-fence ) that works great with Claro and even runs hook_preprocess_html on the admin theme. Gin still doesn't look quite right and while I think the remaining tweaks need to be done on Gin's end. However, if it seems like an XB change might be the most reasonable, lmk and we'll figure it out.

Perhaps someone could try the current MR in this issue with the XB branch I've created and we can discuss what other changes might help and in which project.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

I believe this is similar to my comment in πŸ› Random failure in empty-canvas.cy.js Active where there are waits being added for endpoints to address the issue, but the problem occurs slightly earlier so those endpoints are not being requested at all.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

The screenshot indicates the preview endpoint isn't being requested at all, so adding a wait on it would not improve the chances of success.

The problem is likely due to the preceding drag and drop not completing successfully. There are a few areas I recommend looking at when this is the point of failure:

Possibility one - it is not sufficiently waiting on something that guarantees the target area is droppable
This test had been flaky in the past, and was addressed by adding cy.waitForElementInIframe('.xb--sortable-slot-empty-placeholder'); as the presence of that class indicates the iframe is equipped to receive drops. This part of the UI has changed a great deal recently and it's possible this isn't a reliable indicator anymore.

Possibility two - it needs an explicitly set position option for the realDnd call
This one seems less likely to be the case since the target area is fairly large but it has fixed flaky realDnd calls in the past

And it might be something entirely different but the above are things that addressed similar issues in the past.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

And with the most recent push...

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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.
πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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 πŸ€“.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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...
πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

With the suggested change in #16 this is what it looks like

It looks like this with my MR from this XB issue πŸ“Œ Media Library dialog styling Active

I'm also aware the suggested solution is pretty similar to what is in the MR. The differentiator may be due to how library alters are cached, which we discovered in πŸ› Many css files incorrectly being loaded on /xb/ pages Active make the (sorta reasonable) assumption that a single page isn't summoning multiple themes at once.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

With the disclaimer that I looked into this on a standard Drupal install with Gin/XB and not Drupal CMS I was able to reproduce the issue reported.

Then, I was able to address the majority of the problems with a single line adding variables.css to the dialog library in gin.libraries.yml

dialog:
  css:
    theme:
      dist/css/components/dialog.css: { minified: false, weight: 99 }
      dist/css/theme/variables.css: { minified: false } # adding this line fixes everything

I also have a branch in πŸ“Œ Media Library dialog styling Active named 3471978-try-using-admin-theme-without-fence that once it (or something comparable) lands it will look even better, but I think the above puts out the major fires that led to this issue being created.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

The fix in πŸ› Cannot upload new image in media library Active should fix this, but even if it doesn't it should be committed before any work proceeds on this as it addresses an underlying issue that causes all sorts of flakiness.

That issue has a fix and tests already. The final thing needed is to set up Gitlab CI so a built version of the UI app is available to FunctionalJavascript tests.

Production build 0.71.5 2024