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.

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 don't think 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 #3376457: Change in orientation doesn't load collapsible icons until page refresh. and start fixing it here.

That's weird, I am not running drupal in a subdirectory it does seem to be working for me and other machines. Maybe just needs a drush cr.

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

Rebased and to apply patch locally.
Tested the same, I can see the current active menu trail. Nice work here. 🙂

Test failure seems legit to issue.

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.

.

The problem right now is a line in core/modules/toolbar/tests/src/Nightwatch/Tests/toolbarApiTest.js 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.

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

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. Expanded the test coverage to cover this in the previous commit.

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;
   

   🤔 It's possible to add external links anywhere in a menu.

   How will this behave in that case?

   Let's add explicit test coverage. Chances are a number of sites are using that, and we want to ensure there are no JS errors in that case!

   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 but the basePath is always absolute I guess so maybe we don't need the condition at all ?.

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.

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

Horizontal admin/structure/types on initial load not showing active tab

Vertical admin/structure/types/manage/article/fields not showing active tab

Horizontal admin/structure/types/manage/article/fields not showing active tab

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.

Feedback has been addressed