joeldbirch / superfish

Superfish is a jQuery plugin that adds usability enhancements to existing multi-level drop-down menus.
Other
912 stars 315 forks source link

Conflict with WP admin bar #14

Closed dariodev closed 11 years ago

dariodev commented 11 years ago

Hi Joel, I've been using Superfish for years and I'm happy to see that you are continuing to update the plugin.
There is compatibility issue with WordPress admin-bar (wp-includes/js/admin-bar.min.js?ver=3.5.1). When the user is logged in (and/or the admin bar is turned on), menus are not opening on mouseover. Menus are opening only on mouse click (but not behave as intended) , even if "useClick" is set to false. Only if "useClick" is true, then menus are working good. And again, if WP admin bar is off, everything (useClick false/true) works great.

joeldbirch commented 11 years ago

Hi,

Great to hear from a die-hard Superfish fan :)

I've tried to reproduce this issue but it works fine for me. I use WordPress daily and haven't seen the issue. I wonder if it still happens for you if you change the theme? Can you try that please? It would be good to figure out what is causing the issue and fix it if it does turn out to be an issue with Superfish.

Cheers Joel.

dariodev commented 11 years ago

:) Yes, I've tried with twentytwelve, with all plugins turned off. I've tried again on clean WP installation - WP 3.5.1, Twentytwelve. I just made screen-cast with annotations: http://youtu.be/6roe8wGiY00 This screencast is made in Chrome, but issue is same in all browsers.

joeldbirch commented 11 years ago

Amazing work. Your screencast really removed any question of there being a problem, I really appreciate the effort!

I began to replicate the same environment you have in the screencast, and half way there it dawned on me. Superfish uses the hoverIntent plugin if it detects its availability on the page. However, because Superfish now uses event delegation, it requires a patched version of hoverIntent. Wordpress includes the official (unpatched) hoverIntent plugin for use in the admin bar, so Superfish is using that and failing. You should see an error displayed in the JavaScript console explaining the problem.

This explains why only the hover events are affected and not the click events used with the useClick feature. The reason why I've never seen the problem in my own projects is because I've preempt it, apply a fix and (stupidly) not given a thought to how it would also be affecting other people. See below for solutions.

Quick fixes:

wp_deregister_script('hoverIntent');
wp_register_script('hoverIntent', ( '/path/to/hoverIntent.js'), array('jquery'), 'r6.1');

More thorough solutions: I'll have a think about what I should do to save other people from encountering this conflict. I think the easiest solution would be to for me to set the default for disableHI in the next release to true so people have to opt-in to it, but I'd prefer to avoid doing this. Or maybe I'll ask Brian Cherne to update his official hoverIntent plugin to include the event delegation patch. I guess I then need to get WordPress to include that updated version in their next release, somehow. This would be the ideal—but not instant—resolution.

Thanks so much for bringing this to my attention. Please let me know if this fixes the issue for you.

Cheers Joel.

joeldbirch commented 11 years ago

I have commented about this on an existing pull request over on Brian Cherne's repo.

dariodev commented 11 years ago

All right, it works! You rock, thank you very much. About possible solutions. I'm for disableHI to true for the next release (temporarily?), this could save a lot of headache for someone. The ideal solution involves a lot of variables, so yes I guess it's far from an instant :) Thank you so much for your quick response and quick fixes.

joeldbirch commented 11 years ago

Brian Cherne has now updated his plugin to support event delegation! We just need to get WordPress to update to the new version. Not sure how to go about this, but I'll have a look around…

joeldbirch commented 11 years ago

I have created a WordPress Trac ticket with the update request.

RollYourOwnEd commented 11 years ago

Hey guys, I had exactly the same problem and this helped me! Thanks to both of you!

ruudbwai commented 11 years ago

yeah also had this issue, almost cried :/ didn't know hoverintent was bundled in with wordpress

ElricE commented 11 years ago

Got a curly one for you, as I've found this behaviour is still happening in our WP site. When disableHI is set to "true", the hoverClass correctly appends to the LI elements with child menu elements. However, when disableHI is set to "false", the hoverClass is instead appending to the top level UL. Menus expand on right-click but not on hover as a result of the hoverClass appending incorrectly.

Now hoverIntent has been upgraded so no dramas there, and I've tried disabling all non-essential javascript hoping to find a conflict but no luck. I also dropped in a duplicate of the "basic" menu provided in the plug-in's example folder to see if it was an issue with the menu itself, but got exactly the same result.

Now I have been able to patch in a temp work around. I changed line 60 from

$menu.hoverIntent(over, out, targets);

to

$('li:has(ul)').hoverIntent(over, out, target); 

which seems to work for now but, but not being the most adept at JS am not sure whether it the best option. Could the hoverIntent events be bound using

.on
as the variables in the next section

$menu.on('mouseenter.superfish', targets, over).on('mouseleave.superfish', targets, out);

works without a hitch.

To be quite frank this has had me stumped for days

joeldbirch commented 11 years ago

Curly indeed! Are you able to provide a link so I can see the problem in action? I'd be happy to look into it but I need to be able to reproduce it, somehow. Thanks.

ElricE commented 11 years ago

Hi Joel I just set up a test menu and swapped out the patched version of superfish with the original for that page only so it doesn't interfere with the global site navigation. Greatly appreciate your time, and it would be interesting to know if others have encountered the problem.

Also note that although I'm running latest releases of jquery & jquery ui, temporarily reverting back to the original WP releases made no difference either.

http://stuartsuits.generatordigital.com.au/superfish-testpage/

Thanks again - E

joeldbirch commented 11 years ago

Thanks Elric,

I created my own set of files from that test page and set about deleting everything not related to Superfish. It turns out that the file named "jquery.slick.contact.1.3.2.js" includes its own (old) version of hoverIntent. Deleting that fixes the issue.

Hope this helps.

Joel.

ElricE commented 11 years ago

Wow, nice pick... thankyou so much for that. I'll advise the developer of the plug-in, as bundling hoverIntent into the source code when WP already includes a more recent version is a no-go.

BTW... one other difficult I'm having. In the older builds the submenu indicators were in a separate span making hiding the top-level quite easy. These new csArrows seem to make use of Toggle(show/hide) function in jQuery itself and thus Superfish doesn't seem to generate the rules of application.

Is there any means beyond css of removing arrows from top menu level only and retaining for deeper nesting?

joeldbirch commented 11 years ago

I can't think of a JavaScript way of removing only the top level arrows, but the CSS solution is pretty simple:

.sf-arrows > li > .sf-with-ul:after {
  content: normal;
}