Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Active language in language switcher does not have 'is-active' class if query string present in URL, e.g. pager ?page=0
.
Comment | File | Size | Author |
---|---|---|---|
#41 | fixed-language-code-for-switcher-v1.patch | 1008 bytes | Remco Hoeneveld |
#35 | active-language-links-2775651-d85-35.patch | 2.89 KB | psf_ |
Issue fork drupal-2775651
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
Comment #2
matulis CreditAttribution: matulis commentedComment #3
matulis CreditAttribution: matulis commentedComment #4
rodrigoaguileraThe fix should be for the latest stable branch.
This happens also in views with exposed filters that add parameters to the query
Comment #5
dpacassiI 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 theis-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.
Comment #6
nod_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.
Comment #7
dpacassiOh, 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.
Comment #8
dpacassiFixed the PHP part as well, see the attached patch and interdiff.
Comment #9
SteffenRI'll have a look at this issue / review it.
Comment #10
SteffenRI 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
Comment #12
SteffenRNot sure why testbot resetted the status of the patch to "needs work" - all tests are green.
Comment #14
droplet CreditAttribution: droplet commentedThis 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.
Comment #15
rodrigoaguilera@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
Comment #17
igalafate CreditAttribution: igalafate at La Drupalera by Emergya commentedOn 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=""
Comment #18
simbaw CreditAttribution: simbaw commentedI 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.
Comment #20
simbaw CreditAttribution: simbaw commentedFix javascript coding standards error for the #18 patch
Comment #22
simbaw CreditAttribution: simbaw commentedI 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)
Comment #23
gbisht CreditAttribution: gbisht as a volunteer and at Acquia commentedPatch #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.
Comment #25
droplet CreditAttribution: droplet commentedIt seems the patch still not address #14.
And the code should put on the module provide `language switcher` rather than global active-link.js
Comment #27
Eleven Yu CreditAttribution: Eleven Yu commentedFor #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.
Comment #29
Eleven Yu CreditAttribution: Eleven Yu commentedFor 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.
Comment #30
thomas.frobieterCan 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.
Comment #31
nicrodgers@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.
Comment #32
joelpittetLet's see where this is at with 8.5.
Comment #33
bighappyface CreditAttribution: bighappyface as a volunteer commentedI 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)
Comment #34
GhostInTheMachines CreditAttribution: GhostInTheMachines as a volunteer commented1. 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?
Comment #35
psf_ CreditAttribution: psf_ at SDOS commentedUpdated patch to the last 8.5.x dev branch.
Comment #36
andypostI'd separate bug with both links active on 404 pages & query string additions for normal pages
this could go separate issue & needs test
Comment #37
bighappyface CreditAttribution: bighappyface as a volunteer commented@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.
Comment #38
BlondeSwan CreditAttribution: BlondeSwan commentedIt's amazing to me that after 4 years this issue still exists. Drupal is a dumpster fire.
Comment #39
gbisht CreditAttribution: gbisht as a volunteer and commented@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.
Comment #40
Remco Hoeneveld CreditAttribution: Remco Hoeneveld as a volunteer and at Atom commented@blondeSwan i still also had this issue when switching to /en or /nl quick fix is in my patch.
Comment #41
Remco Hoeneveld CreditAttribution: Remco Hoeneveld as a volunteer and at Atom commentedFixed the patch.
Comment #42
AnybodyAs @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.
Comment #43
AnybodyComment #47
johnzzonAlso 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.
Comment #48
AnybodyThranks @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!