Problem/Motivation

When several menu links point at the same route and differ only by their query string, the active trail always resolves to the same menu link, no matter which of them the visitor actually followed. The wrong parent menu item is highlighted.

With the merge request from #2845319 applied, the correct child link receives is-active while the wrong parent is still highlighted, because in_active_trail is computed on a separate code path that the merge request does not touch.

Steps to reproduce

1. Create a menu structure similar to this:
-- Parent A > /all-topics
---- Child A > /all-topics?foo=1
-- Parent B > /node
---- Child B > /all-topics?foo=2
2. Visiting Child A will mark the link as is-active and the Parent A as active
3. Visiting Child B will mark the link as is-active and Parent A as active

Expected: Parent B should be marked as active when visiting Child B.
Actual: Parent A is marked active when visiting Child B.

Proposed resolution

When more than one menu link matches the current route, prefer the link whose query options are all present in the current request. Fall back to the current behaviour when none match.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#7 3618456-11.x-7.patch7.45 KBrobertragas

Issue fork drupal-3618456

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

robertragas created an issue. See original summary.

robertragas’s picture

Issue summary: View changes
robertragas’s picture

Test coverage added

robertragas’s picture

Pushed fixes for the code quality pipeline.

robertragas’s picture

Added more test coverage and a fix now.

Had to include the RequestStack as MenuActiveTrail needs the current query string so it can compare it against the query options a menu link declares. Nothing already injected exposes it I believe.

That means it would need to land in Drupal 12, and will create a patch for Drupal 11 with the deprecation notice. Not sure how that goes with the gitlab PRs if you want to create a PR for Drupal 11 too.

robertragas’s picture

StatusFileSize
new7.45 KB

Added a patch for Drupal 11 that includes a deprecation notice similar to PathMatcher. The Drupal 11 version in which this will be deprecated still needs to be determined, and a change record needs to be created. I have left those out for now, as I would first like to get feedback on the proposed solution and whether this is the right direction.

robertragas’s picture

Status: Active » Needs review
robertragas’s picture

Rebased

ribel’s picture

The approach looks correct as a follow-up to #2845319: The highlighting of the active links does not respect query strings and fragment identifiers: when several menu links share a route, MenuActiveTrail now prefers an enabled link whose query options are a subset of the current request (with unrelated params like pager still allowed), falls back to the previous first-enabled-link behaviour when nothing matches, and includes the query in getCid() so the trail cache varies correctly.

I also have tested patch #7 locally and it works as expected.

Thanks @robertragas!

sonikam’s picture

I have tested the fix. Selecting a particular child always points to it's own parent now

kingdutch’s picture

Status: Needs review » Reviewed & tested by the community

Marking as RTBC based on review by ribel and the testing by our QA specialist Sonika.

And congratulations to her on her first contribution to a Drupal.org issue 🎉