Improve rendering account link in the toolbar

I think this should also be fixed in core/modules/toolbar/css/toolbar.icons.theme.css as well as in the theme?

The video added to this issue makes it clear that this change is helpful. Patch also looks good, great work @Chi

Any steps for reproducing this bug? I tried with chrome and I couldn't see any flickering.

Hi @Chi

I have enabled the Big pipe module but I couldn't see any flickering. How can I reproduce this issue?Thank you

Was send here by Chi from #2998451: Toolbar tray rendering can result "flickering" resizing of content area to accommodate open trays

4 gifs:
Drupal 8.7.x Standard profile:
1. Mobile: 2951268-below-mobile-breakpoint--without-patch.gif
2. Above mobile breakpoint: 2951268-above-mobile-breakpoint--without-patch.gif
See issue-summary.

Drupal 8.7.x Standard profile + Patch #5
3. Mobile: 2951268-below-mobile-breakpoint--with-patch-5.gif
4. Above mobile breakpoint: 2951268-above-mobile-breakpoint--with-patch-5.gif

Patch #5 works nicely, but only above the mobile breakpoint (only screen and (min-width: 36em))
Needs work because of gif 3.

@lauriii #7 This is just standard profile. I used Giphy capture on OSX+Chrome to record this.

I applied patch #5 and added the same 39px to the mobile .toolbar-icon:before
Result no toolbar-jumping anymore!

My patch was wrong because I used drupal-composer-template for development. Let the gitlab integration come soon1

Added fix based on @ndf changes this also fixes jumping/flicker toolbar issues https://www.drupal.org/project/drupal/issues/2998451

Thanks drintios!
Can you provide a interdiff between #19 and 17? That makes reviewing easier.

I have tested your patch and it works nice for the account link (this issue), but it doesn't solve #2998451: Toolbar tray rendering can result "flickering" resizing of content area to accommodate open trays.

Regarding #2998451: Toolbar tray rendering can result "flickering" resizing of content area to accommodate open trays see my comment #12. Your patch solves the horizontal orientation. But when the toolbar is hidden or has vertical orientation there is a redraw.

We seem to be super close to fixing the account link icon here.
Let's don't make it extra hard by adding changes to the toolbar too. (and do that in 2998451).

Re-roll of #19 that applies to 8.7.x. Interdiff is confused, so this is from:

diff 2951268-19-toolbar-rendering.patch 2951268-24.patch > 2951268-19_24.interdiff.txt

Otherwise, doesn't address #21 so still NW.

Cheers,
-Derek

If you're also using #2801501-5: Option for the Toolbar *not* to auto-open the "Manage" sub-menu? #24 is wrong. We always want min-height 39 in that case.

#25 This patch cannot be applied in the Drupal 9.1.x , so needs reroll.

Re-rolled against 9.1.x.

Flickering issue is resolved on 9.1 after adding patch #30

reviewed 2951268-30.patch in drupal 9.1 dev version , Flickering issue is resolved .

Thanks everyone for their efforts here and having a working patch. Yay!

Just a few things to do here.

I read the Issue Summary and it explains the problem. It also needs to include the proposed resolution and since this is a UI issue, it needs to have before and after screenshots. It will help reviewers as well if the Issue Summary template is used, there is a button for that if you use dreditor. Setting needs issue summary for that.

Next I looked at the patch, and it is all css/js so I have no comment on that.

Next I looked at the test results, https://www.drupal.org/pift-ci-job/1787674, and see that there are 56 coding standard errors. Setting NW for that.

Next I looked at the tags, this is marked needs manual testing and needs reroll. It looks like that has been done, so removing tags. Please, as you do work on an issue, update the tags. This will save time for anyone, for example, someone who is looking to reroll a patch and finds this issue. Takes time to skim the issue and then finds the reroll is done.

#30 I have applied this patch and it was working fine for me .

1. This needed a reroll. Attaching a patch that applies cleanly to both 9.2.x and 9.3.x (for now).
2. Also added the same CSS changes to stable9. Not sure we should change stable* at all, but if so, we should do both.
3. Updated the summary with the template. Removing that tag.
4. Re: "56 coding standard errors": I believe that core CSS files don't comply with our current linting standards. Any patch that touches the CSS ends up triggering a "these files changed" check and all existing errors in each modified file are reported as CS violations. They're pre-existing, not introduced by this patch. At least that's how I understand the current situation.
5. Remaining tasks include:
   • Decide if we can change stable + stable9 like this, or if we should leave them out of this fix.
   • Decide if we need a CR.

   RFC

Thanks!
-Derek

Applied #38 patch applied successfully
After patch in profile icon in toolbar min-height added
Thanks for the patch
For ref sharing screenshot...

Last patch doesn't apply to target branch 10.1.x-dev, please reroll the patch.
Thank you.

I failed to find this issue before creating the very similar #3331531: Reduce toolbar reflow/repaint due to username lazy builder. My approach is different, but should be easier to review as it's just removing a few lines. Plus, it addresses inefficiencies with vertical and horizontal rendering, while the current solution here only addresses vertical - maybe @dww or someone else on this issue can weigh in on their preferred approach and we can close one of the issues and transfer credit.

Here's a new approach that doesn't require specifying heights.

Not sure where did #47 come from but looks like #46 passed 🤔

Verified the patch #47 and tested it on Drupal version 10.1.x. Now the vertical position of the account icon is not changing on page load and I have added the before and after screen recordings for reference.

#2632750: Interface previews/skeleton screens through optional "preview" or "placeholder" templates landed which means we could probably customize the placeholder for the account link.

Patch has the placeholder approach, and here's a video of it working on a throttled connections, so we can see that the icon remains in the correct location throughout the deliberately slow load process. https://youtu.be/JrU7Cvqr53c

Not able to verify the patch #53 as it is showing error while applying the patch:

Checking patch core/lib/Drupal/Core/Render/Renderer.php...
error: while searching for:
        '#cache',
        '#create_placeholder',
        '#lazy_builder_preview',
        // The keys below are not actually supported, but these are added
        // automatically by the Renderer. Adding them as though they are
        // supported allows us to avoid throwing an exception 100% of the time.

error: patch failed: core/lib/Drupal/Core/Render/Renderer.php:327
error: core/lib/Drupal/Core/Render/Renderer.php: patch does not apply
Checking patch core/modules/user/user.module...

@Nayana Ramakrishnan just above your comment in #54 is clear evidence the patch applies fine. Whenever a patch is uploaded, Drupal's testbot applies that patch to the specified branch and runs tests. If there's a green box stating "pass", that means the patch is applying to Drupal fine.

Since it's proven that the patch is applying fine to Drupal 10.1.x, the issue is specific to you and doesn't need a comment here. If you need help with troubleshooting the patch not applying, Drupal Slack might be a good place to look as what you're dealing with is not specific to this issue. Ideally we avoid comments within an issue that are not specific to that issue (such as the one I just typed... 😐)

@bnjmnm Thank you for pointing out that. I guess there was some issue with my repo. I created a new repo and applied the patch. Patch #53 applied cleanly and tested it on Drupal version 10.1.x. Attaching the before and after screen recordings for reference.

Reviewed this.

+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -327,6 +327,7 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
+        '#preview',

This change was surprising to me at first, but after re-reading #2632750: Interface previews/skeleton screens through optional "preview" or "placeholder" templates, it was a missed addition from that issue.

Due to the nature of this change (printing a space during rendering), and given the gaps in our testing infrastructure, this is not testable (other than manually).

Manual review shows that the vertical position is fixed!

Committed 905efbc and pushed to 10.1.x. Thanks!

Linked this as an example in the interface preview change record: https://www.drupal.org/node/3338948