Problem/Motivation

With big pipe enabled on profile pages the toolbar overlaps the content.

After loading the page initial the toolbar overlaps the heading.

After clicking on admin in the menu to close the sub menu the same margin as before is used.

After opening the sub menu again the height is correctly set and all the content moves downwards.

Steps to reproduce:

  1. Install a fresh Drupal site with a standard profile.
  2. Login as admin
  3. Click 'Edit profile' on the toolbar
  4. You can see that the toolbar overlaps the body of the page.

With BigPipe enabled this happens as the toolbar links are fetched later by Drupal\user\ToolbarLinkBuilder::renderToolbarLinks

On load the toolbar gets initializied and sets the padding-top of 40px. After that big pipe adds the toolbar, but doesn't invoke ToolbarVisualView.updateToolbarHeight

Proposed resolution

TBD

Maybe an option would be that big pipe introduces a new event when it's finished loading a block so ToolbarVisualView then can update the toolbar height.

Remaining tasks

Fix and test

User interface changes

none

API changes

none

Data model changes

none

Release notes snippet

CommentFileSizeAuthor
#49 3021147-nr-bot.txt150 bytesneeds-review-queue-bot
#48 After_Patch_Seven.PNG15.52 KBgaurav-mathur
#48 Before_Patch_Seven.PNG15.33 KBgaurav-mathur
#48 After_Patch_Claro.PNG7.92 KBgaurav-mathur
#48 Before_Patch_Claro.PNG10.11 KBgaurav-mathur
#37 After Patch 3021147 Two.png283.36 KBchetanbharambe
#37 After Patch 3021147 One.png305.85 KBchetanbharambe
#37 Before Patch 3021147 Two.png314.63 KBchetanbharambe
#37 Before Patch 3021147 One.png293.72 KBchetanbharambe
#32 3021147-32-test-only-should-fail.patch2.37 KBkndr
#29 interdiff-3021147-24-29.txt557 bytesyogeshmpawar
#29 3021147-29.patch2.46 KByogeshmpawar
#28 interdiff-3021147-24-28.txt557 bytesyogeshmpawar
#28 3021147-28.patch2.82 KByogeshmpawar
#24 interdiff_21-24.txt891 bytesvsujeetkumar
#24 3021147-24.patch2.34 KBvsujeetkumar
#23 interdiff_21-23.txt773 bytesvsujeetkumar
#23 3021147-23.patch2.5 KBvsujeetkumar
#21 3021147-21.patch2.5 KBsuresh prabhu parkala
#19 patchapplying.png116.24 KBrinku jacob 13
#19 beforepatch.png87.13 KBrinku jacob 13
#18 After-patch-pipeline.png148.11 KBMadhu kumar
#17 Patch 3021147 Two.png286.79 KBchetanbharambe
#17 Patch 3021147 one.png275.94 KBchetanbharambe
#15 Error at time of Applying patch.png74.73 KBmanojithape
#15 Before patch.png24.22 KBmanojithape
#10 afterPatch.JPG34.74 KBpankaj.singh
#10 Toolbar Overlap_beforePatch.JPG27.66 KBpankaj.singh
#10 39x padding_beforePatch.png8.6 KBpankaj.singh
#5 toolbar_initial_height-3021147-5.patch2.28 KBjamesashok
#5 Before Patch.png47.2 KBjamesashok
#5 After Patch.png46.38 KBjamesashok
Bildschirmfoto 2018-12-18 um 10.45.00.png15.41 KBkfritsche
Bildschirmfoto 2018-12-18 um 10.44.52.png12.4 KBkfritsche
Bildschirmfoto 2018-12-18 um 10.44.37.png15.71 KBkfritsche

Issue fork drupal-3021147

Command icon Show commands

Start 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:

About issue forks

Comments

kfritsche created an issue. See original summary.

kfritsche’s picture

kfritsche
Primary language German
Location 🇩🇪🇪🇺
commented
Related issues: +#2958478: Toolbar height calculation is faulty in multiple cases, +#2951268: Improve rendering account link in the toolbar, +#2998451: Toolbar tray rendering can result "flickering" resizing of content area to accommodate open trays

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

jamesashok’s picture

jamesashok commented
Assigned: Unassigned » jamesashok
Status: Active » Needs review
Issue tags: +toolbar, +overlap, +content, +updateToolbarHeight, +height, +ToolbarVisualView
StatusFileSize
new After Patch.png46.38 KB
new Before Patch.png47.2 KB
new toolbar_initial_height-3021147-5.patch2.28 KB

Issue is related to Issue #2952447.

The following patch is being updated on both issues.

The issue arises because the Toolbar height is being calculated before its contents are rendered on the initial load at ToolbarVisualView.js; so, the patch makes the height calculation asynchronous, by deferring it and pushing it to queue. So, once the contents are rendered, the height of the toolbar will be calculated.

Tested across all major browsers and OS simulations and adding a couple of screenshots..

Before Patch

Before Patch

After Patch

After Patch

toolbar_initial_height-3021147-5.patch

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Issue tags: -toolbar, -overlap, -content, -updateToolbarHeight, -height, -ToolbarVisualView
jamesashok’s picture

jamesashok commented
Assigned: jamesashok » Unassigned

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

pankaj.singh’s picture

pankaj.singh commented
Assigned: Unassigned » pankaj.singh
pankaj.singh’s picture

pankaj.singh commented
Assigned: pankaj.singh » Unassigned
StatusFileSize
new 39x padding_beforePatch.png8.6 KB
new Toolbar Overlap_beforePatch.JPG27.66 KB
new afterPatch.JPG34.74 KB

Tested on 9.1 and the issue is reproducible here. Applied the patch and its fixes the issue.

Atatched the SS for ref. RTBC+1

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

ramonma1989’s picture

ramonma1989
Primary language Spanish
commented

#5 worked for me. Tested on D8.9.13. Thanks.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

manojithape’s picture

manojithape commented
Assigned: Unassigned » manojithape
manojithape’s picture

manojithape commented
StatusFileSize
new Before patch.png24.22 KB
new Error at time of Applying patch.png74.73 KB

Same issue I tested on Drupal 9.2.x Dev version and the issue is reproducible here. Please refer attached Before patch screenshot.
@jamesashok At the time of applying patch#5 getting an error and not able to apply the patch.

manojithape’s picture

manojithape commented
Assigned: manojithape » Unassigned
chetanbharambe’s picture

chetanbharambe commented
StatusFileSize
new Patch 3021147 one.png275.94 KB
new Patch 3021147 Two.png286.79 KB

This issue is no more existing on the 9.3.x-dev version.
As a user, I am not able to see toolbar overlap content with big pipe enabled.

Need +1 RTBC

Madhu kumar’s picture

Madhu kumar commented
StatusFileSize
new After-patch-pipeline.png148.11 KB

I'm unable to replicate the issue in 9.3.x-dev version.
Sharing Screenshot for reference.

rinku jacob 13’s picture

rinku jacob 13 commented
StatusFileSize
new beforepatch.png87.13 KB
new patchapplying.png116.24 KB

i have same issue on 9.3.x-dev.but the above patch not applicable for 9.3.x-dev.
i am getting error while applying the patch.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work
Issue tags: +Needs reroll
suresh prabhu parkala’s picture

suresh prabhu parkala commented
Status: Needs work » Needs review
StatusFileSize
new 3021147-21.patch2.5 KB

A re-rolled patch against 9.3.x.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work
vsujeetkumar’s picture

vsujeetkumar commented
Status: Needs work » Needs review
StatusFileSize
new 3021147-23.patch2.5 KB
new interdiff_21-23.txt773 bytes

Fixed "Custom Command Fail" issue, It is related to file "ToolbarVisualView.es6.js" is not updated.

vsujeetkumar’s picture

vsujeetkumar commented
StatusFileSize
new 3021147-24.patch2.34 KB
new interdiff_21-24.txt891 bytes

Updated patch, Please ignore previous one.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented 
+      // Update toolbar height asynchronously to ensure the toolbar elements are
+      // rendered to DOM before fetching them.

Should not that comment also be added to the ToolbarVisualView.js file? The patch adds it only to the ToolbarVisualView.es6.js file.

kndr made their first commit to this issue’s fork.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work
yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 3021147-28.patch2.82 KB
new interdiff-3021147-24-28.txt557 bytes

Addressed #27 & added an interdiff.

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
StatusFileSize
new 3021147-29.patch2.46 KB
new interdiff-3021147-24-29.txt557 bytes

Accidentally added one extra line at top of the file so removing it, which resolves custom commands failure.

kndr’s picture

kndr
he/him
Primary language Polish
Location Rybnik, Poland
commented
Issue summary: View changes

I've just added a short version of the "Steps to reproduce" in the Issue summary.

kndr’s picture

kndr
he/him
Primary language Polish
Location Rybnik, Poland
commented
Status: Needs review » Needs work

I followed the steps to reproduce but #29 doesn't work for me.

kndr’s picture

kndr
he/him
Primary language Polish
Location Rybnik, Poland
commented
Status: Needs work » Needs review
Issue tags: +Bug Smash Initiative
StatusFileSize
new 3021147-32-test-only-should-fail.patch2.37 KB

Here is the test only. The patch should fail.

kndr’s picture

kndr
he/him
Primary language Polish
Location Rybnik, Poland
commented

I've pushed the new patch to the branch. As I mentioned, #29 doesn't work for me. I've tested #29 manually with no good result. I like the idea that the toolbar can response to the event triggered by Big Pipe when its work is done. Unfortunately I didn't find any event. I also tried the solution introduced at #2794099-11: JS relying on document.ready event does not work with BigPipe but it doesn't work for me too. Eventually I implemented the solution described at #2794099-9: JS relying on document.ready event does not work with BigPipe. Patch works fine but one issue is still bothering me. The test #32 combined with #29 passed without any errors despite the fact that #29 failed when I tested it manually. I can't explain that. I think the test should be altered in some way.

kndr opened merge request !1329

Status: Needs review » Needs work

The last submitted patch, 32: 3021147-32-test-only-should-fail.patch, failed testing. View results

kndr’s picture

kndr
he/him
Primary language Polish
Location Rybnik, Poland
commented
Status: Needs work » Needs review
chetanbharambe’s picture

chetanbharambe commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new Before Patch 3021147 One.png293.72 KB
new Before Patch 3021147 Two.png314.63 KB
new After Patch 3021147 One.png305.85 KB
new After Patch 3021147 Two.png283.36 KB

Verified and tested merge request !1329 - (https://git.drupalcode.org/project/drupal/-/merge_requests/1329.patch)
Merge Request applied successfully and looks good to me.

Testing Steps:
# Goto: Appearance -> Apply Seven theme
# Click on Profile section
# Click on Edit Profile
# Observe the results

Expected Results:
# After applying merge request!1329, User should not see toolbar overlaps on header and content. (should see some margin has been added.)

Actual Results:
# Currently, the toolbar overlaps on the header and the content.

Please refer attached screenshots for the same.
Looks good to me.
Can be a move to RTBC.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

We still need to make this apply to 9.4.x and 10.0.x.

ankithashetty made their first commit to this issue’s fork.

ankithashetty opened merge request !1874

ankithashetty’s picture

ankithashetty
Location Karnataka, India
commented

Here is an MR to 9.4.x branch, thanks!

ankithashetty opened merge request !1875

ankithashetty’s picture

ankithashetty
Location Karnataka, India
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll

Here is an MR to 10.0.x branch, thanks!

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

gaurav-mathur’s picture

gaurav-mathur commented
Assigned: Unassigned » gaurav-mathur
gaurav-mathur’s picture

gaurav-mathur commented
Assigned: gaurav-mathur » Unassigned
StatusFileSize
new Before_Patch_Claro.PNG10.11 KB
new After_Patch_Claro.PNG7.92 KB
new Before_Patch_Seven.PNG15.33 KB
new After_Patch_Seven.PNG15.52 KB

Patch #32 applied successfully on verify and tested on Drupal 10.1.x-dev with claro and seven theme.
The patch work properly for me.
Refer to screenshots.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3021147-nr-bot.txt150 bytes

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

quietone’s picture

quietone commented
Status: Needs work » Postponed

The Toolbar Module was approved for removal in #3476882: [Policy] Move Toolbar module to contrib.

This is Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

The deprecation work is in #3484850: [meta] Tasks to deprecate Toolbar module and the removal work in #3488828: [meta] Tasks to remove Toolbar module.

Toolbar will be moved to a contributed project before Drupal 12.0.0 is released.

quietone’s picture

quietone commented
Project: Drupal core » Toolbar
Version: main » 1.x-dev
Component: toolbar.module » Code
Status: Postponed » Needs work

Toolbar module has moved to contrib.