New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conflict with WP admin bar #14

Closed
dariodev opened this Issue Mar 6, 2013 · 15 comments

Comments

Projects
None yet
5 participants
@dariodev

dariodev commented Mar 6, 2013

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

This comment has been minimized.

Show comment
Hide comment
@joeldbirch

joeldbirch Mar 7, 2013

Owner

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.

Owner

joeldbirch commented Mar 7, 2013

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.

@joeldbirch joeldbirch closed this Mar 7, 2013

@joeldbirch joeldbirch reopened this Mar 7, 2013

@dariodev

This comment has been minimized.

Show comment
Hide comment
@dariodev

dariodev Mar 7, 2013

:)
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.

dariodev commented Mar 7, 2013

:)
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

This comment has been minimized.

Show comment
Hide comment
@joeldbirch

joeldbirch Mar 7, 2013

Owner

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:

  • If you don't intend Superfish to use hoverIntent, simply set its disableHI option to true upon initialisation, or
  • If you do intend Superfish to use hoverIntent, you need to deregister WordPress' version and point to a patched version. The patch is backward compatible, so WordPress won't care that it is using the updated version. Here is the code I use in my functions.php file:
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.

Owner

joeldbirch commented Mar 7, 2013

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:

  • If you don't intend Superfish to use hoverIntent, simply set its disableHI option to true upon initialisation, or
  • If you do intend Superfish to use hoverIntent, you need to deregister WordPress' version and point to a patched version. The patch is backward compatible, so WordPress won't care that it is using the updated version. Here is the code I use in my functions.php file:
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

This comment has been minimized.

Show comment
Hide comment
@joeldbirch

joeldbirch Mar 7, 2013

Owner

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

Owner

joeldbirch commented Mar 7, 2013

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

@dariodev

This comment has been minimized.

Show comment
Hide comment
@dariodev

dariodev Mar 7, 2013

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.

dariodev commented Mar 7, 2013

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.

@dariodev dariodev closed this Mar 7, 2013

@joeldbirch

This comment has been minimized.

Show comment
Hide comment
@joeldbirch

joeldbirch Mar 13, 2013

Owner

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…

Owner

joeldbirch commented Mar 13, 2013

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

This comment has been minimized.

Show comment
Hide comment
@joeldbirch

joeldbirch Mar 13, 2013

Owner

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

Owner

joeldbirch commented Mar 13, 2013

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

@RollYourOwnEd

This comment has been minimized.

Show comment
Hide comment
@RollYourOwnEd

RollYourOwnEd Jun 6, 2013

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

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

@ruudbwai

This comment has been minimized.

Show comment
Hide comment
@ruudbwai

ruudbwai Jul 19, 2013

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

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

@ElricE

This comment has been minimized.

Show comment
Hide comment
@ElricE

ElricE Sep 4, 2013

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

ElricE commented Sep 4, 2013

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

This comment has been minimized.

Show comment
Hide comment
@joeldbirch

joeldbirch Sep 6, 2013

Owner

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.

Owner

joeldbirch commented Sep 6, 2013

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.

@joeldbirch joeldbirch reopened this Sep 6, 2013

@ElricE

This comment has been minimized.

Show comment
Hide comment
@ElricE

ElricE Sep 9, 2013

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

ElricE commented Sep 9, 2013

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

This comment has been minimized.

Show comment
Hide comment
@joeldbirch

joeldbirch Sep 9, 2013

Owner

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.

Owner

joeldbirch commented Sep 9, 2013

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.

@joeldbirch joeldbirch closed this Sep 9, 2013

@ElricE

This comment has been minimized.

Show comment
Hide comment
@ElricE

ElricE Sep 9, 2013

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?

ElricE commented Sep 9, 2013

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

This comment has been minimized.

Show comment
Hide comment
@joeldbirch

joeldbirch Sep 14, 2013

Owner

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;
}
Owner

joeldbirch commented Sep 14, 2013

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;
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment