Active language in language switcher does not have 'is-active' class if query string present in URL, e.g. pager ?page=0.

CommentFileSizeAuthor
#41 fixed-language-code-for-switcher-v1.patch1008 bytesRemco Hoeneveld
#40 fixed-language-code-for-switcher-v1.patch989 bytesRemco Hoeneveld
#35 active-language-links-2775651-d85-35.patch2.89 KBpsf_
#34 after-anchor-click.png140.84 KBGhostInTheMachines
#34 before-anchor-click.png133.74 KBGhostInTheMachines
#33 is-active-present-403-authenticated-2775651-33.png280.23 KBbighappyface
#33 is-active-missing-403-anonymous-2775651-33.png267.62 KBbighappyface
#33 is-active-missing-404-anonymous-2775651-33.png265.4 KBbighappyface
#33 is-active-missing-404-authenticated-2775651-33.png290.5 KBbighappyface
#33 is-active-present-node1-authenticated-2775651-33.png300.6 KBbighappyface
#33 is-active-present-node1-anonymous-2775651-33.png279.93 KBbighappyface
#29 inter_diff-2775651-22-29.txt746 bytesEleven Yu
#29 active-language-links-2775651-29.patch2.89 KBEleven Yu
#27 inter_diff-2775651-22-27.txt746 bytesEleven Yu
#27 active-language-links-2775651-27.patch2.89 KBEleven Yu
#22 inter_diff-2775651-17-22.txt737 bytessimbaw
#22 active-language-links-2775651-22.patch2.87 KBsimbaw
#20 active-language-links-2775651-20.patch2.79 KBsimbaw
#20 interdiff-2775651-17-20.txt646 bytessimbaw
#5 active-language-links-2775651-5.patch697 bytesdpacassi
#8 active-language-links-2775651-8.patch2.07 KBdpacassi
#8 interdiff-2775651-5-8.txt1.25 KBdpacassi
#10 2775651_active_state.png346.68 KBSteffenR
#17 active-language-links-2775651-17.patch2.14 KBigalafate
#17 interdiff-2775651-8-17.txt608 bytesigalafate
#18 interdiff-2775651-17-18.txt620 bytessimbaw
#18 active-language-links-2775651-18.patch2.74 KBsimbaw

Issue fork drupal-2775651

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

matulis created an issue. See original summary.

matulis’s picture

Title: No active language if pager used on a views page » No active language in language switcher for URL with query string
Issue summary: View changes
Issue tags: -views
matulis’s picture

Version: 8.1.1 » 8.1.7
rodrigoaguilera’s picture

Version: 8.1.7 » 8.2.x-dev

The fix should be for the latest stable branch.

This happens also in views with exposed filters that add parameters to the query

dpacassi’s picture

Status: Active » Needs review
Issue tags: +JavaScript, +Dublin2016
FileSize
697 bytes

I was able to reproduce the bug on Drupal 8.2.x-dev with the standard installation profile.
I've tracked down the error to the core's active-link.js file, which set's the is-active class for active links.

Currently, the JS only adds the is-active class if the URL in the link matches the current URL exactly (including URL parameters).
While that's for most links intended, language links should always be marked as is-active as long as they match the current language.

I've written a patch which covers this issue, feel free to review it.

nod_’s picture

Does the same issue happens when the user is anonymous?

There are 2 different script checking for the active link, one in JS and one in PHP. If the JS is bugged, the PHP most likely is too.

dpacassi’s picture

Status: Needs review » Needs work

Oh, nice input, I didn't know about the PHP part.
The issue still exists for anonymous users, I'll try to fix the PHP part as well.

dpacassi’s picture

Status: Needs work » Needs review
FileSize
2.07 KB
1.25 KB

Fixed the PHP part as well, see the attached patch and interdiff.

SteffenR’s picture

Assigned: Unassigned » SteffenR

I'll have a look at this issue / review it.

SteffenR’s picture

Assigned: SteffenR » Unassigned
Status: Needs review » Reviewed & tested by the community
FileSize
346.68 KB

I could reproduce the bug. After adding the patch i get the is-active class for anonymous and logged in users as well.
From my perspective it looks fine and could be set to RTBC.

Attached you find a screenshot, showing that its working like it should.

SteffenR

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: active-language-links-2775651-8.patch, failed testing.

SteffenR’s picture

Status: Needs work » Reviewed & tested by the community

Not sure why testbot resetted the status of the patch to "needs work" - all tests are green.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: active-language-links-2775651-8.patch, failed testing.

droplet’s picture

This isn't a bug but a new feature IMO. Or say this isn't a bug of active-link.js but the language switcher (or language system). There missing query string in language switcher link.

If page=1 changing the page content, language switcher redirects you to another content.

rodrigoaguilera’s picture

@droplet that is a good point.

If the language switcher renders the links with the current query parameters it will be marked as active and it will take you to a more "proper" page when you click and switch languages.

I think the patch should be reworked so it does that.

I still feel like it is a bug since having query parameters shouldn't make the user lose the language context like filtering a view or navigate to a different page

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

igalafate’s picture

On one side, I have applied the #8 patch but I could not get the "is-active" class in the "li" elements so I have added it to that patch version.

On the other side, I was trying to develop another patch following a totally different approach because I do not really like so much the orignial way the current path is dealt on the scripts where the active links are set (ActiveLinkResponseFilter.php and active-link.js). But I could not fix the anonymous user case so far.
My point is, when the links are created the data-drupal-link-system-path and the data-drupal-link-query attributes are populated with the internal current path info, but when both scripts try to set the active link, they use a different current path info. So for example, in the 404 case, the data-drupal-link-system-path="system/404" but the set active links scripts are looking for drupal-link-system-path=""

simbaw’s picture

I have applied the #17 patch but I could not get the "is-active" class on the 404 page when current user is anonymous so I have added it to that patch version.

The last submitted patch, 18: active-language-links-2775651-18.patch, failed testing.

simbaw’s picture

Fix javascript coding standards error for the #18 patch

Status: Needs review » Needs work

The last submitted patch, 20: active-language-links-2775651-20.patch, failed testing.

simbaw’s picture

Status: Needs work » Needs review
FileSize
2.87 KB
737 bytes

I have applied the #17 patch but I could not get the "is-active" class on the 404 page when current user is anonymous so I have added it to that patch version.(Fix code error for #21)

gbisht’s picture

Status: Needs review » Reviewed & tested by the community

Patch #22 seems to be working perfectly and fixes one more scenario where this type of parameter in url "&f[0]=price%3A0&f[1]=color%3A58" replicated the same scenario.

The last submitted patch, 17: active-language-links-2775651-17.patch, failed testing.

droplet’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs JavaScript testing

It seems the patch still not address #14.

And the code should put on the module provide `language switcher` rather than global active-link.js

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Eleven Yu’s picture

For #22 patch, it fixed 404 page, but 403 page have same issue, so I add a new patch version. This patch applies to 8.3.x.

Status: Needs review » Needs work

The last submitted patch, 27: active-language-links-2775651-27.patch, failed testing. View results

Eleven Yu’s picture

For Patch #22, it fixed the 404 page, but 403 page has the same issue. So I add a new patch, it's useful for 8.3.x.

thomas.frobieter’s picture

Can someone explain why this is done with JS instead of simply adding the active-class via twig / php like every ohter menu? Seems like a really bad idea to me.

nicrodgers’s picture

@thomas.frobieter it's to do with caching, I believe. If you're logged out, it works without JS. But logged-in users use the JS version.

joelpittet’s picture

Version: 8.3.x-dev » 8.5.x-dev
Status: Needs work » Needs review

Let's see where this is at with 8.5.

bighappyface’s picture

I am picking this up for Sprint Weekend 2018 @ ADUG.

Steps to reproduce:

1. Setup a bare installation of latest Drupal dev branch
2. Enable Language and Content Translation modules
3. Add a second language
4. Place the "Language switcher" block into the breadcrumb region
5. Create a node

Authenticated

1. Visit node URL, verify "is-active" class is present in the appropriate language switcher list item and anchor (working)

2. Visit a 404 URL, verify "is-active" class is missing from appropriate language switcher list item and anchor (broken)

3. Visit a 403 URL, verify "is-active" class is present in the appropriate language switcher list item and anchor (working)

Anonymous

1. Visit node URL, verify "is-active" class is present in the appropriate language switcher list item and anchor (working)

2. Visit a 404 URL, verify "is-active" class is missing from appropriate language switcher list item and anchor (broken)

3. Visit a 403 URL, verify "is-active" class is missing from appropriate language switcher list item and anchor (broken)

GhostInTheMachines’s picture

FileSize
133.74 KB
140.84 KB

1. Visit node URL, verify "is-active" class is present in the appropriate language switcher list item and anchor (working)

Clicking the "Spanish" anchor loads the page by es/node/1 rather than the alias I created /lang-switch-test

Is this the expected behavior for step one?

psf_’s picture

Updated patch to the last 8.5.x dev branch.

andypost’s picture

I'd separate bug with both links active on 404 pages & query string additions for normal pages

+++ b/core/modules/system/system.module
@@ -672,7 +672,8 @@ function system_page_attachments(array &$page) {
-  if (\Drupal::currentUser()->isAuthenticated()) {
+  $route_name = \Drupal::request()->attributes->get('_route');
+  if (\Drupal::currentUser()->isAuthenticated() || in_array($route_name, ['system.404', 'system.403'])) {

this could go separate issue & needs test

bighappyface’s picture

@andypost I agree that the 403/404 problems can go into a separate issue and I think #2802371: On a 404 page, none of the language switcher links have the 'is-active' class is that issue.

Also, it looks like #2968541: ActiveLinkResponseFilter fails to set active link with query in non-alphabetical order has solved the original problem in this issue related to query parameters.

I am closing this task as a duplicate now that I can't reproduce the original symptoms in the 8.6.x dev branch.

BlondeSwan’s picture

It's amazing to me that after 4 years this issue still exists. Drupal is a dumpster fire.

gbisht’s picture

@BlondeSwan issue is closed 2 years back as duplicate and fixed in https://www.drupal.org/project/drupal/issues/2968541
Feel free to open the issue if you can still replicate the issue in any scenario.

Remco Hoeneveld’s picture

@blondeSwan i still also had this issue when switching to /en or /nl quick fix is in my patch.

Remco Hoeneveld’s picture

FileSize
1008 bytes

Fixed the patch.

Anybody’s picture

As @quietone reported the issue to be still present in https://www.drupal.org/project/drupal/issues/2802371#comment-14964008 and the patch from #41 fixes it, we should reopen this.

It doesn't seem to be a duplicate, as otherwise it shouldn't be there since Drupal 8.6 where #2968541: ActiveLinkResponseFilter fails to set active link with query in non-alphabetical order was fixed.

Please also see #2802371: On a 404 page, none of the language switcher links have the 'is-active' class and check out, if both fixes are needed and solving different problems.

Anybody’s picture

Version: 8.5.x-dev » 10.1.x-dev

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

johnzzon made their first commit to this issue’s fork.

johnzzon’s picture

Also had this issue, we also saw it on views pages with no query string. Created MR with changes from #41 but rerolled to 11.x.

Seems to apply cleanly to 10.2.x as well and fixes the issue for us.

Anybody’s picture

Status: Active » Needs work

Thranks @johnzzon! I just verified the MR is equal to the patch and it is! Thanks for that on 11.x!

As it was confirmed several times to be working (also by me), I just left one question that's not clear to me code-wise, see the comment.

Also we need a simple test to ensure this works as expected and the class is there in the case, where it wasn't before. Thanks!