Problem/Motivation

1) On clean Drupal 7.12 install enable Content Translation module
2) Installed the second language (Latvian) (Default - English)
3) On admin/config/regional/language/configure enabled "Determine the language from the URL (Path prefix or domain)"
4) For the Basic Page content type - enabled Multilingual support -> Enabled, with translation
5) Create "Basic Page" node - and choose the language "English"
6) Save the node
7) Click on the node "Translate" tab
8) Click "add translation" link on opposit the non-default language (Latvian in my case). - The screen opens without the overlay!
When you click on the "edit" opposit the English (the default language) - it opens in the overlay!

Proposed resolution

Hope to get soon from the community ... :)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Version: 7.12 » 8.x-dev
Issue tags: +Needs backport to D7

Thanks @Nor4a for the clear report!

xjm’s picture

Title: Overlay does not work with prefixed URL paths 7.12 » Overlay does not work with prefixed URL paths

Here's the similar issue that was resolved last spring: #759844: Overlay does not work with prefixed URL paths.

nod_’s picture

Issue tags: +JavaScript

For those watching at home, confirmed in d8 and the issue is indeed that urls with language prefix are not recognized by Drupal.overlay.isAdminLink event if after the prefix they are indeed admin links.

It's either a tweak to the regxp, a monkey-patch or (from what I read in the other issue) rethinking the way urls are handled by drupal.

nod_’s picture

Status: Active » Needs review
FileSize
1.32 KB

wow, this sent me way way down the rabbit hole.

anyway, here is a fix, it's not pretty (only checking the end of the path, not the whole path) but hey it works for now. I changed stuff that was hurting my eye in the process. I know it won't go in like that but that's just for people to work with it, there is a bigger issue underneath anyway.

nod_’s picture

xjm’s picture

Issue tags: +Needs manual testing

Cool.

cosmicdreams’s picture

I'm in the process of manually testing this one, thanks xjm for pointing me to it. I'll update this post with the result.

Before applying this patch: I can confirm this issue is happening (7.12)

Applied the patch...

Patch fixed the issue described in OP. Testing for regressions now...

Tested many of the typically troublesome pages (Dashboard, Reports > Recent log messages, Content) and I found no regressions.

cosmicdreams’s picture

Status: Needs review » Reviewed & tested by the community

Marking as RTBC, this one's ready to commit IMO.

xjm’s picture

Issue tags: -Needs manual testing

Excellent; thanks @cosmicdreams!

catch’s picture

Version: 8.x-dev » 7.x-dev
Component: translation.module » overlay.module
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +Novice

Thanks! Committed/pushed to 8.x, this should be an easy re-roll for 7.x so tagging as novice for the backport.

Purencool’s picture

Assigned: Unassigned » Purencool
Purencool’s picture

This patch has been tested in 7.x and was correct

Purencool’s picture

Status: Patch (to be ported) » Needs review

patch above needs review

KrisBulman’s picture

Status: Needs review » Needs work
FileSize
99.92 KB
270.68 KB

I'm still experiencing the problem after the patch & clear cache in d7

$ git apply -v core-js-overlay-lang-prefix-1431076-4-version-2.patch
Checking patch modules/overlay/overlay-parent.js...
Applied patch modules/overlay/overlay-parent.js cleanly.

Can someone else please test and confirm

nod_’s picture

Have you cleared you browser's cache too? the iframe gets in the way.

nod_’s picture

Status: Needs work » Needs review
KrisBulman’s picture

Status: Needs review » Needs work

I get the same results, here..

my tests use the exact steps as listed in the original ticket..

$ drush cc all
* cleared browser caches
** tried this in these browsers: chrome, safari, firefox 10

Also added multiple other languages (ltr & rtl), and get the same effect (does not appear in overlay) when clicking 'add translation' from those either (after clicking translate on the node)

needs further testing by others to rule out a local development environment issue.

c960657’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs work » Needs review
FileSize
2.36 KB

anyway, here is a fix, it's not pretty (only checking the end of the path, not the whole path) but hey it works for now.

Not quite. Doing a substring search only works when you are on the default language (where there is not path prefix) and the link is to a page that has a path prefix.

I wrapped up a patch that checks for enabled path prefixes. Now we no longer used Drupal.settings.pathPrefix that was added in order to support path prefixes in Overlay (see #759844: Overlay does not work with prefixed URL paths), but it seems like a useful addition, so I have left that in.

-  if (href != undefined && href != '' && target.protocol.match(/^https?\:/)) {
+  if (typeof href !== 'undefined' && href !== '' && (/^https?\:/).test(target.protocol)) {

What was the reason for this change? It isn't mentioned above and seems unrelated to the path prefixing stuff.

Status: Needs review » Needs work
Issue tags: -JavaScript, -Novice, -Needs backport to D7

The last submitted patch, overlay-path-prefix-1.patch, failed testing.

c960657’s picture

Status: Needs work » Needs review

#18: overlay-path-prefix-1.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +JavaScript, +Novice, +Needs backport to D7

The last submitted patch, overlay-path-prefix-1.patch, failed testing.

c960657’s picture

Status: Needs work » Needs review
FileSize
2.39 KB

Only check for prefixes when Locale module is enabled.

ryan.gibson’s picture

#22: overlay-path-prefix-2.patch queued for re-testing.

ryan.gibson’s picture

I experienced the behavior mentioned without the patch.

After applying the patch to a clean Drupal install, the overlay displayed perfect on step 8. :)

xjm’s picture

@ryanissamson: Excellent. Which browsers did you test in?

ryan.gibson’s picture

@xjm thanks for asking because I had forgotten - I originally tested in safari and chrome - the patch was successful.

I just tested in FF and on the final step, the error was present, the overlay disappeared.

xjm’s picture

Hm, did you make sure to clear the site cache after applying the patch? If so, let's have one more person confirm whether FF is broken (and mark the patch NW if so).

ryan.gibson’s picture

Yea, I thought the same thing, but after clearing site and browser cache it still didn't work. I am going to run the test one more time myself to be sure.

ryan.gibson’s picture

Okay, so after retesting - I apparently made a mistake - FF is working fine now.

So, in Webkit and FF the patch is good; the overlay display perfect when reproducing the steps.

Tor Arne Thune’s picture

Status: Needs review » Reviewed & tested by the community

I can confirm the results of ryanissamson. Did the steps to reproduce with the patch applied on Drupal 8 in the following browsers:

Chromium 17 on Linux
Firefox 10 on Linux
Opera 11 on Linux
Internet Explorer 8
Internet Explorer 9

All tries opened in the overlay!

KrisBulman’s picture

applied D8 patch & tested steps in FF10, Chrome, Safari 5.1.3 (OS X), IE8 (Windows): overlay stays active

This patch will need to be backported to d7

webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Excellent! Thanks for all the through testing, folks!

Committed and pushed to 8.x. This needs a re-roll for 7.x.

c960657’s picture

FileSize
2.48 KB

Backported to D7. The patch differs a bit, because locale_language_negotiation_url_prefixes() is D8-only (introduced in #1301460: Decouple domain/path language negotiation storage from language config storage), and the LANGUAGE_NEGOTIATION_URL_PREFIX constant was renamed for D8 (#1431040: Rename LOCALE_LANGUAGE_NEGOTIATION_* constants to LANGUAGE_NEGOTIATION_*).

BTW, I noticed that Drupal.settings.pathPrefix contains e.g. "de/" (with a trailing slash), but Drupal.settings.pathPrefixes (introduced in this ticket) contains e.g. ["da", "de"] (without trailing slashes). Both variants seem to be used in core. Also, they seem to be referred to with the less accurate url_prefixes. Do you think we should do anything about it?

c960657’s picture

Status: Patch (to be ported) » Needs review
KrisBulman’s picture

Status: Needs review » Reviewed & tested by the community

ok, tested this patch in d7

patch applied cleanly, i had to fully clear the cache of all browsers i tested in order for it to work

browsers tested:

Chromium 17 on OSX
Firefox 10 on OSX
Safari 5.1.3 on OSX
Internet Explorer 8

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome, thanks.

Ideally for D7 we'd have IE7 and IE6 testing, as well. However, this patch seems to be only moving JS code around, not changing it at all, so it shouldn't make Overlay support in those browsers any buggier than what they already are.

Committed and pushed to 7.x. Thanks! Another major bites the dust. :D

KrisBulman’s picture

Oops! I guess I had D8 on the brain. Even though this is committed, I put it through the ringer on IE6 & IE7 as well & they passed the test!

webchick’s picture

Oh, perfect then! Thanks, Kris! :)

Automatically closed -- issue fixed for 2 weeks with no activity.

Status: Closed (fixed) » Needs review

Status: Needs review » Needs work

The last submitted patch, 33: overlay-path-prefix-3.patch, failed testing.

Tor Arne Thune’s picture

Issue summary: View changes
Status: Needs work » Closed (fixed)