When on an admin page with the local actions block (e.g. "Add content", "Create a new field", etc) at the top of the page, BigPipe will inject this slightly after pageload. This causes the entire page to render, and then immediately shifts the content down, which can be perceived as unpolished and janky.
Navigate to a page with the local actions block such as /admin/structure/types/manage/article/fields. Notice how the "Create a new field" button pops in, and then shifts everything down.
As of Drupal 10.1, BigPipe placeholder content can now be customized! Let's do this.
Do it and review it.
Instead of pushing everything down, space will now be reserved, and the buttons will pop in without shifting content around.
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
mherchelMR 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.
Comment #4
mherchelComment #5
smustgrave commentedTested 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.
Comment #6
catchOne question on the MR.
Comment #7
mherchelI'm not sure. I'm assuming the template naming convention (
big-pipe-interface-preview--block--claro-local-actions.html.twig)is based on Claro'score/themes/claro/config/optional/block.block.claro_local_actions.ymlconfig. 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.
Comment #8
catchI 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 #3437499: Use placeholdering for more blocks 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.
Comment #9
mherchelI'd appreciate it. I don't have too much contrib time available, and this is a bit outside of my wheelhouse.
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!
Comment #10
mherchel@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.
Comment #11
catchPut up a rough MR with the alter approach.
Comment #13
catchDifferences 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.
Comment #14
mherchelDoesn't appear to be working (Screenshot below).
To test this out
/admin/config/development/settingsadmin/structure/types/manage/article/fields. Note this is the only page where I consistently see this loaded by BigPipeI 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.
Comment #15
mherchelComment #16
catchI 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.
Comment #17
mherchelYeah, 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.Comment #18
mherchelCrap. This causes a regression on
/admin/structurethough. 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?
Comment #19
mherchelThe latest changes 1) correct the regression, and 2) fixes the original issue.
🙌 🙌 🙌
Thanks @catch!
Comment #21
catchI 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.
Comment #22
catchGot the unit test failing when it should - it specifically happens when the lazy builder returns auto-placeholderable cache contexts and the main render array doesn't, otherwise the placeholder preview gets passed around fine. Should be good now.
Comment #23
mherchel@catch mentioned in https://drupal.slack.com/archives/C7AB68LJV/p1714544076932459?thread_ts=...
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.
Comment #24
mherchelLooks good! Tests are passing and space is now being reserved for the button. Thank you @catch!
Comment #25
alexpottCommitted and pushed 3203676df8 to 11.x and 60b7fbf05a to 10.4.x and 041a12b513 to 10.3.x. Thanks!
Comment #29
wim leersTIL
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!
Comment #30
catch#29 yes exactly :)
Comment #32
duaelfrGood 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.