CiviCRM Community Forums (archive)

Hi,

Currently, Civi is attaching an onClick handler to any item in the Drupal admin menu that contains the text "CiviCRM".  The purpose is to allow the "CiviCRM" menu item to display the CiviCRM navigation menu.  Unfortunately, the jQuery selector in use also adds this onClick handler to any item in the menu that contains "CiviCRM" anywhere, and this includes items named, for example, "foo CiviCRM bar", plus any of their parent items, since the parent items are also containers of that text.

An example of the problem in real life: the "CiviCRM congressional district" module ads a menu item labeled "CiviCRM congressional district" under "Site configuration," thus rendering the whole "Site configuration" menu un-clickable.

There's a comment in the code for this that says, "Need to fix this properly."  Here is a "relatively more proper" fix.  This uses child selector syntax to make sure only top-level menu items are checked for the "CiviCRM" string.  Then the onClick handler checks that the full string of the link text really is exactly 'CiviCRM', before executing the $.toggle() call.

The patch is:
Index: templates/CRM/common/Navigation.tpl
===================================================================
--- templates/CRM/common/Navigation.tpl   (revision 30326)
+++ templates/CRM/common/Navigation.tpl   (working copy)
@@ -84,9 +84,11 @@
 
 /* Need to fix this properly*/
 cj( function() {
-    cj("#admin-menu").find("li :contains('CiviCRM')").click(function() {
-        cj("#civicrm-menu").toggle();
-        return false;
+    cj("#admin-menu>ul>li>a:contains('CiviCRM')").click(function() {
+        if (cj(this).html() == 'CiviCRM' ) {
+            cj("#civicrm-menu").toggle();
+            return false;
+        }
     });
 
     var contactUrl = {/literal}"{crmURL p='civicrm/ajax/rest' q='className=CRM_Contact_Page_AJAX&fnName=getContactList&json=1&context=navigation' h=0 }"{literal};



This patch works for me and seems sound, but I have not tested it much. But there it is, in case it's useful.

- TM

Yeah, good call X+, I don't know why I didn't think of it that way.  You're probably right.

Probably to know which one is really the most efficient, we'd have to know more about the internal workings of  jQuery's :contains selector.  But considering that the admin menu is only going to have a limited number of top-level menu items, it may not be worth figuring out.

BTW, this is probably worth submitting an issue with patch, right?

-TM

Probably to know which one is really the most efficient, we'd have to know more about the internal workings of  jQuery's :contains selector.  But considering that the admin menu is only going to have a limited number of top-level menu items, it may not be worth figuring out.

I'm pretty sure that doing a contains + adding a event click on each + doing a test on == is longer than simply doing a == and only add the events on the right
(:contains is an expensive selector, at least more expensive than nothing

This being said, as you mention it, that's about 2,5 dom node anyway, so no big deal;

X+