The highlighting of the active menu-link does not respect query strings and fragment identifiers

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Added patch for 10.1.x. please review

Fixed CCF, attached interdiff for same.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

This line needs parenthesis to clarify the logic:

if (!isset($query) || empty($query) || $node->getAttribute('data-drupal-link-query') !== Json::encode($query) && !empty($node->getAttribute('data-drupal-link-query'))) {

But really what it needs is simplification IMO, maybe something like:

$attribute = $node->getAttribute('data-drupal-link-query');
if ($attribute && attribute != Json::encode($query ?? [])) {

Try to fix error.

Having a merge request and the patch is confusing. I've tried to sort out which is newer, I think that the patch is newer. The merge of 9.3.x by @Juan and the merge of 9.5.x by me complicate the history. I suggest that we start over with the current patch 72.

The attached patch

• is based on 2845319-72.patch
• is updated it to use es6, and has a complied version of the js changes
• rolled back the logic changes in ActiveLinkResponseFilter.php because the current logic is clearly faulty and it's unclear why this was introduced in #54. I'm unclear if this was a fix or code cleanup. I'd appreciate @juanolalla's comments on that

In #54 patch I just re-rolled the merge request, I don't know who/when/why that logic changed in ActiveLinkResponseFilter.php at some point. As long as ActiveLinkResponseFilterTest.php is testing the logic well and the patch passes tests we are fine.

#73 patch is failing to apply. Does it target 10.1.x?

#73 is for 9.x

Here is a patch for 10.x (the primary change is [#3305487], but this needs review to ensure I resolved the conflicts properly).

added updated patch and fixed CCF of #75

fixed CCF #76

I'm uploading a new patch with just the changes to active-link.js. I think this is going to pass tests, and I'll explain why.

I'm removing the changes to ActiveLinkResponseFilterTest and UrlTest. They fail because there is no related implementation on php, which is what they are testing. They are testing the logic in ActiveLinkResponseFilter, that hasn't change at all. So it make no sense to change the testing logic without changing the code being tested.

So, as we have seen from the beginning, there are different places where the logic that states that a URL with query parameters is not the same as without or different query parameters in terms of marking links as "active", has been implemented, on the backend code, and with JavaScript. We are fixing this just in the JavaScript layer, and with that change, tests don't fail. That means that there is no JavaScript tests in core right now checking that the links shouldn't have the 'is-active' class when not matching query parameters.

So, to do this right, in my opinion, we should discuss if we are agree that the current logic is not right: all same url with different query parameters combinations (with the same language), should be treated as the same in terms of marking is as active.

Then, if we agree on that and this issue makes sense, we should create JavaScript testing cases for the new logic (there isn't any for the current logic at this level).

And then, we should rewrite the PHP code as well, and so for the tests managing that. Why do we have both PHP and JavaScript implementation. I would love to know.

#78 looks good - this just gets rid of a bit of repetition and concat() uses.

Patch #79 wasn't working, the is-active stopped appearing (and tests failed). JSON.stringify() was adding double quotes, so we ended up having double quotes within double quotes in the queryMatchSelector string variable.

Fixed in this patch #80. Adding interdiff.

The test form earlier patches was lost, and need to be restored.

@douggreen please see #78 🐛 The highlighting of the active menu-link does not respect query strings and fragment identifiers Needs review above, where I explained the problem I found with tests (tldr: wrong tests, js tests don't exist yet for this in core)

Just run into this issue as well. What about only checking the query arguments from data-drupal-link-query and only skip if any of those don´t match the current query, while ignoring any additional query arguments from the current query.

Tried to fix the CCF in #84.

The last submitted patch, 85: 2845319-85.patch, failed testing. View results →

Using patch #80 against 10.2.3 allows local task tabs to highlight as active if there is any added url query.

I have not tested what happens if the local task target url itself contains a query. In that case the behaviour should probably be to match the local task specific query parameter(s) and only highlight as active if the task url query parameters match, ignoring added query parameters. @casey is this what #84 is attempting?

Stopped working in 10.3 -- Re-roll of #85

Reroll for 10.4.x