Toolbar height calculation is faulty in multiple cases

Hi @jessebaker thanks for reporting, would be great if the next time you use the Issue Summary Template to report an issue.

Do you have some strange behavior with the module? Can you show some screens?

I see, can you help us providing a patch for this?

@jessebaker no problem, this is normal, ;-) you can move this issue to the Drupal core issue queue and select the correct component.

Hi,

https://www.drupal.org/project/drupal/issues/2959062 same issue in drupal 8.4.x-dev. I have applied patch. thanks.

Is this a duplicate of #2951268: Improve rendering account link in the toolbar?

This patch solves the issue Core Settings tray hides navbar in normal mode.

Yes it does thank you so much for this patch.

Confirming this patch works.

Tagging for subsystem maintainer review.

The latest patch also changes the compiled JavaScript file directly. Please update the patch to change the ES6 file and then compile it to take effect. More info: https://www.drupal.org/node/2815083.

What if we just use the toolbar-bar's height if it contains at least one visible item (instead of the first visible item's height)?

The attached patch changes both the compiled JS file directly and the ES6 file.

Confirming this patch works.

Added a test-only patch and a test+fix patch with an improved toolbar height calculation.

Summary about the test extensions:

Toolbar height test theme (toolbar_test_themeTest):

• Extends stark.
• Contains a library extension for the system/base library's hidden.module.css that hides .hidden elements with display: none !important;. This mimics the attribute of Bootstrap that is crucial in the reported issue.

Toolbar items for toolbar testing module (toolbar_test_toolbar_items):

• Adds a link to the home page with hook_page_top().

  The test tries to click on this element. Without the proper fix, this is impossible, because the toolbar overlaps this link.

• Provides a toolbar tab item with .hidden CSS class and -1000 weight. This mimics the Edit toolbar item (provided by the contextual module and altered by settings_tray) thats get hidden if no contextual links are present on the current page.
• Optionally provides additional toolbar (tab) items that can be set with Drupal State API. These items are used to make the toolbar bar occupy more lines (and make it much taller than expected). This is needed for underlining why #17 is not the best solution.

The test (contains four test cases):

• The first test case reproduces the originally reported error (Settings Tray enabled, .hidden elements are removed with display: none !important;, current user has permission to access contextual links, but these are removed from the page).
• The second test case tests uses three additional toolbar tab that follow the Manage item. This items are big enough to make the toolbar bar be multi-line.
• The third test case tests uses the same extra toolbar tabs that case #2, but here those follow the first, hidden tab and precede the Manage item.
• The fourth test uses a multi-line toolbar tray for testing the toolbar height calculation.

I am currently working on a project which encountered a similar height issue of the toolbar causing the toolbar items overlapping the tray items. The cause was too many toolbar items (we use the contenta cms installation profile). To reproduce on vanilla Drupal, I have created a toolbar_test module which I am am attaching to this issue.

To reproduce:

• Download
• Un compress in modules/ directory
• Install the module via drush or from /admin/modules
• Zoom in if required. I had to zoom in to 120% to reproduce locally, which my colleague was able to see it at 100%

Adding a screenshot below (after clicking the manage item):

Re #29, @AjitS:

I'd fix the bug you described in a separate issue. This is about the toolbar height calculation.

Back to NR.

It is possible to switch to position: sticky instead of position: fixed. This way, we can completely ditch the ugly padding hacks we have use now. The effect will be the same, only sticky doesn't work in IE (see https://caniuse.com/#feat=css-sticky). The toolbar will still functionally work in IE though.

I just tested the concept and succeeded to get rid of the padding hacks. There's some rewriting to be done for it (the navbar will need to use flexbox instead of floats and I'll need to finetune everything for mobile.

Let me know if I should continue on this. As well template, CSS and JS changes will be needed, it's gonna be quite a large patch.

Was there an issue made for #29 @huzooka? We have the same problem on a project and I am searching for a solution.

I'm still running into an issue where the height is being calculated after orientation change. If I have 1 extra menu tab, the menu drops to 2 lines, and then if I stretch the screen a little wider the extra menu tab goes up onto the 1st line, but the toolbar height does not get calculated again so I end up with too much padding and a gap of space. Anyway we can calculate as the viewport width changes?

To be honest at this point i would try to use position:fixed and let IE have a non-sticky toolbar, I was never comfortable doing this kind of page offsets. There were a few attempts a few years ago but support was too low. Need signoff from Usability folks though.

I can see we're not updating the drupal-offset-top attribute, any idea when se stopped using that?

Related check out @droplet module: https://www.drupal.org/project/toolbar_anti_flicker

@nod_, @porchlight you only have to apply https://www.drupal.org/project/drupal/issues/3035577 (listed as a related issue)

@huzooka I tried the patch in that issue as well but still no luck. I'm not sure where to put my feedback at this point. I still think this needs work, but I'll just follow whichever one has more action.

@nod_, position: fixed works in IE, position: sticky doesn't

FWIW, the CSS only solution would look something like this: https://codepen.io/MartijnCuppens/pen/eYpabGZ
In IE, the toolbar just isn't sticky.

Right I meant sticky! Sorry

@porchlight, I thought you have to apply both patches at once.

The if I stretch the screen a little wider case is fixed by #3035577: Re-calculate toolbar's height every time when the viewport changes.

@huzooka - I tried applying both this patch and the patch from #3035577 but they won't apply at the same time. Tried ordering this patch first then that one, didn't apply. Tried applying that patch then this one, didn't apply... Maybe they can be combined?

Can we make a meta out of this issue please? too many different patches for slightly different issues. It's very hard to follow and review I could use some help aggregating everything. Thanks :)

@nod_, If you think that a meta would be useful, please create a new meta issue instead of changing this to a meta.

Maybe you missed, but I we have a patch here that also provides test coverage. Meta issues shouldn't have patches.

Seems like there is an issue with offcanvas. Using the module that adds a link and a menu item, on the offcanvas test page, the link is hidden: /off-canvas-test-links and when opening/closing a tray, the margin is a bit too extreme probably not entirely toolbar fault but still a regression.

The code is very similar to Drupal.displace, in fact doing

      this.model.set('height', Drupal.displace.calculateOffset('top'));
      $('body').css({
        'padding-top': this.model.get('height')
      });

has the same effect as the code in the patch and is broken in the same way with off-canvas.

Seems like the issue was caught before we changed the code to remove the flicker.

MULTIPLE LINES is illegal usage IMO. To me, I'd say many items also illegal usage! (If you do, MAX of 7 I think, which can fit into a small 375px screen)

If the CORE accepts multiple lines:
- It should be scrollable or extra dropdown (also Bad UX)
- You should consider what happens when the items covered half or 90% of the screens, or even 100% :s

In another word, if you used a lot of items and can't fit into a single line, make it responsive (to icons, even on a wider screen)

Still a problem with Drupal 10.1.2 and Seven 1.0.0 (contrib)

Before:

After:

re-rolled patch

Tried fixing CCF in #55, 55 not applied on D11, so not attaching the interdiff.

Fixed CCF failure

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

Trying to fix errors reported by bot.

Seems to have caused nightwatch failures.

Converted 59 to an MR, hiding patches.

Not just nightwatch that's failing now:

Functional JavaScript:

• Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest::testBlocks
• Drupal\Tests\layout_builder\FunctionalJavascript\LayoutBuilderToolbarTest::testBackToSiteLink
• Drupal\Tests\contextual\FunctionalJavascript\ContextualLinksTest::testContextualLinksClick
• Drupal\Tests\toolbar\FunctionalJavascript\ToolbarHeightCalculationTest::testToolbarHeightCalculation

Nightwatch:

• Check toolbar overlap with page content

I was able to fix this issue by adding one line of css.

#toolbar-item-administration-tray.toolbar-tray-horizontal {
  height: 40px;
}

Summary of my changes:

• Merged in the main branch. (Removed two files which were deleted by main, the MR just added a declare line to them anyway.)
• Added px units to the body paddings, it would not work correctly without it.
• Added an explicit top style to the vertical toolbar tray so it follows --drupal-displace-offset-top when displacement is recalculated.
• Increased the z-index of the horizontal tray so that it renders on top of any extra wrapped toolbar rows. Otherwise clicks would got to the wrapped toolbar tabs instead of the tray.
• Added a resize observer to recalculate the toolbar height (and body padding) when the main toolbar element changes its size, fixes issues with resizing making the toolbar and tray overlapping or leave big gaps. Added a small debounce to this, which makes it less smooth, but may help lower-end devices.
• Fixed various linting errors.
• Fixed toolbarApiTest.js - Not sure why the size increased to 80px, maybe rounding error?
• Fixed AssetAggregationAcrossPagesTest.php - Looks like the changed scripts were part of the asset aggregation size test and the change was large enough to fail it.

Some test runs also failed CKEditor5MarkupTest.php, but that test seems really unreliable even main and it looks like a race condition. Running it multiple times locally makes different parts of it fail. If I insert a sleep(1) on line 346 before it checks if the response contains the expected scripts it will always pass. I did not include that change here though as it seemed too unrelated.

The last run failed testEditorFileReferenceIntegration::testEditorFileReferenceIntegration(), again it is also failing randomly for me on main.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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