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

One thing you could do is to try to write a test case. core/tests/Drupal/Tests/Core/Path/PathMatcherTest.php already contains a bunch of tests for this class.

Just to be sure, I first write a test that is supposed to fail, and then a patch so the test passes again right?

That would be ideal indeed.

Rerolled the patch, please review

Thanks @Neograph734 and @danylevskyi.

We do have the ability to write automated tests exercising JavaScript; see JavaScriptTestBase and https://www.drupal.org/docs/8/phpunit/phpunit-javascript-testing-tutorial. So, setting back to "Needs work" and re-tagging for an automated test to be written.

@Pavan B S, I've removed credit for the reroll in #14 because it was indicated in comments after #7 that it was no longer the correct approach. Please read the issue comments before submitting patches.

Adding to @Neograph734's comment, I'm not sure why it is implemented this way, but, adding an active class to link based on query parameters, seems a very rare case.

Also, patch in #23 resolves the issues for authenticated users only. For the anonymous user, "ActiveLinkResponseFilter" class is used to set "is-active" class to link.

I also want to add to this that I think it is fair to expect the active-link to not be query string dependent.

It's been two years since this has been touched from the looks of it. I just now have run into an issue that is resolved by #23. (authenticated user problem).

I'll be implementing the patch, but I'd much prefer about seeing a solution make it into core.

Re-rolled patch against 9.2.x.

Just fixing the custom command failure errors in #37, thanks!

Fixing the custom command failure errors, thanks.

Patch #39 worked for me on Drupal 9.1.7.

Links are now highlighted when accessing the page with a query string.

Changing status of the to RTBC.

Re #39, .js files should be generated by running the following commands:

$ cd core
$ yarn
$ yarn build:js

I am afraid this one would not get committed without tests added as this is a bug, tagging back Needs tests. Also tagging Bug Smash Initiative. The current active dev version is 9.3.x-dev.

Now is-active class is available after using ?foo=bar after the URL on the homepage.

I have added before and after patch screenshot for reference.

Moving to RTBC.

Updated IS

Committers do not monitor the "Needs committer feedback" tag, so no use adding it. :) (You probably figured that out since no one replied for years.) We monitor tags for our individual roles ("Needs release manager review", etc.), or the RTBC status.

This seems a reasonable change to me; however, we still need test coverage for it. (The manual testing and screenshots were helpful! But we also need an automated test.) It's also worth noting that (as described in #27), this sounds like it was working as documented. If we're changing the behavior to allow any query string to be highlighted, should we also update the docs?

Thanks!

Updating issue title.

This needs a reroll, core/modules/system/tests/src/Functional/Common/UrlTest has changed: "Merge blocked: fast-forward merge is not possible. To merge this request, first rebase locally."

Rerolled merge request. Attaching updated patch.

Patched 9.4.2 with 2845319-54.patch, looks good

Applied MR 866 on 9.5.x and it works fine.The is-active class is appearing even when the query strings are present, after applying this patch.

Before patch:

After patch:

I can also confirm that the patch in #54 works with 9.3.21. Thanks!

There are some suggested changes from @bnjmnm and also we will need a 10.x version of the MR because there are no longer .es6.js files and JS standards have been tweaked slightly no we don't have to support IE.

#54 reroll for 9.5.x.

FIxed CCF for #54 for 9.5.x.
Please review.

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 agreed 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 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.

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?