Hi :)

Problem

If I have a set-up with :

  • i18nmenu module
  • DHTML Menu module

And I create a menu such that :

  • Menu item M is set to 'All languages'
  • Menu item M has sub-menus
  • Menu item M has been localised using the translation interface into language L

Then:

  • When viewing in the default language, the menu item M expands/collapses properly using DHTML
  • When viewing in the language L, the menu item M causes a page reload when clicked on

Patch

Here is my attempt at understanding what is going on :

The problem is that by the time dhtml_menu_theme_menu_item is called, i18nmenu has already localised the terms. So it is not possible to use the title of the item to index the item (as done in _dhtml_menu_subtree).

This is a generic problem, and not just for i18nmenu -- any other module could also have modified the link's title/weight before it got to dhtml. So the patch I propose here (against 3.0 beta) uses only the mlid as index (hopefully that should never change !!!)

Index: dhtml_menu/dhtml_menu.module
===================================================================
--- dhtml_menu/dhtml_menu.module        (revision 1296)
+++ dhtml_menu/dhtml_menu.module        (working copy)
@@ -129,13 +129,14 @@
   reset($stack);
   $start = current($stack);
   $tree = menu_tree_all_data($start['menu_name']);
+  // Create an index of mlid
+  $tree_index = array();
+  foreach ($tree as $key => $item) {
+    $tree_index[$item['link']['mlid']] = $key;
+  }
+
   foreach ($stack as $item) {
-    // Generate the sortable array key of an item:
-    $path[] = (50000 + $item['weight']) .' '. $item['title'] .' '. $item['mlid'];
-  }
-
-  // Traverse the tree.
-  foreach ($path as $key) {
+    $key = $tree_index[$item['mlid']];
     if (!isset($tree[$key])) {
       $tree = $tree[key($tree)]['below'];
       if (!isset($tree[$key])) return array();
CommentFileSizeAuthor
#12 dhtml_menu_mlid_7-298720-12.patch3.17 KBcburschka
#10 dhtml_menu_63_mlid-298720-10.patch2.99 KBcburschka
#6 dhtml_menu_63_mlid-298720-6.patch2.74 KBcburschka
#4 dhtml_mlid_patch.txt831 bytesAlice Heaton

Comments

cburschka’s picture

cburschka
they
commented
Title: DHTML effect does not work with i18nmenu (report + patch) » DHTML effect does not work with i18nmenu

I've tried to work with mlid before, specifically when assigning unique ID attributes to links, and for some reason it turned out that not all items had an mlid set.

I'll check again, and if I find I can rely on the mlid, this would be a good change.

Meanwhile, please upload your patch as a file.

cburschka’s picture

cburschka
they
commented

Sorry, your fix breaks book navigation - for some reason the mlids of the book menu items are not included in the tree index that you build, meaning that book navigation items can't expand dynamically. You'll need to find some workaround for that...

cburschka’s picture

cburschka
they
commented

I can narrow down the problem: It specifically occurs when you are inside the book navigation menu. If you are anywhere else, the complete menu will load dynamically, but if you are inside it, only the active path will be loaded.

Alice Heaton’s picture

Alice Heaton commented
StatusFileSize
new dhtml_mlid_patch.txt831 bytes

Here is the patch as a file. I am confused about the mlid thing - if that is not guaranteed, it means there is no way to identify a menu item ? I'll have a look when I can ;)

cburschka’s picture

cburschka
they
commented

That is exactly what I found, and it is the reason I am using that silly "50000 + weight, title" index in the first place - that is what the menu tree generator seems to use for unique keys internally.

I may have found the problem: They have mlids, but they don't have a menu name set for some odd reason.

cburschka’s picture

cburschka
they
commented
Version: 6.x-3.0-beta » 6.x-3.x-dev
Status: Active » Needs review
StatusFileSize
new dhtml_menu_63_mlid-298720-6.patch2.74 KB

Ha, sorry, your approach is actually correct; all items except for tabs like node/%nid/edit have mlids and menu_names; my earlier problem has nothing to do with this.

Your implementation was merely incomplete. You are indexing the top level of the tree, but you need to use a recursive function to index the whole tree.

Patch is here.

cburschka’s picture

cburschka
they
commented
Status: Needs review » Reviewed & tested by the community

This can probably go in. I suggest you test it again to make sure it works with the i18nmenu module now.

Alice Heaton’s picture

Alice Heaton commented

Hi Arancaytar - thanks for your work ! I haven't had time to test your new patch yet, but will do so today or tomorrow ! Thank you so much for this !

cburschka’s picture

cburschka
they
commented
Status: Reviewed & tested by the community » Needs work

Patch borked.

patching file dhtml_menu.module
Hunk #1 FAILED at 45.
Hunk #2 succeeded at 140 (offset 13 lines).
Hunk #3 succeeded at 167 (offset 13 lines).
1 out of 3 hunks FAILED -- saving rejects to file dhtml_menu.module.rej
cburschka’s picture

cburschka
they
commented
Status: Needs work » Needs review
StatusFileSize
new dhtml_menu_63_mlid-298720-10.patch2.99 KB

Testing this.

cburschka’s picture

cburschka
they
commented
Status: Needs review » Patch (to be ported)

Committed to DRUPAL-6--3. Still needs porting to D7.

cburschka’s picture

cburschka
they
commented
Version: 6.x-3.x-dev » 7.x-1.x-dev
Status: Patch (to be ported) » Needs review
StatusFileSize
new dhtml_menu_mlid_7-298720-12.patch3.17 KB

Here we go.

cburschka’s picture

cburschka
they
commented
Status: Needs review » Fixed

Committed to HEAD after some testing.

Alice Heaton’s picture

Alice Heaton commented

A bit late, but I can confirm this patch works :) Thanks !

Status: Fixed » Closed (fixed)

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