[already commited but followup for CR?] <front> menu links are missing active trail classes

I had the same problem. When change the menu linking for "Home" link from node/6 to the li lost it's active-trail that was used for styling. After applied the patch everything worked as it should.

Might not be the best solution to special case but I don't know if there is any other way.

I think is why using

menu_set_active_trail('my-menu','<front>')

Fail and make the menu block disappear.

I don't understand the tests than fail but we need to add some test for this patch anyway.

I have this problem and am wondering if the proposed patch is really a fix or does it fail in other places. If someone is watching this is this fixed or not?

TIA

Still have this issue in drupal 7.16

I'm also not getting children of in the secondary menu - maybe this is related. Sort of shocked here. Fighting this and the fact that "active" never shows up for "my account".

Workaround for the <front> links
http://drupal.org/node/1571058#comment-6788662

Patch in #8 works great, thanks @ttiurani!

As far I can tell, this would exist in Drupal 8 too? Also, #10 indicates there are problems with the patch...

See also possible related issue or duplicate: #1063278: Menu item linked to <front> doesn't know its active

I applied the patch v4 to 7.19, but it does't work

I'm using Nice menu. I can't combine Nice menu with Menu Block :(

Add this http://drupal.org/node/1571058#comment-6788662
Its works!

Alright, this takes the approach from the comment mentioned above and adds the active class attribute in the theme function - getting around the caching issues caused by the previous approach.

Attached patches for D8, and D7.

patch #20 works great!

Patch core at your own risk. Until core is updated appropriately, I like this solution better: http://drupal.org/node/1571058#comment-6788662

update to 7.22 and apply #20 - cool!

fix_missing_active_trail.patch queued for re-testing.

#20: core-fix_missing_active_trail-1578832-20.patch queued for re-testing.

As per #2126319: <strong>Strip HTML tags for issue titles in listings</strong>, HTML tags in issue titles are not entirely escaped on the listing page, which is breaking validity.

(and has been preventing http://drupalreleasedate.com from being able to parse the page in order to get a count of issues for some time now)

Patch in #20 still applies cleanly to 7.32

adding tag back.

Patch in #20 still applies cleanly to 7.39

This work-around almost works with Drupal 8.1.8...

1. Disable the default/standard 'Home' menu link on the 'Main navigation' menu:
   /admin/structure/menu/link/standard.front_page/edit
   
   
2. Add a new menu link to the 'Main navigation' menu:
   /admin/structure/menu/manage/main/add?destination=/admin/structure/menu/manage/main
   
   Menu link title: Home
   Link: /node

But /node is shown when you hover over the Home link, and /node will be exposed to search engines... neither of these are desirable.

I believe this is another symptom of the <front> or /node or / mess.

Harrumph, any chance of a fix getting into 8.x soon, or a functional patch for D8? :( :(

(Yes I know I'm not helping....... I'll look now and see if I feel capable enough to do something to help instead of just whining........)

Errrrr never mind! Apparently it "just works," we just had a config wrong (someone added the menu link via menu admin UI, and linked to the front page or node/1 -- as soon as I removed that link and instead edited node #1 and added the menu link from in there, it worked fine/no troubles).

Sorry and never mind and thanks!

Using superfish module has resolved it for me

I tried fixing this in the part that builds the menu tree, but time constraints on my project force me to take the template_preprocess_menu() route.

Shouldn't have changed the version. Sorry

Previous patch caused fatal errors with external urls,

Patch in #34 still applies cleanly to Drupal 7.59.

I can confirm that code from #46 works well. I use it as my theme preprocess until it will be tested and committed to core.

Tried patch #48 as described in #50

When I visit the frontpage. Not only the frontpage link < li > is in the active-trail. But also all siblings.

<ul class="nav navbar-nav">
  <li class="nav-item active">
    <a href="/" class="nav-link active is-active" data-drupal-link-system-path="<front>">Home</a>
  </li>
  <li class="nav-item active">
    <a href="/2" class="nav-link active" data-drupal-link-system-path="node/4">link2</a>
  </li>
  <li class="nav-item active">
    <a href="/3" class="nav-link active" data-drupal-link-system-path="node/5">link3</a>
  </li>
  <li class="nav-item active">
    <a href="/4" class="nav-link active" data-drupal-link-system-path="node/6">link 4</a>
  </li>
</ul>

Improved patch 46:

• Use PathMatcher->isFrontPage() to check if preprocess is needed.
• Add extra instanceof Url checks to make sure no WSOD occur.
• Check if the menu item Url object isRouted instead of isExternal, as only isExternal could still throw an exception if an invalid internal route is in the menu items.

This version is working for me.

Sorry lukassykora but your patch is just a less defensive version of my patch and it failed the tests, so changing it back to review on my patch.

The patch in 52 seems to make *all* links on the front page have the "active-trail" class instead of just links to the front page :(

i found a work-around for this issue, just create a disabled menu item for the true home page (e.g. /node/1) under home link.

The patch in #52 applied cleanly on 8.7.8 and it worked for me. I cannot confirm the side effect mentioned in #57 - every menu link looks like it has the appropriate classes and only links representing the current page show as is-active.

Added an automated test for 8.8 and 8.9 to the patch #52.

Hopefully this can be committed soon.

I think #60 is missing a patch. This issue needs an automated test in order to be committed.

@Martijn de Wit, can you please dig up the automated test that you wrote and somehow failed to upload to this issue?

patch #52 still works great! Just tested it manually.

@Martijn de Wit, could you please find that automated test you wrote 5 months ago and somehow failed to upload?

Had to reroll patch 52 for Drupal 9.0.x

So to simplify I up uploading two patches, one for 9.0.x and the other for 8.9.x/8.8.x/8.7.x

patch 8 is for D7, still applies cleanly, I'll split that into a new seperate D7 issue

For the D7 issue:
#3119861: [D7] Class active-trail not added to li element when linking to front page

Active class missing (illustration)

Active After Applying Active Class

no, sry

after a few weeks of testing, patch 64 has regressions, it caused more than one link to have the active trail classes on the same page

I backed off the patch and removed it from our build.

As a workaround , our links for both of our languages are on one menu, internal links are fine, for external links we're using a workaround with css using a contrib module which allows specifying a class to the link. There's other ways to work around the external links I've used this method and another is to use also html lang attribute then the a element css selector on the href attribute based on a pattern to hide or show the opposite language external link.

Using one menu for both languages our active trail works fine. I'm not sure about the site default page issue haven't noticed it yet.

Anyhow, the patch 64 was derived from the others and I backed it off, no longer using it, doing a workaround instead.

previously we had menus for the default language and another different menu with another language and used block visibility on language for this, but this method proved to break the active trail for the 'other' language. So workaround is to use one menu for both our languages as I mentioned in this comment.

hope this helps.

We should fix this bug in D8 too, so filing against 8.8.x (which is the current bugfix support branch). The issue will be automatically updated to 8.9.x after the last 8.8.x bugfix release. Thanks for juggling all the different patches for this fix!

Just applied patch 64 before reading comment 69... Can confirm 64 adds the in_active_trail too generously 😉

Ahh so the problem for me is that the frontpage is a node (node/1). So:

$validHomepageRoutes = ['', $homepageUrl->getRouteName()];

This contains and "entity.node.canonical" so every menu item that is front or a node gets flagged.

This being not fixed leaves us wanting to fix this.

challenges:

• keep active trail functionality working for all links even if they are in another language
• ensure wcag keyboard navigation is not broken by our workaround, which we wouldn't have to do if this issue was fixed.
• wcag keyboard navigation fix is required because hidden links in a menu are not tabbable and since we display:none the other language link using an eng-only and fra-only / esp-only / jap-only class that goes something like html['lang='fr'] .eng-only { display:none; }
• this breaks wcag , so go further into javascript now, and remove the .eng-only elements something like $(html['lang='fr']).find('.eng-only').remove(); using jQuery (actual code we're using is not exactly this but just for an idea).
• By removing the display:none element from the html dom, the keyboard tab works and keyboard menu navigation works hence we can conserve our wcag compliance and accessibility

rather than do these workarounds, we would like to add a menu block per language allowing us to have unique weights for menu links and not have to fuss about external link translation for the url which is not always the same , often each language has an external link equivalient but for whatever reason (maybe explained above) this breaks the active-trail likely due to the bug mentionned above.

Currently we're working around this issue by using the https://www.drupal.org/project/menu_link_attributes menu_link_attributes module to provide a class attribute for links. We put this together on the same menu for both languages instead of a menu for each language (weights is another issue)

We want to make sure to maintain active trail functionality in both languages, there's the active class that indicates that the current link is what we're presently on, this is part of the menu functionality we didn't want to break and also keyboard navigation, due to wanting active trail to work in both languages we used one menu for two languages instead of two menus due to us wanting to work with what works for active trail menu links, one menu, links of all languages, internal links are no problem as they have a label that is translated and the internal link is automatically generated however external links like this one required a change in implementation to both the javascript and css, so we put a eng-only class on the english link that puts display: none on for the english link in french and a fra-only class on the french external link and there's another class attribute also, Drupal will render both links , css hides the other language external link, HOWEVER this does not fully allow wcag compliance in my recent wcag audits, SO we wrote some javascript to remove the other language link after the page is ready before the user navigates with the keyboard so when the menu link is configured as such using the specified class the javascript will also kick in and actually remove the other language link from the front end display which allows keyboard navigation to function correctly (wcag compliance, keyboard navigation is required). So the page renders, the css hides the other language link so it is never visible, then the javascript kicks in and removes the other language link from the html and thus we are able to pass wcag keyboard navigation and use the keyboard and tab key as expected.

it could be easier!

I'm going to try and address this in MenuTreeStorage::loadByRoute. Reasoning for this:

Assume you have the front page set as /node/1234. Also assume you have a menu named main and that menu has a single link which uses <front>.

Given the above, currently if you were to attempt to invoke MenuTreeStorage::loadByRoute('entity.node.canonical', ['node' => 1234], 'main'), you would get no results, even though that node is the front page.

I think the best course of action here is for the service to be aware of whether or not the passed route name/parameters represents the front page, and if it does then make the 'route_name' portion of the query part of an OR condition, where that condition also checks for <front>.

EDIT: I realize I didn't explain why this actually solves this issue. Which menu items get flagged as active comes from Drupal's menu.active_trail service. That service uses the current page's route match to get the page's route name and parameters, and invokes the aforementioned function to get the active link, and then it gets that links parents to build the rest of the trail.

The current page's route match will never, ever be the <front> route, it will always be what the actual route object is for that page in the system. So, we have to help MenuTreeStorage understand what the front page is.

Opened a MR with a somewhat rough implementation of where I think this needs to go. My local testing has been positive so far, and it appears to fix this issue in its entirety. I am not sure what kind of performance hit this adds. Still needs automated tests.

Yeah don't use the above branch for anything yet, lots of failed tests relating to cyclic dependencies, I'm hoping to be able to dig into it later today.

This is the circular reference error thrown that is mentioned by @Ipeabody:

Circular reference detected for service "plugin.manager.menu.link", path: "plugin.manager.menu.link -> router.no_access_checks -> route_enhancer.param_conversion -> paramconverter_manager -> drupal.proxy_original_service.paramconverter.menu_link".

I've previously also noticed some weird behavior in the active trail when the homepage contained a query string (example.org?foo=bar). Weirdest that I fgured out is that there is also JavaScript involved. If you disable Javascript the front page never gets an active trail class.

Linking as relevant issues.

The following workaround code was adapted from #64. It corrects the problem of flagging all routes of the same name by also comparing the route parameters.

/**
 * Prepares variables for block__system_menu_block templates.
 *
 * Work around an issue where front page menu links are not set in the
 * active trail.
 *
 * Default template: block.html.twig.
 *
 * @see https://www.drupal.org/project/drupal/issues/1578832
 */
function my_theme_preprocess_block__system_menu_block(&$variables) {
  if (!\Drupal::service('path.matcher')->isFrontPage()) {
    return;
  }

  $homepage_path = \Drupal::config('system.site')->get('page.front');
  $homepage_url = \Drupal::service('path.validator')->getUrlIfValid($homepage_path);
  if (!$homepage_url instanceof Url) {
    return;
  }

  foreach ($variables['content']['#items'] as $key => $item) {
    /** @var \Drupal\Core\Url $url */
    $url = $item['url'];
    if (!($url instanceof Url) || !$url->isRouted()) {
      continue;
    }

    $is_front = $url->getRouteName() == '<front>';
    $is_configured_route = $url->getRouteName() == $homepage_url->getRouteName() &&
      $url->getRouteParameters() == $homepage_url->getRouteParameters();
    if ($is_front || $is_configured_route) {
      $variables['content']['#items'][$key]['in_active_trail'] = TRUE;
    }
  }
}

I disagree with the approach in the previous MR that made MenuTreeStorage::loadByRoute() load front page routes. It violates the single responsibility principle and will result in unexpected behavior for calling code.

My own approach is to add a check for the front page to the menu.active_trail service's getActiveLink() function. That function is responsible for determining whether a menu is in the trail or not. <front> is effectively an alias for the front page route. So if anything needs to compensate for these non-standard <front> links, that seems like the most appropriate function to do it.

That said, I was a little confused at first because the docblock for MenuActiveTrailInterface::getActiveLink() is misleading:

  /**
   * Fetches a menu link which matches the route name, parameters and menu name.
   *
   * @param string|null $menu_name
   *   (optional) The menu within which to find the active link. If omitted, all
   *   menus will be searched.
   *
   * @return \Drupal\Core\Menu\MenuLinkInterface|null
   *   The menu link for the given route name, parameters and menu, or NULL if
   *   there is no matching menu link or the current user cannot access the
   *   current page (i.e. we have a 403 response).
   */

There is no "given route" because you don't pass a route to the function. Instead, it gets the current route from the current_route_match service. There's some code smell going on here. I assert that the docblock in the interface is incorrect and needs to be changed. I updated it to say that it looks for a menu link for the currently active route, although even that can't be guaranteed by the interface. There's an argument that the docblock change should be a separate issue, but since we're altering the behavior of this function to look for "alias" links that should be active for the current route, then I argue that it's an appropriate change for this issue.

The lack of route injection for the function also made the test harder to write. I tried to make a kernel test, but eventually punted on that and made a functional test that checks for Olivero's active trail class. I don't like it, but I couldn't figure out how to do it any other way. If someone has suggestions, please let me know.

@dcam There seems to be a test has got broken. Line 126 of SystemMenuBlockTest.php seems root of the problem?:

  // This creates a tree with the following structure:
    // - 1
    // - 2
    //   - 3
    //     - 4
    // - 5
    //   - 7
    // - 6
    // - 8
    // With link 6 being the only external link.
    $links = [
      1 => MenuLinkMock::create(['id' => 'test.example1', 'route_name' => 'example1', 'title' => 'foo', 'parent' => '', 'weight' => 0]),
      2 => MenuLinkMock::create(['id' => 'test.example2', 'route_name' => 'example2', 'title' => 'bar', 'parent' => '', 'route_parameters' => ['foo' => 'bar'], 'weight' => 1]),
      3 => MenuLinkMock::create(['id' => 'test.example3', 'route_name' => 'example3', 'title' => 'baz', 'parent' => 'test.example2', 'weight' => 2]),
      4 => MenuLinkMock::create(['id' => 'test.example4', 'route_name' => 'example4', 'title' => 'qux', 'parent' => 'test.example3', 'weight' => 3]),
      5 => MenuLinkMock::create(['id' => 'test.example5', 'route_name' => 'example5', 'title' => 'title5', 'parent' => '', 'expanded' => TRUE, 'weight' => 4]),
      6 => MenuLinkMock::create(['id' => 'test.example6', 'route_name' => '', 'url' => 'https://www.drupal.org/', 'title' => 'bar_bar', 'parent' => '', 'weight' => 5]),
      7 => MenuLinkMock::create(['id' => 'test.example7', 'route_name' => 'example7', 'title' => 'title7', 'parent' => 'test.example5', 'weight' => 6]),
      8 => MenuLinkMock::create(['id' => 'test.example8', 'route_name' => 'example8', 'title' => 'title8', 'parent' => '', 'weight' => 7]),
    ];

Added tags including an issue summary update: the current issue summary dates back to Drupal 7. Before and after screenshots for Drupal 11 are needed. The steps to reproduce could be simplified.

Another thing is, it states in the current Issue summary:

A better solution would be to get the 'in_active_trail' variable correctly set for links to , but unfortunately I could not find out how to do that.

This should be investigated.

There is now a failing test so test coverage is in place. There is currently one broken kernel test that needs to be fixed before the issue can be reviewed.

Have updated the Issue summary so it makes more sense in relation to Drupal 11.

There is more work to do. The screenshots need to be updated for Drupal 11 and any discrepancies with the existing D8 and D7 sections in the issue summary can be rectified.

To improve readability, the latest issue summary template should be used.

@dcam There seems to be a test has got broken.

Yeah. I saw that. But it was well past quitting time for the day, so I haven't tried to debug it. That's why I didn't set the issue status to Needs Review. Just from glancing at the backtrace it looked like the change to the service either broke the system menu block or maybe just the test. I'm not sure which.

@dcam Great work. This is a long-standing bug!

SystemMenuBlockTest was failing because it mocks all the routes. But when the menu links were being built for the block they went through the path.matcher service which checked for the existence of the routes. Since they didn't actually exist it threw exceptions. I mocked the service to fix the test.

@dcam Great!

Issue summary update

Left some comments on MR

Thanks

A kernel test needs to be created to verify the fix.

I’ve contributed my two cents and implemented it as a unit test, as the necessary mocking was straightforward, just like in the existing MenuActiveTrailTest unit test.

I've tested the MenuActiveTrail::getActiveLink() method, which should return the first <front> route menu link when the current page is the front page and corresponds to a route without its "own" links. This scenario occurs in a default fresh Drupal installation, where the home page is the view.frontpage.page_1 route and the main navigation menu has only a "Home" link provided by the system core module for the route <front>.

Super close, just 1 more comment.

That was a hard one! :)

Thanks! just needs a rebase now and think it's good to move up

Oops! I'm sorry, I was so focused on the test pipeline passing that I overlooked the merge conflicts :)

Thanks for the review!

Thanks, believe this is good to go now.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

Updated the target branch. Restoring RTBC status.

@dcam just re-ran the one failing test. Test that failed, error mentioned CKeditor. Cant see the connection?..

Pipeline green, test only test fails as it should.

Thanks @oily. I forgot to check on the test results after I rebased the MR to make certain it would still apply after all the recent commits to Core.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

I'm going to have to iteratively update the StandardPerformanceTest to account for the changes to the cache as a result of the bug fix. Because for some reason my local environment doesn't want to run FunctionalJavascript tests today so that I can do this in one commit.

I could help you out if you want. Let me know here or contact me in slack :)

All the tests are green again. I'm setting this back to Needs Review instead of RTBC only because this wasn't a simple rebase this time. I had to make adjustments to the expected values in the StandardPerformanceTest. They're probably innocuous, but someone should look them over.

The new numbers look reasonable to me

I rebased again due to a merge conflict caused by this morning's commits to Core. This was an easier rebase than the last one, so I'm leaving it at RTBC.

Marking as needs work, as this patch does not apply to 10.4.

That's normal @drupgirl, it has to apply to the last major development branch (11.x in this case)

You can make a backport and publish it here as a patch, for others.

Sorry for all the rebasing spam. I've been trying to keep this up to date despite all of the other recent commits. Practically every change to the performance test forces a rebase and update of this MR.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

I rebased the MR against 11.x again. Restoring RTBC status.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

Rebased again.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

Rebased. Again. Restoring RTBC.

Looks good to me but outside my scope

Looks good to me too but we need to do the deprecation dance in the constructor because there are existing contrib modules that extend this service.

I responded to all MR feedback and made the suggested changes. I don't have a good explanation for the typehint removals though.

Apparently PHP blows up with a fatal error if they're typehinted. See the results of https://git.drupalcode.org/issue/drupal-1578832/-/pipelines/476013. I don't really get why.

Oh I get it now - CacheCollector doesn't type hint the class, so they can't be constructor property promoted and also type hinted.

In this case what we need to do is remove the constructor property promotion, and add back the type hints.

Needs work for that but I think this is ready otherwise.

Recent commits trying to achieve #138. I think need to add back in 2 of the params following the same typehint-no-constructor-property-promotion pattern. Also need to figure out which params to put in parent and which to put in the current class constructors.

I'm daring to self-RTBC this because:

• The update was requested by a core committer who said the MR would probably be ready afterward.
• The update removed somewhat arbitrary code style changes in the MR.
• The update simplified the diff.

But since there are a lot of commits that have happened since #138 I figure that a good, old-fashioned interdiff wouldn't go amiss.

This needed yet another rebase due to performance test changes.

Left a comment on the MR regarding the deprecation link.

There's also a performance impact here, I've pinged catch about whether we need to fix that here or in a followup.

Discussed with @catch and he was ok with adding the 'load multiple' equivalents in a follow up
Can we get that issue added and update the code in MenuActiveTrail service to add a @todo pointing to the new issue?
That plus a change record (and update the link) and this can go back to RTBC, self RTBC is fine as its only comment/deprecation changes.

Thanks!

Can we get that issue added and update the code in MenuActiveTrail service to add a @todo pointing to the new issue?

Follow-up issue: https://www.drupal.org/project/drupal/issues/3523497

This needs to link to a change record rather than the nid of the issue

Change record: https://www.drupal.org/node/3523495

Thanks again!

Committed to 11.x
Published the change record.
Whilst this is a bug it is not eligible for backport because of the constructor BC change.

Thanks all, great to see this one finally resolved!

PSA: This broke menu_trail_by_path due to the constructor changes in a way that confused me a bit.

What that module does is alter the service like this:

$definition = $container->getDefinition('menu.active_trail');
    $definition->setClass('Drupal\menu_trail_by_path\MenuTrailByPathActiveTrail');
    $definition->addArgument(new Reference('path.validator'));
    $definition->addArgument(new Reference('router.request_context'));
    $definition->addArgument(new Reference('language_manager'));
    $definition->addArgument(new Reference('config.factory'));

So it gets the new argument added here but then its constructor doesn't expect that.

Don't see how this could do anything different, turns out that's a bad pattern for extending a constructor. Will switch to setters or autowire in #3524724: MenuActiveTrail::__construct() changes in Drupal 11.2 conflict with service alteration

The change record for this issue is a repeat of the deprecation message. It should include an example of before and after as well.

Were #148 and #149 addressed?

Assigning to myself to followup in a week or so

Since I originally authored the CR I took responsibility for updating it with the before/after examples. But I'm going to leave the issue metadata the same so @quietone can review my changes and address #148 if necessary.

@dcam, thanks for updating the CR. Providing extra detail, even for this straightforward deprecation, makes it clear what needs to be done.