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.

isaura’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)

gulab.bisht’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 JS 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