456 Berea Street
29th May 2007, 10:50 pm
Everybody makes mistakes, so occasionally things go wrong. One example is when Swedish design magazine Cap&Design (http://capdesign.idg.se/) recently published a tutorial on creating a collapsing and expanding menu.
While reading the article my jaw dropped as I realised that the tutorial uses invalid, non-semantic HTML, inline CSS and event handlers, bloated (but, amazingly, valid) CSS, and obtrusive JavaScript. In 2007.
I sent an email to the magazine's editor, who agreed that the article shouldn't have been printed and took the example code offline. Thank you very much for doing that – hopefully it will limit the number of people using the code in their projects.
Instead of just complaining about the lousy code and deriving satisfaction from making them take the code offline, I decided to recreate the menu from the tutorial in a more modern way, using leaner and more efficient HTML and CSS, and unobtrusive JavaScript. This kind of menu isn't anything new, of course. Nevertheless I think publishing this comparison of the two approaches may be useful.
The HTML
The HTML that appeared in the article is this (shortened, and with id values translated from Swedish):
Category 1 (http://www.websitearchitecture.co.uk/forum/#)
Submenu 1 (http://www.websitearchitecture.co.uk/forum/#)
Submenu 1 (http://www.websitearchitecture.co.uk/forum/#)
Category 2 (http://www.websitearchitecture.co.uk/forum/#)
Submenu 2 (http://www.websitearchitecture.co.uk/forum/#)
Submenu 2 (http://www.websitearchitecture.co.uk/forum/#)
Category 3 (http://www.websitearchitecture.co.uk/forum/#)
Submenu 3 (http://www.websitearchitecture.co.uk/forum/#)
Submenu 3 (http://www.websitearchitecture.co.uk/forum/#)
This markup is problematic for several reasons:
id values must be unique, and may not be shared by several elements on the same page.
Using CSS to hide something (via style="display:none") and JavaScript to show it makes the hidden content inaccessible to people who browse with CSS on and JavaScript off.
It uses inline CSS.
It uses inline event handlers.
The menu consists of several lists of links, so list elements should be used instead of div elements.I replaced the HTML with this:
Category 1 (http://www.websitearchitecture.co.uk/forum/#)
Submenu 1a (http://www.websitearchitecture.co.uk/forum/#)
Submenu 1b (http://www.websitearchitecture.co.uk/forum/#)
Category 2 (http://www.websitearchitecture.co.uk/forum/#)
Submenu 2a (http://www.websitearchitecture.co.uk/forum/#)
Submenu 2b (http://www.websitearchitecture.co.uk/forum/#)
Category 3 (http://www.websitearchitecture.co.uk/forum/#)
Submenu 3a (http://www.websitearchitecture.co.uk/forum/#)
Submenu 3b (http://www.websitearchitecture.co.uk/forum/#)
The menu now consists of nested lists of links, the inline CSS and event handlers are gone, and a class name is used to allow for more than one menu on a page, should there be a need for that. Should the browser lack support for JavaScript, all sub menus will now be displayed, and the user is not prevented from navigating the site. Much better in my opinion.
The CSS
Next a look at the CSS used in the article (id selectors translated from Swedish):
#menu {
font-family: Verdana, Arial, Helvetica, sans-serif;
font-size: 10px;
font-weight: normal;
left: 20px;
top: 20px;
}
#menu #mainmenu a {
color: #FFFFFF;
text-decoration: none;
display: block;
background-color: #990066;
padding-top: 2px;
padding-right: 5px;
padding-bottom: 2px;
padding-left: 5px;
width: 150px;
position:relative;
}
#menu #mainmenu a:hover {
background-color: #CC6699;
}
#menu #submenu a {
color: #FFFFFF;
text-decoration: none;
display: block;
background-color: #CC0066;
padding-top: 2px;
padding-right: 5px;
padding-bottom: 2px;
padding-left: 5px;
width: 135px;
left: 15px;
position:relative;
}
#menu #submenu a:hover {
background-color: #CC6699;
}While that CSS is not invalid, it is definitely not optimal, for the following reasons:
It suffers from what I call selectoritis, that is the use of overly specific selectors (#menu #mainmenu etc.).
No use of shorthand, making the CSS bulkier than necessary.
It uses positioning for no apparent reason.Here is what I replaced it with:
.menu,
.menu ul {
margin:0;
padding:0;
list-style:none;
}
.menu {width:200px;}
.menu li {
margin:0;
padding:0;
margin-bottom:1px;
}
.menu a {
display:block;
padding:2px 5px;
color:#000;
background:#b0c23d;
text-decoration:none;
}
.menu a:hover {background:#d9dcb0;}
.menu ul li {padding-left:15px;}
.menu ul a {background:#ced174;}
.hidden {display:none;}I removed the font declarations since font-family and font-size are normally specified elsewhere in the CSS. While I was at it I also changed the colours to make looking at the menu a bit easier on my eyes, and added some padding and a bottom margin to the links.
The JavaScript
Finally a look at the JavaScript. The article uses the following (variable names translated from Swedish):
function toggle(submenu) {
if (document.getElementById(submenu).style.display == "none")
document.getElementById(submenu).style.display = "block";
else if (document.getElementById(submenu).style.display == "block")
document.getElementById(submenu).style.display = "none"
else alert("An error occured in the menu. Contact the web master.");
}The problems with this script are:
It doesn't check if the browser supports the method it uses.
It works by directly changing the element style.
If anything should go wrong it displays a completely useless message.Ok, I'll admit that the script does have an advantage over my replacement: it is a lot shorter than this:
var toggleMenu = {
init : function(sContainerClass, sHiddenClass) {
if (!document.getElementById || !document.createTextNode) {return;} // Check for DOM support
var arrMenus = this.getElementsByClassName(document, 'ul', sContainerClass); // Find all menus
var arrSubMenus, oSubMenu, oLink;
for (var i = 0; i < arrMenus.length; i++) { // In each menu...
arrSubMenus = arrMenus[i].getElementsByTagName('ul'); // ...find all sub menus
for (var j = 0; j < arrSubMenus.length; j++) { // For each sub menu...
oSubMenu = arrSubMenus[j];
oLink = oSubMenu.parentNode.getElementsByTagName('a')[0]; // ...find the link that toggles it...
oLink.onclick = function(){toggleMenu.toggle(this.parentNode.getEl ementsByTagName('ul')[0], sHiddenClass); return false;} // ... add an event handler to the link...
this.toggle(oSubMenu, sHiddenClass); // ... and hide the sub menu
}
}
},
toggle : function(el, sHiddenClass) {
var oRegExp = new RegExp("(^|\s)" + sHiddenClass + "(\s|$)");
el.className = (oRegExp.test(el.className)) ? el.className.replace(oRegExp, '') : el.className + ' ' + sHiddenClass; // Add or remove the class name that hides the element
},
/* addEvent and getElementsByClassName functions omitted for brevity */
};
// Initialise the menu
toggleMenu.addEvent(window, 'load', function(){toggleMenu.init('menu','hidden');});The replacement script works by adding or removing a class name instead of directly changing the style property of the sub menu elements. It
The addEvent() and getElementsByClassName() functions are included in the full togglemenu.js (http://www.456bereastreet.com/lab/accessible-expanding-collapsing-menu/togglemenu.js) script. If you use a JavaScript library that contains similar functions (they all do, probably) I suggest using those functions instead to avoid code bloat.
You can try it all put together on the Accessible expanding and collapsing menu demo page (http://www.456bereastreet.com/lab/accessible-expanding-collapsing-menu/).
Caveats and some homework for the reader
Like I said at the beginning of this article, this has been done before. But hopefully showing the before and after versions of the HTML, CSS, and JavaScript will make it easier to see why I made the changes.
I should also note that this script does inherit a problem from the script published in the magazine: it disables the links to the top level category pages, preventing the user from navigating to them. That could be a serious problem depending on how the site is structured.
This script also does not handle multiple levels. Extending it to allow for that is no big deal, but I'd think twice before doing so. Remember that there will be a lot of links for people with JavaScript disabled to wade through.
I'll leave it as a learning exercise for the reader to alter the script so that it inserts a separate link for expanding and collapsing each sub menu and lets the top level links behave normally. And, if you want the functionality, add support for multiple levels.
Visit site to read or post comments… (http://www.456bereastreet.com/archive/200705/accessible_expanding_and_collapsing_menu/#comments)Add 456 Berea Street to your Technorati favorites. (http://technorati.com/faves?add=http://www.456bereastreet.com)
Posted in JavaScript (http://www.456bereastreet.com/archive/categories/javascript/).
http://feeds.feedburner.com/~a/456bereastreet?i=84oScK</img> (http://feeds.feedburner.com/~a/456bereastreet?a=84oScK)
http://feeds.feedburner.com/~f/456bereastreet?i=e3i4YDko</img> (http://feeds.feedburner.com/~f/456bereastreet?a=e3i4YDko) http://feeds.feedburner.com/~f/456bereastreet?i=fQeioUvV</img> (http://feeds.feedburner.com/~f/456bereastreet?a=fQeioUvV) http://feeds.feedburner.com/~f/456bereastreet?i=MqXA7YXY</img> (http://feeds.feedburner.com/~f/456bereastreet?a=MqXA7YXY)
More... (http://www.456bereastreet.com/archive/200705/accessible_expanding_and_collapsing_menu/)
While reading the article my jaw dropped as I realised that the tutorial uses invalid, non-semantic HTML, inline CSS and event handlers, bloated (but, amazingly, valid) CSS, and obtrusive JavaScript. In 2007.
I sent an email to the magazine's editor, who agreed that the article shouldn't have been printed and took the example code offline. Thank you very much for doing that – hopefully it will limit the number of people using the code in their projects.
Instead of just complaining about the lousy code and deriving satisfaction from making them take the code offline, I decided to recreate the menu from the tutorial in a more modern way, using leaner and more efficient HTML and CSS, and unobtrusive JavaScript. This kind of menu isn't anything new, of course. Nevertheless I think publishing this comparison of the two approaches may be useful.
The HTML
The HTML that appeared in the article is this (shortened, and with id values translated from Swedish):
Category 1 (http://www.websitearchitecture.co.uk/forum/#)
Submenu 1 (http://www.websitearchitecture.co.uk/forum/#)
Submenu 1 (http://www.websitearchitecture.co.uk/forum/#)
Category 2 (http://www.websitearchitecture.co.uk/forum/#)
Submenu 2 (http://www.websitearchitecture.co.uk/forum/#)
Submenu 2 (http://www.websitearchitecture.co.uk/forum/#)
Category 3 (http://www.websitearchitecture.co.uk/forum/#)
Submenu 3 (http://www.websitearchitecture.co.uk/forum/#)
Submenu 3 (http://www.websitearchitecture.co.uk/forum/#)
This markup is problematic for several reasons:
id values must be unique, and may not be shared by several elements on the same page.
Using CSS to hide something (via style="display:none") and JavaScript to show it makes the hidden content inaccessible to people who browse with CSS on and JavaScript off.
It uses inline CSS.
It uses inline event handlers.
The menu consists of several lists of links, so list elements should be used instead of div elements.I replaced the HTML with this:
Category 1 (http://www.websitearchitecture.co.uk/forum/#)
Submenu 1a (http://www.websitearchitecture.co.uk/forum/#)
Submenu 1b (http://www.websitearchitecture.co.uk/forum/#)
Category 2 (http://www.websitearchitecture.co.uk/forum/#)
Submenu 2a (http://www.websitearchitecture.co.uk/forum/#)
Submenu 2b (http://www.websitearchitecture.co.uk/forum/#)
Category 3 (http://www.websitearchitecture.co.uk/forum/#)
Submenu 3a (http://www.websitearchitecture.co.uk/forum/#)
Submenu 3b (http://www.websitearchitecture.co.uk/forum/#)
The menu now consists of nested lists of links, the inline CSS and event handlers are gone, and a class name is used to allow for more than one menu on a page, should there be a need for that. Should the browser lack support for JavaScript, all sub menus will now be displayed, and the user is not prevented from navigating the site. Much better in my opinion.
The CSS
Next a look at the CSS used in the article (id selectors translated from Swedish):
#menu {
font-family: Verdana, Arial, Helvetica, sans-serif;
font-size: 10px;
font-weight: normal;
left: 20px;
top: 20px;
}
#menu #mainmenu a {
color: #FFFFFF;
text-decoration: none;
display: block;
background-color: #990066;
padding-top: 2px;
padding-right: 5px;
padding-bottom: 2px;
padding-left: 5px;
width: 150px;
position:relative;
}
#menu #mainmenu a:hover {
background-color: #CC6699;
}
#menu #submenu a {
color: #FFFFFF;
text-decoration: none;
display: block;
background-color: #CC0066;
padding-top: 2px;
padding-right: 5px;
padding-bottom: 2px;
padding-left: 5px;
width: 135px;
left: 15px;
position:relative;
}
#menu #submenu a:hover {
background-color: #CC6699;
}While that CSS is not invalid, it is definitely not optimal, for the following reasons:
It suffers from what I call selectoritis, that is the use of overly specific selectors (#menu #mainmenu etc.).
No use of shorthand, making the CSS bulkier than necessary.
It uses positioning for no apparent reason.Here is what I replaced it with:
.menu,
.menu ul {
margin:0;
padding:0;
list-style:none;
}
.menu {width:200px;}
.menu li {
margin:0;
padding:0;
margin-bottom:1px;
}
.menu a {
display:block;
padding:2px 5px;
color:#000;
background:#b0c23d;
text-decoration:none;
}
.menu a:hover {background:#d9dcb0;}
.menu ul li {padding-left:15px;}
.menu ul a {background:#ced174;}
.hidden {display:none;}I removed the font declarations since font-family and font-size are normally specified elsewhere in the CSS. While I was at it I also changed the colours to make looking at the menu a bit easier on my eyes, and added some padding and a bottom margin to the links.
The JavaScript
Finally a look at the JavaScript. The article uses the following (variable names translated from Swedish):
function toggle(submenu) {
if (document.getElementById(submenu).style.display == "none")
document.getElementById(submenu).style.display = "block";
else if (document.getElementById(submenu).style.display == "block")
document.getElementById(submenu).style.display = "none"
else alert("An error occured in the menu. Contact the web master.");
}The problems with this script are:
It doesn't check if the browser supports the method it uses.
It works by directly changing the element style.
If anything should go wrong it displays a completely useless message.Ok, I'll admit that the script does have an advantage over my replacement: it is a lot shorter than this:
var toggleMenu = {
init : function(sContainerClass, sHiddenClass) {
if (!document.getElementById || !document.createTextNode) {return;} // Check for DOM support
var arrMenus = this.getElementsByClassName(document, 'ul', sContainerClass); // Find all menus
var arrSubMenus, oSubMenu, oLink;
for (var i = 0; i < arrMenus.length; i++) { // In each menu...
arrSubMenus = arrMenus[i].getElementsByTagName('ul'); // ...find all sub menus
for (var j = 0; j < arrSubMenus.length; j++) { // For each sub menu...
oSubMenu = arrSubMenus[j];
oLink = oSubMenu.parentNode.getElementsByTagName('a')[0]; // ...find the link that toggles it...
oLink.onclick = function(){toggleMenu.toggle(this.parentNode.getEl ementsByTagName('ul')[0], sHiddenClass); return false;} // ... add an event handler to the link...
this.toggle(oSubMenu, sHiddenClass); // ... and hide the sub menu
}
}
},
toggle : function(el, sHiddenClass) {
var oRegExp = new RegExp("(^|\s)" + sHiddenClass + "(\s|$)");
el.className = (oRegExp.test(el.className)) ? el.className.replace(oRegExp, '') : el.className + ' ' + sHiddenClass; // Add or remove the class name that hides the element
},
/* addEvent and getElementsByClassName functions omitted for brevity */
};
// Initialise the menu
toggleMenu.addEvent(window, 'load', function(){toggleMenu.init('menu','hidden');});The replacement script works by adding or removing a class name instead of directly changing the style property of the sub menu elements. It
The addEvent() and getElementsByClassName() functions are included in the full togglemenu.js (http://www.456bereastreet.com/lab/accessible-expanding-collapsing-menu/togglemenu.js) script. If you use a JavaScript library that contains similar functions (they all do, probably) I suggest using those functions instead to avoid code bloat.
You can try it all put together on the Accessible expanding and collapsing menu demo page (http://www.456bereastreet.com/lab/accessible-expanding-collapsing-menu/).
Caveats and some homework for the reader
Like I said at the beginning of this article, this has been done before. But hopefully showing the before and after versions of the HTML, CSS, and JavaScript will make it easier to see why I made the changes.
I should also note that this script does inherit a problem from the script published in the magazine: it disables the links to the top level category pages, preventing the user from navigating to them. That could be a serious problem depending on how the site is structured.
This script also does not handle multiple levels. Extending it to allow for that is no big deal, but I'd think twice before doing so. Remember that there will be a lot of links for people with JavaScript disabled to wade through.
I'll leave it as a learning exercise for the reader to alter the script so that it inserts a separate link for expanding and collapsing each sub menu and lets the top level links behave normally. And, if you want the functionality, add support for multiple levels.
Visit site to read or post comments… (http://www.456bereastreet.com/archive/200705/accessible_expanding_and_collapsing_menu/#comments)Add 456 Berea Street to your Technorati favorites. (http://technorati.com/faves?add=http://www.456bereastreet.com)
Posted in JavaScript (http://www.456bereastreet.com/archive/categories/javascript/).
http://feeds.feedburner.com/~a/456bereastreet?i=84oScK</img> (http://feeds.feedburner.com/~a/456bereastreet?a=84oScK)
http://feeds.feedburner.com/~f/456bereastreet?i=e3i4YDko</img> (http://feeds.feedburner.com/~f/456bereastreet?a=e3i4YDko) http://feeds.feedburner.com/~f/456bereastreet?i=fQeioUvV</img> (http://feeds.feedburner.com/~f/456bereastreet?a=fQeioUvV) http://feeds.feedburner.com/~f/456bereastreet?i=MqXA7YXY</img> (http://feeds.feedburner.com/~f/456bereastreet?a=MqXA7YXY)
More... (http://www.456bereastreet.com/archive/200705/accessible_expanding_and_collapsing_menu/)