Set the toolbar to position fixed on mobile

Here is a plain CSS fix for this. Probably the javascript should be updated not to toggle the toolbar-fixed css class, but I'm not familiar enough to change the code and make sure it works with the Barebone library.

I'll ping tkoleary and ask if there was a design decision behind not making it fixed position.

It was because position fixed didn't work in iOS 7.
http://caniuse.com/#feat=css-fixed

I think JohnAlbin may be right: I vaguely remember that this was done solely for browser compatibility reasons. It would be good though if Kevin could confirm this.

I can't seem to recreate the original bug on my iOS7 simulator, it's possible it was fixed in a 7.x point release.

Applying Drupalisms to iOS :P

Here's another reason to make it fixed:

Hi, all!

If we will decide to keep toolbar menu fixed. I suggest this patch.
I've changed style for toolbar and removed unnecessary js code.

Also, about zoom bug.

https://www.drupal.org/files/issues/prevent_auto_zoom_in_mobile-2516918-...
This patch can fix many issues with zoom on mobile. But it might not be so comfortable for people with bad eyesight.

+++ b/core/modules/toolbar/js/views/BodyVisualView.js
@@ -38,8 +38,6 @@
-        .toggleClass('toolbar-fixed', (isViewportOverflowConstrained || this.model.get('isFixed')))

We should also remove the isFixed property from the model, and the toolbar-fixed class from the CSS and JS.

Removed "isFixed" from model and also removed "toolbar-fixed" class from the CSS and JS.

Looks good, but this does mean we need another round of manual testing IMO.

Things related to isViewportOverflowConstrained must go too, that would solve the eslint error too.

And now there is a zindex issue, some header links show up on top of the toolbar (like my account, home, log out).

Removed things related to isViewportOverflowConstrained, added position: static for #toolbar-administration to prevent zindex issue.

It's a bit buggy from my last test in #14 when you scroll the mobile down and scroll back to top very fast (like pull to fresh, out of screen size).

the debounce function in Drupal.drupalDisplace caused the wrong calculate. (leaves a big grey gap)
$(window).on('resize.drupalDisplace', debounce(displace, 200));

@droplet That sounds a lot like the issue I reported in #2551777: Scrolling creates an empty space on mobile browsers a while back. It is actually a separate bug since it happens regardless of the position:fixed property.

I'm going to review this.

Issue is fixed & tested.

Before applying patch

After applying patch

#21 lost all JavaScript changes. It was a bad reroll.

What about patch from #18 ? Wich includes javascript changes and zindex fixes.
Just re-added.

I like some changes in this patch. But I'd say to keep it clean and let it move forward. Dead code / improves can be cleanup in other issues.

Above JS changes are uncertain (from my EXP on #2542050: Toolbar implementation creates super annoying re-rendering.. I can't tell the exact problem now but it's a bit tricky.)

.toolbar-fixed was designed to set Toolbar to be fixed. But now it is not present in mobile. Here's the problem we have to address.

I closed #2864051: Admin toolbar disappears when scrolling and #2562251: .toolbar-bar becomes detached from fixed position tray items when scrolling on mobile, leaving gap as a duplicate for this.

Applied #31 patch on 8.4.x. Worked for me. Just need to clear cache after patch

Patch works well for me too. The duplicates @laurii has noted were RTBC and have been closed already.

There are 4 coding standards messages but are unrelated to this patch.

Setting this to RTBC unless anyone else objects.

It has a wired implementation in Toolbar's MQ handler

Rerolled #36 for 8.8.x.

Referring the above comments and taking pointers from #9, #13 and #16, the following patch makes toolbar fixed across all screen sizes and browsers. Please refer the screenshot attached...

With this patch,
# isFixed and isViewportOverflowConstrained occurrences from JS has been removed.
# Narrow devices will have a fixed top admin toolbar and an overlapping secondary side toolbar.
# Standard devices with side secondary toolbar will push the main content to the right (no overlapping secondary side toolbar), which is the default behaviour as usual (left undisturbed)
# The toolbar-fixed has been permanently added to the toolbar. It is not removed from the code, just in case if it might find some use or other options to compete with in the future.

I have tested across all major browsers and OS simulations and attaching now a couple of screenshots..

Before patch

After patch

#44: very interesting patch, thanks!

It's been too long since I looked at the Toolbar module's code in detail, so I can't sign off on this. My thoughts:

1. 🥳 Simplification, yay!
2. 🥳 But … is it really okay to delete all that code? I feel like we're missing something. It'd help if you could explain why the code that the patch is deleting is no longer needed. Do you think you could do that?

Detailed Changes made to the code with the patch #44:-

1. /core/modules/toolbar/css/toolbar.module.css & /core/themes/stable/css/toolbar/toolbar.module.css
   • Styles when toolbar fixed are adjusted and moved to a media query
   • Because, for narrow devices the toolbar should overlap the main content
   • And for wider devices (landscape, tablet, desktops...), the toolbar should push the main content right
2. /core/modules/toolbar/js/models/ToolbarModel.es6.js & /core/modules/toolbar/js/models/ToolbarModel.js
   • isFixed & isViewportOverflowConstrained properties defined for the toolbar in this model
   • The above properties are only used to decide when to make the toolbar fixed
   • Now that the toolbar is fixed for all screen devices, from mobiles to wider screens, we are removing those properties, the javascript dependencies and conditions for fixed toolbar
3. /core/modules/toolbar/js/toolbar.es6.js & /core/modules/toolbar/js/toolbar.js
   • In this file, we are 1) triggering the event for change in isFixed value and 2) setting the value for isFixed on toolbar model for standard devices
   • We will not be requiring it as the toolbar is now always fixed
4. /core/modules/toolbar/js/toolbar.menu.es6.js & /core/modules/toolbar/js/toolbar.menu.js
   • We have a check for setting active tab if toolbar position fixed
   • With this patch, we are now removing that check as toolbar is always fixed and isFixed property has been removed from the model
5. /core/modules/toolbar/js/views/BodyVisualView.es6.js & /core/modules/toolbar/js/views/BodyVisualView.js
   • We are removing the listener and handler function to the change event for isFixed and isViewportOverflowConstrained value to add toolbar-fixed class on body element
   • Instead we are now permanently adding the toolbar fixed class on the body element (updated isToolbarFixed method to setToolbarFixed)

Basically we are removing all occurrences of isFixed and isViewportOverflowConstrained as they are used only to decide when to make toolbar fixed.

credits from duplicate issue

Adding credit for duplicate issues I just closed.

Tested the patch given in #44 on 8.9.x, the toolbar is fixed on mobile devices while scrolling the page. However, the length of the toolbar touches or going down to the footer of the page.

Also, I tried to apply the same patch on 9.1.x-dev but its getting failed. Please refer to the screenshot attachd for detailed insight

Need reroll for 9.1.x

Adding credit for cat-hearding

reroll

+++ b/core/modules/toolbar/js/toolbar.menu.es6.js
@@ -70,14 +70,12 @@
-      if (!Drupal.toolbar.models.toolbarModel.get('isFixed')) {
-        Drupal.toolbar.models.toolbarModel.set('activeTab', null);
-      }
+      Drupal.toolbar.models.toolbarModel.set('activeTab', null);

This is an issue.

When on desktop and you force the menu to be vertical, clicking on a menu link closes the tray. Before the tray would stay open on the next page when there is enough width space. We need something like "isFixed", but with a better name.

@nod_ thanks for the patch. Tested and applied the patch given in #70 on 9.1.x-dev. The issue with the toolbar on devices while scrolling the page is fixed and working fine on my end. Please find the screenshots attached.

Also, the similar issue with menu stays open on devices(having limited width space) is already reported in comment #10 on the issue: https://www.drupal.org/project/drupal/issues/3097856

+++ b/core/modules/toolbar/js/views/BodyVisualView.es6.js
@@ -15,22 +15,8 @@
-        this.listenTo(
-          this.model,
-          'change:isFixed change:isViewportOverflowConstrained',
-          this.isToolbarFixed,
-        );

To change the API doesn't look right.

I believed #36 still works. (That part exists for less hard-coded style (MQ) I bet) (In general case, to toggle a HTML class is the right way)

some codes removing Backbone style. I don't like it but doing that before #3145958: [META] Re-evaluate use of Backbone.js in core doesn't look right also :)

A combine with https://www.drupal.org/project/body_scroll_lock is needed

yeah making #70 started to feel like it was too much. You're right droplet.

#36 had an issue on narrow screens, for this to work need to change what it means to have the .toolbar-fixed class on the body.

Tested in seven, did not test on claro.

I'd like to see how this plays with text resizing, especially user-specified text sizes on mobile browsers.

Fixed headers sometimes cause problems when users increase the text size; the header ends up occupying too much of the viewport, leaving less room for the content the user is actually trying to focus on.

Verified and tested by applying the patch #73.It looks good to me.Can be moved to RTBC.

It's possible for toolbars to become much larger than what is seen in the earlier screenshots. On a narrower viewport, it's possible for the toolbar to consume a significant amount of the available space on the screen. For toolbars such as mine, I'd only have half the screen for content and wouldn't be able to geet more space by scrolling away.

If this was a brand-new feature, I could see the merits in arguing that this was an acceptable tradeoff for sites that opt to have lengthy toolbars. However, in this case, there may be existing sites with many toolbar items that are accustomed to this behavior. Implementing the solution in its current state could be a regression, and could limit the available non-toolbar space on these sites - potentially preventing them from accessing the page content altogether.

We should ask: Does the toolbar support unlimited items first?

If NO, then make it fixed. (But, see below)
If YES, then found a solution (, because no matter fixed or not, that's a problem)

In fact, I doubt users can't find the toolbar. I agree a pinned toolbar is provided a quick way to perform actions (eg. FB.com). But without a pinned toolbar is an extremely common pattern around the net, d.org also the same. ( I wouldn't say common is always good. We need to take a balance since we can't pin every actions)

"Failed to find the toolbar" vs "Failed go back to Admin area". To me, this is two different concepts.

Sometimes the UX testing is designed and guides the tester to fail. For example, after creating a new node, you asked "How to go back?". As a long time user, I may fail too. But if you asked "How to edit it?". I will find the edit button. The further problem is "Pinned toolbar" vs "Pinned EDIT"

Another problem, they may found the "Main nav" menu instead.

Does the toolbar support unlimited items first?

yes 🤦‍♂️.

If YES, then found a solution (, because no matter fixed or not, that's a problem)

I agree. This is a problem for not just situation such as the one I pointed out in #78 , but has other impacts such as the ones you elaborated on here: #2958478: Toolbar height calculation is faulty in multiple cases.

Since a JavaScript maintainer agrees this is a concern, I'm comfortable switching this to Needs Work. This issue could be rescoped to include figuring out how to deal with excess toolbar items, or a followup issue could be created, and this would be postponed until that was completed.

Needs to tag the UI team I think (, if there's a UI/UX team for the design)

At the moment there are three menu icons available in the toolbar: "menu", "shortcuts" and "profil". In case there are one to x custom helper modules installed like @bnjmnm illustrated in #78 would it be a reasonable step to introduce a fourth icon wrapping and collapsing all registered custom helper modules into for viewports smaller 610px? That way it might be possible to keep the single row toolbar as well as still add the position:fixed for viewports smaller 610px.

@rkoller I think that will be a good idea to make the UI clean.
Also in some websites the toolbar is hidden when you scroll down and it appears when you scroll up a little, that can also be implemented for the mobile view and even the desktop too.

Applied patch #73 applied successfully in durpal-9.3.x-dev
and looks good for me
Thanks for the patch
for ref sharing screenshots ....

Since several bugs have been marked duplicate of this, I'm going to reclassify as a bug here too.

I've also run into #2516938-44: Set the toolbar to position fixed on mobile several times personally which is definitely a functional bug.

Started an MR which includes the beginning of a solution for excess toolbar tabs. There are steps to do before it is review ready, but the underlying functionality is now there.

Applied patch #73 on Drupal 9 PHP version 8.1. I am attaching my screenshots for reference. Please review & thank you.

3809.patch has been successfully applied on Drupal 10.1
Now toolbar is fixed and working as expected in mobile.
For ref attached recording.

There are already more than enough screenshots and recordings - it's not necessary to provide more such as #96 and #97. These are demonstrating something that has already been demonstrated 7 times prior, so it does not move the issue forward and will not receive contribution credit.

A significant feature was added in #92 reviewing that addition might even warrant screenshots or videos. However but neither of the above have the toolbar configured to include tabs that don't open menu trays, so the end result is just screenshots of the exact same functionality that was screenshotted in #87, #76, #71, #66, #44, #23, #21.

Tested the toolbar and the excess toolbar items toggle manually with different phone sizes and window sizes and looks good.

Since there's no explanation of the near instantaneous switch from RTBC to "Needs review" in #101 I'm guessing this was an accidental submit?

Switching the status back to RTBC since #101 appears to be accidental (either accidentally switching status, or accidentally omitting the explanation for the status change)

Asked few questions on the MR

This it would help to have before/after screenshots in the issue summary to show what is being fixed.

Moving back for the screenshots

Attached the screenshots of how it looks before and after the changes.

Looks like the styling on the toggle when there are excess items is gone now 😅 Attached video here

When we try to add multiple toolbar items and use the mobile view, ideally the items should collapse and on clicking the ...it should expand but it's not happening here.And even the click on ...just updates the HTML element via ajax but it does not perform any action on click of it.I have attached the screenshot for the problem that i described.

Only open thread I see

I think this would benefit from a comment and also potentially from additional test coverage 😇

I reviewed the test in testCollapsibleToolbarItems() and it appears to pass test.
Note: DrupalCon2023, bug smash initiative.

Tested this manually at DrupalCon and the toolbar does stay fixed.

But adding extra tabs the "..." appears but clicking on it does nothing and when using enough tabs they wrap.

Clicking "..." works for me (enabled Workspaces module, it toggles workspace switcher).
However I found an issue related to z-index when manage icon is clicked.

The toolbar stays fixed and the toggle works and looks as expected. Pushed a small commit that just changes the selector of the toggle in a test. #119 was discussed today with lauriii and bnjmnm and the z-index issue is pre-existing and out of scope of this MR.

There's one kind of a major issue with this which is that when there's horizontal scroll on the page, e.g. when there's a table on the page, the toggle is pushed to the right side of the screen. This makes it very hard to notice the toggle. Ideally we would do #3068696: Tables overflow on mobile first but maybe there's a workaround that we could use here before that?

Tested manually and code looks good to me

Tested the scenario mentioned in #74. There's still a ultra-narrow mode which kicks in when zoomed in. Toolbar is not using rem or em units meaning that overriding body/html font-size won't impact toolbar. Pretty sure we can remove the needs accessibility review tag.

Posted few comments on the MR.

Appears the review was done by lauriii and the feedback was addressed.

Applied the MR myself and didn't notice any regression.

Left couple more comments in the MR.

I again tested the above problem that @laurii mentioned in Umami profile after removing the toolbar items but still i can't reproduce it.I'm attaching the screen recording for reference.

First, thanks all for working on this! I'd say the idea looked like a quick win before we get the new navigation, but I've tested the latest changes and I'm a bit concerned it's a bit too far to what I'd sign-off to get in.

1. It ends up in 2 lines of links (testing in FF at 390px wide) and that's too much much vertical space to leave a working area.
2. Apart from that, the z-index for icons gets in the way by showing icons in the middle of other submenus and it's confusing (I know it's a previous problem but it just makes it worst).
3. Another thing is that the open/close icon stays the same all the time, while when it's opened should be something else.
4. When the links open they are one next to the other (I know this is the current behavior) but it's a really strange and unseen pattern, specially interacting with the right "collapse" button making the items float to the left). If something is being changed from the current interaction it should make it better, and right now the result is confusing.

I'm afraid the place where this is right now will need a substantial amount of hours to design&rethink a few things, develop and review and I wonder if it's really worth the time of core experts here when we're planning on replacing this as soon as we can with the new Toolbar.

Hello, I stumbled on this for the issue depicted in #108.
We have a sticky header that relies on Drupal displace - top: var(--drupal-displace-offset-top, 0); - which is not positioned correctly on mobile with the toolbar.

Reading #131 is not really reassuring.
Can a compromize be found here maybe?

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.