🇺🇸United States @luke.leber

Hey folks!

I've re-targeted this issue against 2.x-dev. Good news...there's now automated test support.

If someone were to provide a failing test that proves this is an issue it can likely proceed further than it has the past 4 years :-).

Thanks Kyle.

Merged into 2.x and cherry-picked back to 2.0.x. I'll be cutting 2.0.0 stable shortly and will make 2.0.x the recommended version, but will leave 8.x-1.x supported until the end of Drupal 10 support.

Highly doubtful that this module will see another commit until Drupal 12 though.

luke.leber → changed the visibility of the branch project-update-bot-only to hidden.

This is NR (for real) now for !4. Green pipelines and adjusted target branch.

@kyle | @brianne | @zach - any chance of a review for this? Thanks.

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

Thanks, I plan to open a pull request with an automated test and the appropriate gitlab CI configuration tomorrow and get someone from our internal team to review it.

In all honesty, I haven't had any sponsored time to spend on XB since Atlanta.

I would be curious to hear what Pierre thinks since his SDC advice of iterating slots (versus pursuing dynamic slots is what led me to discover the hacky workaround.

It does seem a very, very fragile state of affairs that iterating a slot's components and injecting some markup can cause XB's front-end to fail by using only well supported twig. I'm assuming that a simple preprocess could do the same.

I do think that this quirk will be fairly commonplace to find in the wild.

smustgrave → credited luke.leber → .

So to summarize the immediate problem to move this forward, when trying to determine if something is an image, there's a string input that's basically in a random format. It might be a scheme://path.extesion?query#fragment with any part present, not present, or even malformed. Anticipate that the string might even be something like a string of random emojis and some unprintable / null control characters, or even characters that aren't allowed in a URL at all that may or may not be encoded - Some of these methods seem to be general utility methods which may be called outside of Mime Mail given that the utility class is not marked @internal!.

This string has to be either:

1. Massaged into something that the mime type guesser won't choke on
2. Assumed to probably not be an image if things are too messed up to pass to the guesser

Even given Drupal's URL utilities, that's likely extremely tricky as this issue stands. IMO it might be worth checking the various call sites and seeing if there's an opportunity to normalize things. That would of course likely need a B/C break in the utility class, but I do fear that tackling the problem naively may be a very disturbing game of whack-a-mole in the wild. Passing a nullable \Drupal\Core\Url object into the various utility methods would make this a trivial problem to solve.

luke.leber → created an issue.

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

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

luke.leber → created an issue.

This change will need a post-update hook to re-save existing configuration.

There's another issue at play here as well. With the syslog module enabled, calls to drupal_flush_all_caches resolves the asset.js.collection_optimizer service, which resolves about 5 other services until it eventually gets to creating a Syslog object. This object causes a cache_config rebuild before drupal_flush_all_caches is ready to start rebuilding.

Specifically, the drupal_static and to a lesser extent the twig caches will be stale / corrupt when this rebuild happens. The state of Drupal is kind of in a weird half-flushed state where all of the bins are empty, but drupal_static isn't.

Crazy idea...but does it really matter if the tempstore entity is updated if the entity was saved with LB? Seems to me at face value that's a premature optimization. Can the route check just be removed...?

2) For now the expectation is that more complex use cases (like tabs or sliders) will have to be coded for by the component developer making use of that window.isInsideExperienceBuilder

I think this could work for the most part if there was sufficient render-time context to conditionally render additional JS. From the limited understanding I have of the semi-decoupled theme engine, that seems possible, so 🥳.

I apologize that I no longer have any officially sponsored time (9-5) to delegate to this effort, but it was a tenuous arrangement from the start. I'll endeavor to respond to issues I'm already involved in until closed though on my own time.

I would like to draw some attention back to the gif at the end of #7. While a naive "in_preview" works for simple accordion-like controls with Boolean states, things like tabs, which could have multiple mutually exclusive states remains a question mark.

To accomplish this in layout builder, our team currently injects additional JS on top of what normally runs on the front-end component to keep track of state.

Admittedly it is a bit clunky as there are massive layout shifts on re-paint though...so I wouldn't read too much into how we "solved" it with layout builder.

Yep! That should do it. I'm not 100% positive on the rationality behind it, but it's a very common pattern for when changes to a module (or core) require a cache flush.

If I had to guess it's so that site owners will see that database updates are required in their dashboards, and when they run said updates, the caches naturally flush out.

Hey,

The call to Html::getUniqueId should prevent duplicate IDs 🤔.

That's mainly why I'm interested in how this is actually happening. I wasn't able to reproduce this. The only thing I keep coming back to is if perhaps your theme or module is also using that ID without wrapping it in Html::getUniqueId?

If the uniquification is broken, we would have to fix it. Removing that ID would be a backwards compatibility breakage that can't happen until a new major is released.

It's my understanding that the mere presence of a post-update hook is sufficient to ensure that caches are flushed appropriately. I don't think we need to call the cache flushing function manually inside.

Almost there for 1.x🙂.

Regarding the 2.x branch and tests, it looks like Drupal 11 has indeed broken some functionality that will need to be addressed 😞.

Best to stick with the stable release until that bag of worms can be resolved.

I gave this a manual test (via the attached model) and everything still seems to be in order. Switching back to NR - thanks Jürgen!

luke.leber → created an issue.

I think the concept is useful, but sometimes the customization can't be hard-coded as data attributes, but rather rely on runtime information.

One example is for the international telephone element. See https://git.drupalcode.org/project/webform_intl_tel_national_mode/-/blob... for a somewhat heavy handed fix for how most UX problems were resolved.

Were there a client side event to tap into, it'd make things a lot easier to tweak. I would LOVE to take a holistic pass on the telephone input given how many quirks there are, and if there was more robust options handling, that'd be even easier.

Moving back to needs work due to...

1. The issue summary describing the problem but not clearing laying out a proposed resolution.
2. This issue has a visible patch (that doesn't apply to HEAD) -- patch workflow is antiquated and will not run automated tests anymore.
3. This issue having 2 forks (which are hidden by default) -- both of which go different directions than the patch.

All in all, I don't think this can be "needs review". There needs to be a written-down plan and one up to date merge request to be eligible for needing review IMHO.

That all said something definitely needs to be done here. The tap area for the current out of the box experience is too small to pass accessibility audits, so tagging with accessibility.

Back to NR for !11689 (10.5.x) - I'm out of time to open other PR's, we've decided to patch this in as needed.

luke.leber → changed the visibility of the branch 3471642-upgrade-asm89stack-cors-to to hidden.

Add backport plea.

I couldn't actually find any instructions on how to upgrade composer dependencies in core issues, so I just took a wild guess. I'm guessing the automation will bark if it's way off-base.

Cleaned up the IS.

Update title + IS.

Thanks Adam.

Great news -- I'm going to close this one out as outdated. While this still results in a somewhat broken design system on my end, this is now able to be very easily resolved by adding a defensive display property that won't be clobbered by the Experience Builder scaffolding styles!

Thanks to everyone involved upstream.

Woo! Odds are the upstream issue is now fixed. Self-assigning for a re-test (tonight or tomorrow on the plane).

I still have a "broken" test case readily at hand.

Clean up I.S.

Added some example model configuration files to aid in review.

I'll mark as NR for the concept check anyhow.

I've taken a slightly different stab at this in the issue fork. Rather than providing various flags for the \sort function, the action operates at a slightly higher level by allowing users to pick different sorting functions.

Jurgen also mentioned in a slack thread...

I'd like to add that there should also be sorting based on either keys, values, or callbacks. The latter would allow sorting based on properties or other logic when values are complex data types.

This might be a crazy idea, but I think that this could be feasible by linking to another ECA action that acts to compare two values with arbitrary steps!

I know that the actual sorting implementation isn't correct -- but it at least allows the kernel tests to pass.

I'll open a draft PR to get some feedback on this feature request.

Thanks!

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

loadCSS is more or less abandonware nowadays. Regarding CSP's there are always some complications there, but nothing insurmountable. CSP has a great API that can probably handle anything we throw at it.

https://git.drupalcode.org/project/inline_all_css/-/blob/1.0.x/src/Event... is how I added CSP support to inline_all_css. Granted core wouldn't implement a subscriber, obviously. I guess CSP would have to come up with some solution to deploy in tandem with this feature.

I can reproduce this! I've opened an upstream issue with diff 💬 A crossroads: how to resolve warnings when a module adds new diff plugins? Active to see what the best way to resolve this is.

Thanks for reporting!

luke.leber → created an issue.

I think the proposal in #19 is a rational start, but am slightly concerned that not all business rules can be represented in YML fashion.

I'm still of the mind that power users will require some degree of programmatic interface to check all the boxes that are presently fulfilled by things like LB Restrictions.

Whether these traits exist on the SDC or in a parallel XB-only configuration is the question.

Additionally I'm thinking that adding all this at the slot level in SDC will make it very hard share SDCs across themes/modules.

This is a great point. I do rather struggle to see how SDC's would be able to be mixed and matched across different themes in ways that wouldn't end up "looking weird" from a branding perspective though. My mind immediately went to an example like "I'd like a header from Princeton's design system, a footer from Harvard's, and accordions from Yale's -- which would result in a very odd looking thing from a branding perspective.

Are there any concrete use cases for wanting to mix and match SDC's across different themes? Typically I'd think that there wouldn't be the desire for design systems to be arbitrarily mixed and matched in that fashion.

Ugh related issue != Child issue

Dropping this in from slack to hopefully get a follow-up at some point (in the project management court now):

With ~100ish components on a page, here are some browser performance metrics from a high end laptop:

~45 second time from page load to rendering the XB UI with dev tooling enabled; ~20 seconds w/o.

INP with dev tools ~3-4 seconds. 1.2 seconds w/o

--

I know that XB is still very early in development, but wanted to raise some awareness about potential scalability problems that will naturally be MUCH harder to tackle later as an after-thought -- even moreso once disruptive changes are harder to justify!

griffynh → credited luke.leber → .

Oo, shiny.

Yeah, this particular issue seems to go away with that MR applied.

I don't see much utility in adding render-blocking CSS at the end of <body>. The HTML spec isn't particularly helpful there and it's really a wild west in terms of how user agents should behave. I did some brief chrome testing and it doesn't look like it matters if render-blocking styles are in the head or in the body. First paint seems to be blocked in either case equivalently. Adding the feature to make certain styles non-blocking seems to have a lot stronger of a behavior guarantee than relying on progressive painting.

Something about

    <!-- ... -->
    <link href="style.css" rel="preload" as="style" onload="this.rel='stylesheet'">
    <noscript><link rel="stylesheet" href="style.css"></noscript>
  </body>

has an aura of elegance around it.

luke.leber → created an issue.

I'm on the fence of deferring CSS. It's often the delicate act of striking a balance between LCP and CLS. The list of things that you truly get for free when deferring CSS is pretty small. Of course, dynamic content added via direct user action (ajax stuff?) and non-visual content that can only be made visible by user interaction (the skip to content link visual styles -- not the css that hides it visually by default?) are the only ones that come to mind.

Users can even land on the page footer with the right #fragment in some cases. We provide dozens of jump links to help people link directly to the correct information, which in some cases means that the footer is effectively above the fold.

I tend to lean toward prioritizing CLS optimization where there's a choice (no one likes to see incomplete stuff or content that bounces around).

Also it feels like it's only worth inlining CSS if we also defer all other CSS

100%

I am mildly concerned about how the Drupal off-canvas CSS inline style rewrites would work with this. They were somewhat quirky sometimes with inline_all_css 😥.

Overall, I think that some real world data should be collected with this before arriving on a consensus. While I have a very strong suspicion that it'd lower LCP for most sites/themes, that intuition should be both quantified and peer reviewed.

In the case of the Penn State World Campus theme, inlining all render blocking resources significantly lowered LCP in lab testing (by a complete http round trip 🥳), but we haven't went forward with this in production yet. Sometimes RUM doesn't necessarily agree with what's measured in the lab.

I would second the notion of not enforcing this at the SDC layer due to inherent limitations. I can think up a couple of scenarios where having access to the entire working render tree may be beneficial to effectively enforcing design expectations.

• As a designer, when I design a component that should only be displayed within full-width contexts, a content editor shouldn't be able to add it in a way such that it would display otherwise.
• As an accessibility auditor, a component known to have dark text shouldn't be able to be placed if the closest parent component that ships with a background color is known to have a dark background

Layout builder restrictions offers an API that does offer quite a bit of additional context in ways that make non-trivial business logic able to be enforced. The "in the weeds" bits here are quite niche in nature, and will likely vary from design system to design system. Is providing an API for restriction management / XB tree validation on the table?

This seems like something we can write a test case for. I think before attempting a fix here, we'll need a failing test.

Equivalency to spaceless is rather heavy handed to achieve. Basically there are two preconditions:

1. There can't be any markup passed in via variables (I think we meet this criteria here!)
2. Every {%, %}, {{, }}, etc... have to be converted to use whitespace control operators (i.e. {{-, -}}

I think we'll still need the second point to apply here, but I'm still rather critical of the upstream decision to remove spaceless without a 100% solid replacement (see #1) ☹️.

luke.leber → created an issue.

kristen pol → credited luke.leber → .

Atlanta would be a great space to discuss this. Between LB styles, LB component attributes, and who knows what else in contrib space, how would contrib even coordinate all of these upgrade paths? Would every module do it independently? Only for their third party settings? What about custom stuff too?

I could see some sites needing like 4 or 5 iterations of saving every revision to make this work in contrib/custom...it just seems like a very messy thing for core to put the burden on site owners with.

If it's not an easy upgrade (read: something that non-technical site owners would have to spend $$$ on an agency to custom code a migration for), odds are trust will be damaged. In today's super competitive market, damaging trust shouldn't be downplayed -- especially if migrating to a competitor's solution ends up being cheaper to the business.

$.02 😥

Rather doubtful anything will happen with this as it's a Drupal 7 issue. Seems to have been committed to 8+ already.

I would like to be linked to one other core commit that would force site owners to run multiple content updates on every single revision of every single content entity on their site.

In the past 10 years I cannot think of one. IMO this sets a very dangerous precedent..

What happens if MYSQL goes away mid-update? Race conditions? Many known race conditions exist in core.

kristen pol → credited luke.leber → .

luke.leber → created an issue.

luke.leber → created an issue.

This sort of thing provides unique challenges (note how it's possible to produce contrast-inaccessible variants).

Originally, we sort of "flattened" the options, but found it wasn't flexible enough for our designer. More recently we've taken the approach in our in-house design system to make colors automatically contrast themselves based on their parent contexts. In general, components have light and dark variations set up and the CSS makes sure things stay accessible.

If curious, see https://psu-ooe.github.io/?p=atoms-cta-auto-contrast-deeply-nested (examples) and https://github.com/PSU-OOE/components/blob/main/packages/button-base/src... (driving CSS).

I also did a write-up at https://www.lukeleber.com/blog/2024-07-25-context-aware-components

It's a lot more complex on the CSS end, but even if JavaScript starts hacking and slashing the DOM after the page is built we usually never have to worry about color contrasting.

Food for thought 🙂.

I wasn't aware that more than one condition of a specific type per block was possible to configure. The core block GUI does not allow for that.

Perhaps with custom code or lesser popularity contrib though? That should be easy to resolve, but I'm not sure it's worth doing.

Those are really good points!

I think perhaps our use case (in using a condition plugin to control the presence of global blocks) may be best achieved with a different mechanism -- probably on the block types themselves.

I don't see an elegant way to hit all of the needs of this module as prescribed in the I.S. I'm leaning towards this being a wontfix.

Thanks for the swift response.

Update I.S. - Moving on to some manual testing on the issue fork on a real site :-).

Self-assigning to work on a pull request.

luke.leber → created an issue.

luke.leber → created an issue.

I've opened an upstream pull request that may resolve this linked to the issue that was reported upstream.

Hopefully the fix ends up being as simple as it seems!

luke.leber → created an issue.

Unassigned + sent to NR status

luke.leber → created an issue.

Everyone: Thank you for manually testing the Drupal 11 upgrade.

I'd like to leave an additional comment regarding the explicit dropping of support for Drupal 9.

Given that no testing has been performed against Drupal 9, it's my opinion that it's unlikely that any future enhancements to this module will be tested against Drupal 9 or prior either. Given this, it's even more unlikely that support ranges excluding Drupal 9 would be considered in future merge requests, meaning that unfortunate site owners that are still trapped in Drupal 9 could otherwise be unwittingly walked into WSOD situations. The most responsible thing to do as a community is to drop support so that users stuck on out of support versions of Drupal will not be able to brick their site by an upgrade of this module that couldn't be effectively tested against end of life software.

I'll cut a new release for Drupal 11 support shortly. Thanks again.

We also need to test on the previous major given Drupal 10 is still supported, so the gitlab CI template needs to be updated.

The MR pipelines are presently failing.

A composer.json file is required for phpunit testing in Gitlab CI.

There needs to be a test for this to happen. Happy to accept any test coverage 😇.

I don't have any cycles to do much more than keep the lights on, as I no longer manage any live sites that use this module.

I'll test drive this over the weekend on 11.x. Given there were no manual tests against Drupal 9 and it's end of life, I think it's best to explicitly drop support, leaving ^10.3 || ^11 as the support range.

Claiming that Drupal 9 is still actively supported would be somewhat false advertising 😅.

luke.leber → changed the visibility of the branch 3506838-consider-adding-scheduling to active.

luke.leber → changed the visibility of the branch 3506838-consider-adding-scheduling to hidden.

I plan on opening a pull request this weekend at some point, so self-assigning.

luke.leber → created an issue.

The root cause here is that version 2.0.6 introduced a MAJOR backwards compatibility problem.

Prior to 2.0.6, library dependencies could be relied upon in order to prevent really gnarly race conditions from happening between the GTM JS and Drupal libraries.

This happened because https://www.drupal.org/project/google_tag/issues/3452712 🐛 Possibly script loading/placement issue Fixed significantly changed the loading order in a way that bypasses dependency resolution. As a result, in order to prevent race conditions, any drupal library that is depended on by GTM must also be loaded in the head, and the GTM library definition must be hooked into in order to declare the dependency..

This feature would allow the Inline All CSS → module to continue to live in Drupal 11+.