Toolbar doesn't indicate active menu trail for pages not included in Toolbar

This feature is not working in other Drupal versions as well. For this we will have to enable the child links in toolbar as well.

Thanks @shailja179! That is indeed the current behavior. 😊 This issue is to address that bug. Hopefully we can find a way to indicate the active menu trail even when the child links are not in Toolbar.

The toolbar seems to be working as expected (The active trail is maintained through the menu even if we traverse deeper into feild settings) in vertical orientation but not in the horizontal one, they basically have different UI's so i'm not sure if the same behaviour should be expected anyways.

ToolbarTest.php is just copied over and i used it to debug initially, might delete it completely or convert into an actual test.

Still need to create an issue for broken breadcrumb under the Appearance tab.

@omkar.podey PR does have 4 unresolved threads please resolve them,

I'm unable to log in into gitlab as of now (just keeps redirecting me to the home page, when clicking signIn), hence the unresolved threads, but i have addressed all of it.

I mostly caused this behaviour so closed 🐛 Change in orientation doesn't load collapsible icons until page refresh. Closed: cannot reproduce and start fixing it here.

I'm not sure if I'm doing something wrong, but I tried to take the MR locally and it isn't changing the behavior at all. Wondering if some of the changes broke this for Drupal instances that are not running in a subdirectory?

That's weird, I am not running drupal in a subdirectory it does seem to be working for me.

Note that in addition to clearing Drupal caches I also ran sessionStorage.clear();localStorage.clear(); in the browser console while testing this just to be sure. Works for me. Pushed a small clean-up

Tested this locally, I can see the current active menu trail. Nice work here. 🙂

Test failure seems legit to issue.

It looks like #20 is because I'm using Umami which is a multilingual site that uses a path prefix for detecting languages.

The bold and underline indicating the active tab is hard to see in my opinion. I think this could benefit from changing the background color to highlight it as well.

Current MR:

Suggestion of highlighted tab (#f5f5f5 is used in the screenshot for the active tab):

i did try to change the highlight colour on the css file but it seemed to show no effect on my local, is there something that i'm missing?

#33 It looks like if on Claro, then claro/css/state/toolbar.menu.pcss.css is used so the bg color needed to be added there

Applied the MR on an Umami install
Went to /en/admin/structure/types/manage/article/fields
But "Content types" doesn't appear to be highlighted.

There a step I'm missing?

Maybe try

1. clearing caches first
2. restarting local server

#37 should work now

Retested MR 4366 and on admin/structure/types/manage/article/fields I see that the "Content types" link is highlighted as expected

Woo!

Uploading static patch for 11.x. Please ignore this and continue further work on MR.

Posted review on the MR

.

The problem right now is a line in night watch js test Drupal.toolbar.models.toolbarModel.set('orientation', 'vertical'); should have triggered

 Drupal.toolbar.models.toolbarModel.on(
          'change:activeTab change:orientation change:isOriented change:isTrayToggleVisible chan

in core/modules/toolbar/js/toolbar.js and which should have the toolbar in vertical state but it doesn't.

Confirmed with @ckrina that the background change this issue is introducing is acceptable.

Left a comment suggesting a small change so marking it back to needs work.

Thanks for the review @Utkarsh_33! Addressed feedback 😊

While reviewing I found a bug when the toolbar is expanded down Configuration-->Development the horizontal orientation button disappears and can't be scrolled down to but this can be a separate issue.

I think we should also test Horizontal->Vertical and vice-versa that tests, the state is maintained properly after orientation change.

Just a bit concerned about render() as we are always calling both this.renderVertical(); this.renderHorizontal(); but it does seem to work fine and the code is much simpler this way.

1. Can we get before vs after screenshots? 🙏 For the horizontal orientation as well as the vertical orientation. And in vertical orientation also for a top-level vs sub-level vs sub-sub-level item.

2. +++ b/core/modules/toolbar/js/toolbar.menu.js
   @@ -14,6 +14,34 @@
   +          const dataDrupalPath = `${basePath}${path}`.startsWith('/')
   

   This logic exists in two places — let's extract it into a helper function? 🙏

3. +++ b/core/modules/toolbar/js/toolbar.menu.js
   @@ -23,7 +51,7 @@
   -     * @param {jQuery} $item
   +     * @param {*|jQuery} $item
   

   Why this change? (Not yet explained on issue or MR AFAICT.)

4. +++ b/core/modules/toolbar/js/toolbar.menu.js
   @@ -161,16 +190,37 @@
   +            const dataDrupalPath = `${basePath}${path}`.startsWith('/')
   +              ? `${basePath}${path}`.substring(1)
   +              : `${basePath}${path}`;
   

   AFAICT the startsWith will always be true due to presence of the base path.

   Shouldn't this be

   path.startsWith('/')
   

   instead? 🤔

5. +++ b/core/modules/toolbar/js/views/MenuVisualView.js
   @@ -13,8 +13,20 @@
   +      /**
   +       * Renders the horizontal toolbar with the right focus.
   +       */
   +      renderHorizontal() {
   +        if ('drupalToolbarMenu' in $.fn) {
   +          this.$el.children('.toolbar-menu').drupalToolbarMenuHorizontal();
   +        }
          },
   

   🤔 The comment mentions "focus", but I don't see anything focus-related here?

6. +++ b/core/modules/toolbar/tests/src/FunctionalJavascript/ToolbarActiveTrailTest.php
   @@ -0,0 +1,76 @@
   +   * Tests that the active trail is maintained even when traversed deeper.
   

   👏

7. +++ b/core/modules/toolbar/toolbar.module
   @@ -350,6 +350,14 @@ function toolbar_preprocess_html(&$variables) {
   +  foreach ($links as $link) {
   +    $paths[] = $link->getUrl()->getInternalPath();
   +  }
   +  $variables['#attached']['drupalSettings']['breadcrumb_paths'] = $paths;
   

   Breadcrumbs can only ever refer to local/internal links! 👍

RE Comment#54 / 4

I think we are majorly concerned with the basePath rather than path which comes from a list of internal paths which is always relative I think, @hooroomoo thoughts?.

RE Comment#54 / 3 and RE Comment#54 / 5

It's no more the case, based on a previous commit.

Added screenshots to IS of before and after changes with both vertical and horizontal orientations for top level (/structure), sub level (/structure/types) , and sub sub level (/admin/structure/types/manage/article/fields)

I believe all the remaining feedback has been addressed or no longer applies. Tested and code looks good to me.

lauriii → credited catch → .

Posted feedback regarding caching + how we could handle custom breadcrumb builders more gracefully. Crediting @catch for discussing this in Slack.

It looks like there was a regression somewhere because I am not getting the active tab anymore when I tested locally. I think the tests also need to be updated since it didn't catch the regression.

According to manually testing with git bisect, https://git.drupalcode.org/project/drupal/-/merge_requests/4366/diffs?co... is the offending commit.

Reverting that fixes things, but I'm not sure why @omkar.podey added that.

And as @hooroomoo points out, we're definitely missing test coverage

Feedback has been addressed. Removing needs test tag. Looks good to me.

One more thread in the MR because we can probably get rid of the breadcrumb dependency with the latest solution 👍

Feedback has been addressed

lauriii → closed merge request !4366

Committed 3165269 and pushed to 11.x. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.