BigPipe injecting Local Actions block creates large janky layout shift in Claro

MR created. Here's a video of the block being added now. Note how space is reserved and the button appears without shifting any other content.

Tested this on Chrome 123.0.6312.122
After applying the MR I don't see any shift just a flicker which I believe is expected based on the 2nd mp4 in #4

Not seen this before but neat solution.

One question on the MR.

Isn't this an issue for all themes? Could we add preview HTML to local actions generically?

I'm not sure. I'm assuming the template naming convention (big-pipe-interface-preview--block--claro-local-actions.html.twig)is based on Claro's core/themes/claro/config/optional/block.block.claro_local_actions.yml config. That naming convention would obviously change based on the theme.

And, if I recall correctly, that was the only template suggestion that was shown within Twig debug (I had to set an early JS breakpoint to see this).

Right now, I'm trying double-check and get the local actions block to be pulled in by BigPipe, and it's not happening. I'm not quite sure how Drupal determines if it's going to immediately render the block or let BigPipe handle it, but I'm having issues right now checking the template naming suggestions.

That all being said, IMO this is an easy win, and fixes something that I've definitely noticed in client sites in the past. Claro obviously powers most Drupal admin pages (and Gin is a subtheme of Claro), so this would positively affect the vast majority of users.

Setting back to RTBC to get committers' attention. Feel free to kick back if needed.

I was thinking more that menu module could implement hook_block_build_local_actions_block_alter() and set up the preview markup in there.

Big Pipe placeholdering depends on the cacheability of the block and auto placeholdering conditions, but 📌 [PP-2] Use placeholdering for more elements to optimize asset serving Postponed would make that more predictable (i.e. on for a lot more things).

Local actions are also shown in Olivero - especially on entity pages like nodes, so I think this can equally be an issue there no?

Back to needs review, I might try to get a proof of concept up (in a separate MR in case it's a dead-end) for the block build alter idea.

I might try to get a proof of concept up (in a separate MR in case it's a dead-end) for the block build alter idea.

I'd appreciate it. I don't have too much contrib time available, and this is a bit outside of my wheelhouse.

Local actions are also shown in Olivero - especially on entity pages like nodes, so I think this can equally be an issue there no?

We don't place the primary admin actions block (as Claro does).

Setting back to Needs Work for now, so you (or anyone else) can work on the block build alter thing.

Thank you!

@catch thoughts on committing the template solution, and then creating a followup to implement it with hook_block_build_local_actions_block_alter()?

Although this only solves the problem with the local tasks block with the Claro theme, I'd argue that is is a very significant reduction of jank for 99% of users.

Put up a rough MR with the alter approach.

Differences with the alter approach are that I used the invisible class instead of style: although that could also be done in the template. The main difference is using #theme => menu_local_action which saves having to duplicate the classes from template_preprocess_menu_local_action()

The ul with class local actions is added by classy - I'm wondering a couple of things:

1. If we didn't add the ul, would just the link be enough to avoid layout shift? That would keep it closer to stark markup then.
2. If #1 isn't OK, is it OK for core to default to adding the list, I think it probably is though since it's still pretty generic overall?

I didn't try artificially slowing down the local actions rendering now to force the layout shift yet, so whether this is actually working yet is tbd.

Doesn't appear to be working (Screenshot below).

To test this out

1. Disable JS/CSS aggregation
2. Disable caching on /admin/config/development/settings
3. Go to a "Manage field" page such as admin/structure/types/manage/article/fields. Note this is the only page where I consistently see this loaded by BigPipe
4. Set a JavaScript breakpoint at the very beginning of drupal.js
5. Hopefully the local actions are not loaded. Use your inspector to find the span. Ideally this should be populated with placeholder data.

If we didn't add the ul, would just the link be enough to avoid layout shift? That would keep it closer to stark markup then.

I think so. I just experimented and I don't think the UL is adding any extra margin in Claro. I'll double check when the code is ready, but right now I don't think we need it.

I got it to work, but for some reason I had to force #create_placeholder. If that's not set, it still gets rendered via a bigpipe placeholder, but not with the preview - haven't figured out why yet. Just the link is indeed plenty to reserve the vertical space.

Yeah, verified that it's working correctly now. The containing elements are both 48px height before and after BigPipe gets to work.

Note you could also yank out that <li> if you want.

Crap. This causes a regression on /admin/structure though. The local actions BigPipe placeholder loads... but since there isn't any local actions, BigPipe simply removes the span, causing the layout to shift up.

Is there anything we can do about this? Not sure if we can limit it to routes or some other way?

The latest changes 1) correct the regression, and 2) fixes the original issue.

🙌 🙌 🙌

Thanks @catch!

mherchel → changed the visibility of the branch 3441137-bigpipe-injecting-local to hidden.

I added some unit test coverage. I cannot figure out how to make the unit test fail, however it does add some explicit coverage for things we don't have unit test coverage for.

Got the unit test failing when it should - it specifically happens when the lazy builder returns auto-placeholderable cache contexts. Should be good now.

@catch mentioned in https://drupal.slack.com/archives/C7AB68LJV/p1714544076932459?thread_ts=...

One last commit on the MR after that - used   so the link actually shows up (then gets wider) instead of being invisible, I have no idea if that is better or worse, feel free to revert the last commit if you don't like it but would re-RTBC otherwise.

We do need the invisible class there. Otherwise the placeholder will be visible as a very narrow button (it'll be visible because it has a background color), and before it's replaced. IMO its better for the actions to just pop in.

I reverted that last commit. Going to do a bit of testing, and then should be ready for a code review.

Looks good! Tests are passing and space is now being reserved for the button. Thank you @catch!

Committed and pushed 3203676df8 to 11.x and 60b7fbf05a to 10.4.x and 041a12b513 to 10.3.x. Thanks!

TIL hook_block_build_BASE_BLOCK_ID_alter() exists 🤯

So this uses that hook to generate a "Add" button as the local action … without that local action pointing anywhere … and with that local action being hidden. Just to consume enough vertical space that when the placeholder is swapped, there's no layout shift. Intriguing!

#29 yes exactly :)

Good job here!
One concern though:
Isn't "Add" an inappropriate button label in some rare edge case where CSS is disabled or overridden somehow? I believe it would be better to use another string explaining what's happening here like "Loading" for example.

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