When using drupal_add_js with nice_menus, drupal will add another

tag for loading jquery. This will cause the older jquery object to get overwritten, along with $.fn.addShim. On my installation, I changed this code in nice_menus_menu: 
    // We only want to include the JS for IE and not browsers 
    // capable of doing everything in css.  We have to put all the JS
    // in drupal_set_html_head so they get called in the right order.
    drupal_set_html_head('<!--[if IE]>
    <script type="text/javascript" src="'. check_url(base_path() .'misc/jquery.js') .'"></script>
    <script type="text/javascript" src="'. check_url(base_path() .'misc/drupal.js') .'"></script>
    <script type="text/javascript" src="'. check_url(base_path() . drupal_get_path('module', 'nice_menus') .'/nice_menus.js') .'"></script>
    <![endif]-->');
Into this: drupal_add_js( drupal_get_path('module', 'nice_menus') .'/nice_menus.js' ); (drupal_add_js adds drupal.js and jquery.js automatically, and knows not to include the same file twice). This seems to do the trick, but includes the JS for browsers other than IE as well. I guess you could check the user-agent header, and do it for IE only.
CommentFileSizeAuthor
#3 nice_menus-drupal-add-js_235562-3.patch5.87 KBadd1sun

Comments

add1sun’s picture

add1sun commented

Urgh, yeah JS browser detection is just ugly compared to IE conditional comments which is why we went that route. Hrm.

(/me keeps praying for IE6 EOL.)

Anonymous’s picture

Anonymous (not verified) commented

Using this method causes jQuery to be included twice. This causes, among other things, the issue described at http://drupal.org/node/242086.

Using the solution outlined above fixes this issue.

add1sun’s picture

add1sun commented
Version: 5.x-1.1 » 6.x-2.x-dev
Status: Active » Needs review
StatusFileSize
new nice_menus-drupal-add-js_235562-3.patch5.87 KB

I'm starting this up working in HEAD and then we can backport if this is a good solution. I have added a checkbox to enable/disable JS on the settings page (defaults to enabled) and include JS with drupal_add_js if that variable is set. In the JS file itself I am using conditional compilation to only apply the JS to IE for the time-being.

I went this route because there is also a ticket to have additional, optional JS for all browsers at some point and this lets us build on later, while screening the weird stuff for IE. This new setup will also let folks just disable nice menus JS altogether if they don't want it and they don't care about IE working properly.

Please test this out, review the approach and provide feedback.

add1sun’s picture

add1sun commented

Hm, well conditional compilation works fine if we want to detect IE but it won't work to sort out IE6 (yeah I know about jscript versions but there apparently was an update that moved IE 6 up to the same jscript version as IE 7, cuz MS hates us). So I'm leaving this for now as is, but chances are we will need to ultimately do other detection because the plan is to not have IE7 use the funky JS anymore (#269857: Remove JS from IE 7) so we will need this to target only IE6 sometime soon.

I did some poking around and basically it looks like a generic object detection is the route we'll need to do (something like if (!window.XMLHttpRequest) which is not in IE6) even though that is crappy. So if anyone has ideas for the best way to filter for IE 6 in JS rather than using the HTML conditional comments, I'm all ears.

Morn’s picture

Morn commented

The patch seems to work (#3). I applied the patch to nice menus 6.x-1.1.
The problems I had with lightbox2 under IE 6 and IE 7 ( because JQuery is included twice) disappeared.
I am using Drupal 6.3.

add1sun’s picture

add1sun commented
Status: Needs review » Fixed

Great, thanks for the feedback! I'm going to add this to the dev versions for 5, 6-1 and 6-2. No release yet because I want to change this up a bit with all of the IE 7 fixes as well.

It's a bummer to load up the drupal and jquery JS files for every page in all browsers but it means it won't break other stuff. Alas.

poiu’s picture

poiu commented

Hi add1sun,

I may be missing something here, but if you're bummed about loading up JS files for all browsers, why not just check $_SERVER['HTTP_USER_AGENT'] against some regexp and do it just for IE? something like

if( $_SERVER['HTTP_USER_AGENT'] contains the string 'MSIE 6' ) {
  drupal_add_js( drupal_get_path('module', 'nice_menus') .'/nice_menus.js' );
}

There's some code that claims to do this (and more) at http://apptools.com/phptools/browser/source.php, it could be useful for all kinds of browser specific hacks, not just nice menus.

Of course, I didn't really test it or anything...

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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

br4t’s picture

br4t commented

Thanks. For me this solved an issue with lightbox2 module in IE7.
Because(i assume) jquery.js got loaded twice this way, lightbox2 showed all navigation links twice; didn't show the close link; did show the increase/decrease size button and the "image loading" animation didn't disappear after img load.

Once more i'm a happy camper!

danschellenberg’s picture

danschellenberg commented

Is this going to be fixed in the main release soon? I'd love to have my lightbox showing properly in IE, but don't really feel like messing about with patches...

add1sun’s picture

add1sun commented

This is in the latest releases.