Skip to main content
Topic: Superfish tweaks. (Read 4679 times) previous topic - next topic
0 Members and 1 Guest are viewing this topic.

Superfish tweaks.

Just noticed something. After pigging around with my custom click menu, I thought I'd take another look at the Superfish plugin that runs the current hover menus. There's room for some optimisation in there (IIRC it was originally written for jQ 1.2.x).

1/ For a start, there's no longer any need to set a hover class here (or anywhere else):

Code: [Select]
					if (o.$path.length && $$.parents(['li.',o.hoverClass].join('')).length<1){over.call(o.$path);}

The hover class was only ever included to provide li:hover support for IE<7.  Since we don't care about that any more, any code related to the hover class can be removed. Not only is this good for shortening the js, but it also gets rid of some stuff from the css (because if the hover class is being applied, the css needs to deal with it, otherwise things don't work).


2/ It would be good to figure out how to get rid of this last line related to the (deprecated) add arrow stuff. Better to add arrows (if wanted) via css.

Code: [Select]
			// This next line is essential, despite the other code for arrows being removed.
// Changing the next line WILL break hoverIntent functionality. Very bad.
addArrow = function($a){$a.addClass(c.anchorClass)};


3/ More hover class poo, and IIRC using li:has(ul) is not the fastest way of handling that one. Would have to check.
Also, I have NFI what the breadcrumbs class (bcClass) was supposed to be for, but I can't recall it ever doing anything useful.

Code: [Select]
				$(this).addClass([o.hoverClass,c.bcClass].join(' '))
.filter('li:has(ul)').removeClass(o.pathClass);


4/ There's no need to allow for disabling hoverIntent. IMO, that setting is just bloat.

Code: [Select]
			$('li:has(ul)',this)[($.fn.hoverIntent && !o.disableHI) ? 'hoverIntent' : 'hover'](($.fn.hoverIntent && !o.disableHI) ? (h) : (over,out)).each(function() {})
.not('.'+c.bcClass)


5/ I have a suspicion that none of the following are really needed (only a suspicion at this stage).

Code: [Select]
	sf.c = {
bcClass     : 'sf-breadcrumb',
menuClass   : 'sf-js-enabled',
anchorClass : 'sf-with-ul',
};


6/ As mentioned, hover class and disable hoverIntent could be dropped here.

Code: [Select]
	sf.defaults = {
hoverClass : 'sfhover',
pathClass : 'current',
pathLevels : 1,
delay : 600,
animation : {opacity:'show', height: 'show', width: 'show'},
speed : 200,
disableHI : false, // Leave as false. True disables hoverIntent detection (not good).


7/ This pile of stuff could be simplified quite a bit.

Code: [Select]
	$.fn.extend({
hideSuperfishUl : function(){
var o = sf.op,
not = (o.retainPath===true) ? o.$path : '';
o.retainPath = false;
var $ul = $(['li.',o.hoverClass].join(''),this).add(this).not(not).removeClass(o.hoverClass)
.find('>ul').hide().css('opacity','0');
o.onHide.call($ul);
return this;
},

For a start, cutting out the IE<7 hover class is going to significantly de-bloat this section.
The other point is the hide() operation. Originally the code in that line read like this:

Code: [Select]
					.find('>ul').hide().css('display','none');

I changed that because display: none; will bork things for screen readers, but of course at the time I didn't realise that .hide() did exactly the same thing. So the original code effectively said "set display: none; by using the slowest method, then set display: none; again".

Currently it effectively says "set display: none; by using the slowest method, then set opacity: 0;".

Methinks that perhaps this is not the best way of coding it. :D A better option would be:

Code: [Select]
					.find('>ul').css('opacity','0');

Fewer operations, uses the fastest one, and better for a11y. :)


8/ More hover class poo here.

Code: [Select]
		showSuperfishUl : function(){
var o = sf.op,
sh = sf.c,
$ul = this.addClass(o.hoverClass)


There are probably a few other things that could be tweaked, but those are the obvious ones.
Last Edit: June 11, 2013, 08:21:20 am by TestMonkey
Master of Expletives: Now with improved family f@&king friendliness! :D

Sources code: making easy front end changes difficult since 1873. :P

Re: Superfish tweaks.

Reply #1

Hmm. Looks like I'm not the only one who thinks Superfish needs a revamp. I did a bit of digging and found this: https://github.com/bobbravo2/superfish-reloaded

Also ran the linked jsperf test and the results are rather astonishing, at least for this particular test case..

Original Superfish - 120 ops/sec.
 Superfish reloaded - 472,034ops/sec.

The difference is so great that the original version doesn't even show up in the bar graphs of the test results.

It's still not perfect. It's bloated with a lot of stuff to support IE6 and older versions of jQuery (back to 1.2). In fact the repo still contains jQuery 1.2.6.

It has markup for things that would be better handled with css and/or pseudo elements. It has settings that nobody is ever likely to want.

It still uses .hide() for the drop menus, which not only borks screen readers but is totally unnecessary anyway, since Superfish menus are always backed by full css which hides the menus off hover even without any help. So, all .hide() does is slow the javascript down and reduce accessibility.

This is also called in the default animation for the drops, in the form of opacity:'hide' and opacity:'show', which has the same screen reader borking effects (but at least is easy to change in that bit).

Note that I've tried playing with the original a bit and the code seems rather brittle (although of course I know bugger all jQ). This one seems to be more complex.

What Superfish really needs is to be rewritten with the assumption that everyone will be using at least jQ 1.7.x, and wont care about pretty effects on old IE, and does want to have good a11y for everyone. That would rock (but it aint been done yet).



Just tried this thing on local with one of the GitHub 2.1 builds. It appears to be buggy. HoverIntent detection is failing, and mouseout delay is failing. It also loses the drop menu sometimes when hovering over a top level tab. So, it may be fast, but it still needs a fair bit of work.
Last Edit: December 30, 2012, 12:51:26 am by Antechinus
Master of Expletives: Now with improved family f@&king friendliness! :D

Sources code: making easy front end changes difficult since 1873. :P

Re: Superfish tweaks.

Reply #2

See what this breaks  :P

Code: [Select]
;(function($){
$.fn.superfish = function(op){
var sf = $.fn.superfish,
c = sf.c,
over = function(){
var $$ = $(this),
menu = getMenu($$);

clearTimeout(menu.sfTimer);
$$.showSuperfishUl().siblings().hideSuperfishUl();
},
out = function(){
var $$ = $(this),
menu = getMenu($$),
o = sf.op;

clearTimeout(menu.sfTimer);
menu.sfTimer = setTimeout(function(){
$$.hideSuperfishUl();
},o.delay);
},
getMenu = function($menu){
var menu = $menu.parents(['ul.', c.menuClass,':first'].join(''))[0];

sf.op = sf.o[menu.serial];
return menu;
};

return this.each(function() {
var s = this.serial = sf.o.length,
o = $.extend({},sf.defaults,op),
h = $.extend({},sf.hoverdefaults, {over: over, out: out}, op);

sf.o[s] = sf.op = o;
$('li:has(ul)', this)[($.fn.hoverIntent && !o.disableHI) ? 'hoverIntent' : (o.enableClick) ? 'click' : 'hover'](($.fn.hoverIntent && !o.disableHI) ? (h) : (over,out)).not('.' + c.bcClass).hideSuperfishUl();

var $a = $('a', this);
$a.each(function(i){
var $li = $a.eq(i).parents('li');
$a.eq(i).focus(function(){over.call($li);}).blur(function(){out.call($li);});
});
o.onInit.call(this);
}).each(function() {
var menuClasses = [c.menuClass];
$(this).addClass(menuClasses.join(' '));
});
};

var sf = $.fn.superfish;
sf.o = [];
sf.op = {};
sf.c = {
bcClass     : 'sf-breadcrumb',
menuClass   : 'sf-js-enabled',
anchorClass : 'sf-with-ul',
};
sf.defaults = {
pathClass : 'current',
delay : 600,
animation : {opacity:'show', height:'show'},
speed : 200,
enableClick : false, // true enables click detection (if disableHI is true or if there's no hoverIntent)
disableHI : false, // Leave as false. True disables hoverIntent detection (not good).
onInit : function(){}, // callback functions
onBeforeShow: function(){},
onShow : function(){},
onHide : function(){}
};
sf.hoverdefaults = {
sensitivity : 8,
interval    : 50,
timeout     : 1
};
$.fn.extend({
hideSuperfishUl : function(){
var o = sf.op,
not = '';

var $ul = this.find('>ul').hide();
o.onHide.call($ul);
return this;
},
showSuperfishUl : function(){
var o = sf.op,
sh = sf.c,
$ul = this.find('>ul:hidden').css('opacity', 1);

o.onBeforeShow.call($ul);
$ul.animate(o.animation, o.speed, function(){o.onShow.call($ul);});
return this;
}
});

})(jQuery);

Re: Superfish tweaks.

Reply #3

It's not bad. Mouseout delay is still failing, and it's not calling the ul background on tab through (falls back to the no-js solution).

Mind you the dropdown animation is still jerkier at times than my nifty new click menus. Those are mega smooth .:D
Last Edit: December 30, 2012, 02:14:01 pm by Antechinus
Master of Expletives: Now with improved family f@&king friendliness! :D

Sources code: making easy front end changes difficult since 1873. :P