Javascript accordion-style works but is UGLY

JavaScript
1

Hallo all,
I have some working code (well, no focus yet but I’m assuming if the code is cleaned up this will help me implement focus), something I was rewriting after getting to the accordion portion of the Javascript Live course and going through the version in Simply Javascript (Events chapters). Mine is more complicated than the SJ example because I have two sets of rules going on plus an extra conditional behaviour.

html (temp)
http://stommepoes.nl/jstest/shlinks.html

CSS in the same folder
http://stommepoes.nl/jstest/shl.css

JS in the same folder:
modified-from-Core functions at http://stommepoes.nl/jstest/funclib.js
page js: http://stommepoes.nl/jstest/shl.js

Posted main JS file:


/*shl link lijst expand/collapse*/

var LinkLijst = {
    init: function() {
  //collapse everything onload
        var mainUL = document.getElementById('beschrijflinks');
        var mainLIs = mainUL.childNodes;
        for(i=0, ii=mainLIs.length; i<ii; i++) {
            if(mainLIs[i].nodeType == 1) {
                var mainLI = mainLIs[i]; //reduce variable lookups
                LinkLijst.collapse(mainLI);

                var subULs = mainLI.getElementsByTagName('ul');
                for(j=0, jj=subULs.length; j<jj; j++) {
                    var subUL = subULs[j];
                    LinkLijst.collapse(subUL);
                }
            }
        }
//add anchors to headers onload
        var h2s = mainUL.getElementsByTagName('h2'),
            h3s = mainUL.getElementsByTagName('h3');
        for(k=0, kk=h2s.length; k<kk; k++) {
            var anchor = Basis.createAnchor('+', 'a_'+k);
            h2s[k].insertBefore(anchor, h2s[k].firstChild);

            Basis.addEventListener(anchor, 'click', LinkLijst.mainClickListener);
           // Basis.addEventListener(anchor, 'focus', LinkLijst.focusListener);
         }
        for(m=0, mm=h3s.length; m<mm; m++) {
            var anchor = Basis.createAnchor('+', 'a_'+m);
            h3s[m].insertBefore(anchor, h3s[m].firstChild);

            Basis.addEventListener(anchor, 'click', LinkLijst.subClickListener);
          //  Basis.addEventListener(anchor, 'focus', LinkLijst.focusListener);
         }
    }, //init
    
    collapse: function(chunk) {
        Basis.addClass(chunk, 'collapse');
    },

    expand: function(chunk) {
        Basis.removeClass(chunk, 'collapse');
    },

    expandAll: function(chunk) { 
        LinkLijst.expand(chunk);
        var alink = Basis.getPrevSibling(chunk).firstChild;
        alink.firstChild.nodeValue = '-'; 
    },

    collapseAll: function(chunk) {
        LinkLijst.collapse(chunk);
        var alink = Basis.getPrevSibling(chunk).firstChild;
        alink.firstChild.nodeValue = '+'; 
 
    },

    mainClickListener: function(event) {
        var anchor = this,
            mainLI = this.parentNode.parentNode;
        
        if (Basis.hasClass(mainLI, 'collapse')) {
            LinkLijst.expand(mainLI);
            anchor.firstChild.nodeValue = '-';
        }
        else {
            LinkLijst.collapse(mainLI);
            anchor.firstChild.nodeValue = '+';
        }
        Basis.preventDefault(event);
    },

    subClickListener: function(event) {
        var anchor = this,
            h3 = anchor.parentNode,
            div = h3.parentNode,
            subs = div.getElementsByTagName('ul'),
            subUL = Basis.getNextSibling(h3);

        if(Basis.hasClass(subUL, 'collapse')) {
            if(h3.childNodes[1].nodeValue == 'Algemeen') {

                for(p=0, pp=subs.length; p<pp; p++) {
                    LinkLijst.expandAll(subs[p]);
                }
            }
            else {
                LinkLijst.expand(subUL);
                anchor.firstChild.nodeValue = '-';
            }
        }
        else {
            if(h3.childNodes[1].nodeValue == 'Algemeen') {
                for(p=0, pp=subs.length; p<pp; p++) {
                    LinkLijst.collapseAll(subs[p]);
                }
            }
            else {
                LinkLijst.collapse(subUL);
                anchor.firstChild.nodeValue = '+';
            }
        }
        Basis.preventDefault(event);
    },

    focusListener: function(event) {
        var anchor = this;
        if(anchor.parentNode.nodeName == 'H2') {
            var mainLI = anchor.parentNode.parentNode;
            if (Basis.hasClass(mainLI, 'collapse')) {
                LinkLijst.expand(mainLI);
                anchor.firstChild.nodeValue = '-';
            }
        }
        else if(anchor.parentNode.nodeName == 'H3') {
            var h3 = anchor.parentNode,
                subUL = Basis.getNextSibling(h3);
            if (Basis.hasClass(subUL, 'collapse')) {
                LinkLijst.expand(subUL);
                anchor.firstChild.nodeValue = '-';
            }
        } 
    }
};


Basis.begin(LinkLijst);

In the Simply Javascript version, I believe the reason focus doesn’t interfere with click is because he loops through (existing) anchors and starts at 1 for his counter instead of 0… but I’m not entirely sure.
I’m looping through header tags and adding anchors… so not sure how to stop focus from interfering with click when doing it this way.

Mostly though I want to know how I can optimise and better write this. The subClickListener has a whole lotta vars listed, and I list them often because listing vars is done inside for loops so I’m repeating myself a lot.

I think expand and collapse should also be dealing with the anchor text as well, but the problem is that those functions could be called by different thises which changes who the element is getting the “collapse” class.

Expand and collapse do the same thing on different elements. Does it make sense to try to get one Listener to use these expand/collapse functions instead of two listeners??

At one point I had looping through two nodeLists (the h2’s and the h3’s) and giving them all just one clickListener, but then there was a lot of iffing around before assigning the functions.

Anyway I would just like some feedback on this code. It works, so the behaviour you see is what is stated in my instructions. Possibly better structured HTML would help?

note
Please don’t mention switching to libraries. I’m learning to write real JS, and the Javascript Live course already showed a BIG discrepancy between how jQuery grabs elementsByTagName versus vanilla Javascript! (jQuery seems to maybe make a real array instead of a nodeList?) I want to improve my vanilla, please.

Thanks in advance,
poes

2

Good to know, I won’t use it in the future. I’m surprised, though, as it’s oft-quoted as a method for merging nodeLists.

It’s very possible something else I did caused IE to have issue with it (though using your code straight also made the error). The only thing I ran into when looking around for “merging two nodeLists” was another StackOverflow page where someone said IE5 (!) didn’t like it without the .prototype method added on (which suggested other IE’s worked) and then a comment from someone later posting the same error I got, in all three of my IE’s (object expected). So, I still kinda wonder if it was something in the execution.

I know practically nothing about screen readers. How do they deal with things like suckerfish menus?

if the suckerfish uses the off-screen method (pull the submenu offscreen with -bazillion units or something) it’s as if the whole list is onscreen like it is in the markup.

If you use display: none, however, the submenu can just be gone entirely, making it inaccessible.

The Big 2 commercial readers for Windows use a virtual buffer that copies the page, and the user has extra tools and keys with which to interact with the copied elements. Older readers didn’t update that buffer unless there was a page refresh, making on-the-fly changes and ajaxy stuff pretty iffy. It still is, if you don’t do it right, but newer versions of the Big 2 readers are now more JS-aware. Thing is, readers are expensive so folks may keep the old ones for a long time, plus this may restrict them to older browsers too.
One of the few reasons I still work a bit at making IE6 better than just barely functional… I couldn’t get JAWS 7 to work well with anything not IE6.

Anyway this is why I initially had the .collapse class styled to abso-po certain elements and pull them offscreen… but yeah that reintroduces the focus/keyboard issue, bleh.

It’s not this, it’s the appendChild that does this. It’s not just for adding new things to nodes, it can also be used as a “moveNode” method.

Aahhh, ok. I had just noticed as a stepped through the code that my h3’s were vanishing and I realised they were being removed from the DOM. I thought it had to do with creating the fragment and adding stuff to it.

You don’t need to specify the length initially when you create an array, and you can just use the “literal” notation to create it. It’s a bit like creating other objects

I thought maybe the new Array knowing how long it was would help speed things up as it went through both loops, wasn’t sure. Normally I like the declaration myself, esp after reading that often using “new” itself was a performance issue (and Crockford had some issue with it but I don’t remember exactly what).

3

IE still complains regularly over the Array.prototype.slice.call thing
Good to know, I won’t use it in the future. I’m surprised, though, as it’s oft-quoted as a method for merging nodeLists.

I didn’t know putting stuff in a document fragment actually removed the contents from the page entirely
It’s not this, it’s the appendChild that does this. It’s not just for adding new things to nodes, it can also be used as a “moveNode” method. You could achieve the same by cloning the node, then appending it to the new location and finally removing the original node. But just using appendChild is quicker.

A documentFragment is a bit like a mini document object, to which you can append other elements. It’s just so that you can build a little bit of a DOM tree before appending it to the actual DOM. It avoids having to create an extra DIV (or something else) to append stuff to before you insert it in the DOM.

I’m a little uneasy about how a non-updating screen reader would deal with this
I know practically nothing about screen readers. How do they deal with things like suckerfish menus? Perhaps you do need the collapse styles to trick the screen reader into thinking that we’re just changing the CSS display state (much more common) and not actually adding/removing stuff on the fly.

A final little tip:

headers = new Array(h2s.length + h3s.length);

// or:

headers = [];

You don’t need to specify the length initially when you create an array, and you can just use the “literal” notation to create it. It’s a bit like creating other objects:

var foo = new String('bar');
var foo = 'bar';

Might as well use the less verbose versions.

I learned a lot from it
That’s the main thing. :slight_smile:

4

Using the second version of your code, I thought "well I can get around this by trying the old loop-concat method (looping through h2’s and h3’s at once with a placeholder variable as posted earlier), but while the first time I did this (with my old code) everyone did indeed get looped through, when I try to implement it Raffles#2 code, it seems to skip the h3’s entirely. I can’t “see” why, and thought maybe the LinkLijst.collapse line was doing it, but not sure.

Aha. The loop-concat method still has nodeLists… as “collapse” was added to the h2’s (I hadn’t thought of that, you just targetted everyone’s nextSibling), the h3’s were taken from the DOM and that updated. So, I really did need a real array. And best I could do was the slow make-an-array deal, but it does work. IE still complains regularly over the Array.prototype.slice.call thing, it must be one of the newer Mozilla things that everyone else picked up.

So, mostly still Raffles2 code lawlz, but I didn’t know putting stuff in a document fragment actually removed the contents from the page entirely… this means that my CSS doesn’t use or need the collapse classes or styles. I’m a little uneasy about how a non-updating screen reader would deal with this (assuming they’ve got JS on which, like most people, they seem to), though the keyboard access helps.

So, for posterity, the code:


var LinkLijst = {
    init: function() {
        var mainUL = document.getElementById('beschrijflinks'),
            h2s = mainUL.getElementsByTagName('h2'),
            h3s = mainUL.getElementsByTagName('h3'),
            anchor;

        var index = 0, 
            headers = new Array(h2s.length + h3s.length);
        for (var j=0; j<h2s.length; j++) {
            headers[index++] = h2s[j];
        }
        for (var k=0; k<h3s.length; k++) { 
            headers[index++] = h3s[k];
        }

        for (var i=0, ii=headers.length;  i<ii; i++) {
            LinkLijst.collapse(headers[i]);
            anchor = Basis.createAnchor('+', 'a_'+i);
            headers[i].insertBefore(anchor, headers[i].firstChild);

            Basis.addEventListener(anchor, 'click', LinkLijst.listener);
        }
    }, 
    
    collapse: function(heading) {
        heading.sib = document.createDocumentFragment();
        heading.sib.appendChild(Basis.getNextSibling(heading));
        Basis.addClass(heading, 'collapse');
    },

    expand: function(heading) {
        heading.parentNode.insertBefore(heading.sib, Basis.getNextSibling(heading));
        Basis.removeClass(heading, 'collapse');
    },

    listener: function(event) {
        var anchor = this,
            heading = this.parentNode;
        if (event && event.type === 'focus') {
            LinkLijst.timer = setTimeout(function() {
                LinkLijst.listener.call(anchor);
            }, 80);
            return;
        }
        clearTimeout(LinkLijst.timer);

        var action = Basis.hasClass(heading, 'collapse') ? 'expand' : 'collapse';
        LinkLijst.swapSign(anchor, action);
        if (heading.nodeName === 'H3' && heading.lastChild.nodeValue.toLowerCase() === 'algemeen') {
            var nod = heading;
            while (nod) {
                if (nod.nodeName === 'UL' || action === 'collapse' && Basis.hasClass(nod, 'collapse')) {
                    nod = Basis.getNextSibling(nod);
                    continue;
                }
                LinkLijst[action](nod);
                LinkLijst.swapSign(nod.firstChild, action);
                nod = Basis.getNextSibling(nod);
            }
        }
        else {
            LinkLijst[action](heading);
        }
        if (event) { 
            Basis.preventDefault(event);
        }
    },

    swapSign: function(anchor, action) {
        anchor.firstChild.nodeValue = action === 'expand' ? '-' : '+';
    }
};

Basis.begin(LinkLijst);

I could have pushed the headers into the Array, I dunno if it matters which method is used. I also added a toLowerCase for the text since someone, somewhere will goof up the caps.

Anyway, thanks for the help and the code. I’m not sure I could just write like this out of the blue, but I learned a lot from it, and have something to look back to in future code. : )

5

Well, I’ve hit a roadblock. In the weekend I tried various things: flat-out copy-pasta-ing your code (both versions), and it seems that IE does NOT like that Array.protortype.slice.call. “JScript expects an object” there type error. As expected, everyone else seems fine : )

Switching to Raffles#1, IE8 bothered to give me the error and strangely IE7 acted like IE8 (stopped running the JS) but didn’t actually give me the little error warning. Dunno why, but I think it’s still hitting that Array issue. Raffles#2 gives the error in both browsers… I didn’t bother with IE6 at this point (save for later).

Using the second version of your code, I thought "well I can get around this by trying the old loop-concat method (looping through h2’s and h3’s at once with a placeholder variable as posted earlier), but while the first time I did this (with my old code) everyone did indeed get looped through, when I try to implement it Raffles#2 code, it seems to skip the h3’s entirely. I can’t “see” why, and thought maybe the LinkLijst.collapse line was doing it, but not sure.

So I’m going to see what I get going back to my old code (where I did get looping in one loop through all headers) and see if I can then go backwards and implement bits of your code. Should be a Good Thing anyway because while I’ve read about .call and Documentfragment(), I’ve never worked with them and they are slightly mysterious. Ideally I come out of this without any mysterious (to me) code : )

I’m quite happy with the focus code you wrote, playing around with setTimeout is fun.

6

That’s a good idea. I hadn’t thought of that. Still, the Array.prototype.slice.call thing should work in IE as well.

I didn’t know if it was better to continue referring to this as this if I first give it a name (anchor) or if it didn’t matter either way.
Not too important really. The only time you need to store this in a variable is if you’re going to introduce a nested closure with its own this and you want to use the parent this (like with a setTimeout - see code I posted at the bottom of this post).

this confused me:
if (nod.nodeName === ‘UL’) LinkLijstaction;
action isn’t an array value of Linklijst…?
This is where JavaScript is very nice. You can refer to an object’s property using dot or bracket notation. So, these are all equivalent:

var action = 'expand';
LinkLijst[action](nod);

LinkLijst['expand'](nod);

LinkLijst.expand(nod);

It means you have a kind of “variable function call”.

I thought it would just quit if there was no next sibling, because if there wasn’t one, then sib=target.nextSibling I thought would just be null.
Yes, but even if it’s null the check for sib.nodeType is still performed to ascertain whether to continue with the current iteration or stop the loop, and that’s what throws the error (sib.nodeType is undefined). That’s why checking that sib is actually “something” first is necessary.

I get the (if true) (?either) (dothis) (:or) (dothat) but the
var something = something === before that, I also couldn’t read in my husband’s code.

   
 var signNode = this.firstChild;
    signNode.nodeValue = signNode.nodeValue == '+' ? '-' : '+';

there this was an anchor. I didn’t get the signNode.nodevalue = signNode.nodeValue == //rest of it
so I ended up writing longer if-else statements.
It means “find out if signNode.nodeValue equals ‘+’ (signNode.nodeValue == '+' ?). If so, set signNode.nodeValue to ‘-’. Otherwise (the : '+' bit), set it to ‘+’.”

Or, “set signNode.nodeValue to the result of the ternary operation”.

OK, now to solve the focus issue, I think I came up with a nice solution. We simply temporarily remove the siblings of the headings until they are ready to be shown. We store them as properties of the heading Elements. That way you can tab through only links that are actually present in the DOM. When you press tab, it only cycles through the ‘+’ signs that are showing, and you open the items with “enter”.

The issue with the click happening along with a focus event is solved by using a setTimeout. If the event was “click”, the function defined in setTimeout will be killed by clearing the timeout (as long as “click” happens less than 80ms after “focus”).

var LinkLijst = {
    init: function() {
        var mainUL = document.getElementById('beschrijflinks'),
            h2s = mainUL.getElementsByTagName('h2'),
            h3s = mainUL.getElementsByTagName('h3'),
            anchor,
            headings = LinkLijst.headings = Array.prototype.slice.call(h2s, 0).concat(Array.prototype.slice.call(h3s, 0));
        for(var i=0, ii=headings.length; i<ii; i++) {
          var chunk = headings[i].nodeName === 'H2' ? headings[i].parentNode : Basis.getNextSibling(headings[i]);
          LinkLijst.collapse(headings[i]);
          anchor = Basis.createAnchor('+', 'a_'+i);
          headings[i].insertBefore(anchor, headings[i].firstChild);
          Basis.addEventListener(anchor, 'click', LinkLijst.clickListener);
        }
    },

    collapse: function(heading) {
        heading.sib = document.createDocumentFragment();
        heading.sib.appendChild(Basis.getNextSibling(heading));
        Basis.addClass(heading, 'collapse');
    },

    expand: function(heading) {
        heading.parentNode.insertBefore(heading.sib, Basis.getNextSibling(heading));
        Basis.removeClass(heading, 'collapse');
    },

    clickListener: function(event) {
        var anchor = this, heading = this.parentNode;
        if (event && event.type === 'focus') {
          LinkLijst.timer = window.setTimeout(function() {
            LinkLijst.clickListener.call(anchor);
          }, 80);
          return;
        }
        window.clearTimeout(LinkLijst.timer);
        var action = Basis.hasClass(heading, 'collapse') ? 'expand' : 'collapse';
        LinkLijst.swapSign(anchor, action);
        if (heading.nodeName === 'H3' && heading.lastChild.nodeValue === 'Algemeen') {
          var nod = heading;
          while (nod) {
            if (nod.nodeName === 'UL' || action === 'collapse' && Basis.hasClass(nod, 'collapse')) {
              nod = Basis.getNextSibling(nod);
              continue;
            }
            LinkLijst[action](nod);
            LinkLijst.swapSign(nod.firstChild, action);
            nod = Basis.getNextSibling(nod);
          }
        }
        else LinkLijst[action](heading);
        if (event) Basis.preventDefault(event);
    },

    swapSign: function(anchor, action) {
      anchor.firstChild.nodeValue = action === 'expand' ? '-' : '+';
    }
};


Basis.begin(LinkLijst);

EDIT: PPK reckons the focus event firing when click fires is a bug: http://www.quirksmode.org/dom/events/index.html

Seems sensible to me. Click obviously infers focus, so I don’t want that event firing too.

7

Raff! Thanks for the replies and input!

What do you mean that focus interferes with click? The only problem I see is that you have to tab through lots of invisible links until the next “+” goes orange.

I have the focusEventLoader commented out, because when it’s in there, then the desired behaviour that I can show you with the mouse is ruined (you must click twice in at least Firefox and Opera because they focus after click… didn’t bother testing further browsers because that is unacceptable). So only if you uncomment the focus listeners at the top do you get any focus behaviour, and it does do what I want: stuff just appears when you focus on any link. It doesn’t get as fancy as the mouse.

I’ve also discovered that “Algemeen” means “general”. I can see what’s going on regarding clicking this, but it’s still unusual behaviour to expand everything else in the parent category.

Not my decision. He wants if you click the “Overal/general” link, all the other subs appear. I was so tempted to leave that out entirely as it was another level of complication in my code, but he did want it and I figured I’d better learn how to write it.

OK, there’s a fair bit of code repetition that can be avoided here. You can also simplify things a lot by merging the H2 and H3 collections into one (the Array.concat bit).

I had something originally… saw it on StackOverflow and thought I’d give it a try.


//after nodeLists of h2s and h3s is created:

for (var k=0; k<h2s.length + h3s.length; k++) {
    var header = (k<h2s.length ) ? h2s[k] : h3s[k-h2s.length];
    //inserted my anchors etc
    Basis.addEventListener(anchor, 'click', LinkLijst.clickListener);
}

So, I was looping through two node lists at once, and so they both got just one listener (clickListener), which meant my clickListener ended up having to check to see who was the parents of this. Which I thought wasn’t the right place to do it, and went back to my old code.

This Array.prototype.slice.call… works in IE as well??

I didn’t know if it was better to continue referring to this as this if I first give it a name (anchor) or if it didn’t matter either way.
this confused me:
if (nod.nodeName === ‘UL’) LinkLijstaction;
action isn’t an array value of Linklijst…?

I made a small change to the Basis.getPrevSibling and getNextSibling. At the moment they throw an error if there is no next sibling, so simply changing the “while” line to this addresses the problem

Oh, thanks. I thought it would just quit if there was no next sibling, because if there wasn’t one, then sib=target.nextSibling I thought would just be null.

Oh, and by the way, in a for loop, it’s normally a good idea to use the var keyword, otherwise you’re declaring i and ii (etc.) as globals.

Yeah I caught that later when I re-ran my code through Lint, which was after I posted it here.

Focus happens before click (when you click) so you could check if the click event is happening immediately after focus (and stop it). Still, I’m not sure it’s a good idea (you can use the main listener with focus too).

In the simpler accordion we did in JSLive some people noticed that specifically Firefox had issues (though possibly Opera simply wasn’t tested?) and the Simply Javascript book seems to get around that by looping through anchors (however, they are not creating anchors, anchors are already in the HTML) and starting their counter at 1 instead of 0. I forget why, this seemed to help by having counters off by 1 or something. Since I was creating them as I went, I couldn’t find a decent way to loop through anchors and then give them the focusListener separately. All it(focusListener) did was, every off-screen link that was focussed upon, ended up onscreen or showing whatever clicking on it would have shown. I’m really not sure how that would work in Opera, who wouldn’t tab on those invisible offscreen links in the first place… I guess Opera users would just have to click onscreen links like mouse users? Whenever I have off-screen skip links, I always think, this is an Opera bug, but for something like this accordion, it’s very nice (for a user).

I still haven’t made it all the way through the Simply Javascript book, I keep skipping around, but while I wanted to use ternary operators like my husband did in an earlier version of this code, I still don’t seem to have the syntax down for it. I get the (if true) (?either) (dothis) (:or) (dothat) but the
var something = something === before that, I also couldn’t read in my husband’s code.

   
 var signNode = this.firstChild;
    signNode.nodeValue = signNode.nodeValue == '+' ? '-' : '+';

there this was an anchor. I didn’t get the signNode.nodevalue = signNode.nodeValue == //rest of it
so I ended up writing longer if-else statements.

It does make sense to me in the “drawer” line.

8

Ok. So because they are equivalent (here at least) it was just a preference of yours, correct?

It’s more a shortcut:

var action = Basis.hasClass(heading, 'collapse') ? 'expand' : 'collapse';

// option 1
LinkLijst[action](nod);
 
//option 2
if (action === 'expand') LinkLijst.expand(nod);
else LinkLijst.collapse(nod);

Since “action” is variable, it basically creates a variable function call.

9

I’m going to have a look at this, but there’s some weird behaviour I’d like to clarify first. In any main category, clicking one of the sub-categories opens just that one, as expected. But the first one opens the category but also all the other sibling categories. I’d expect things to open only when I click them. I’d also expect the expanded items under “Sport” not to collapse automatically when “Sport” itself is collapsed (so they are restored when Sport is re-expanded).

What exactly is the desired behaviour?

10

I think if the things open automatically upon focus, it’s pretty irritating and can make a mess of the page. You’re opening sub-menus without the user asking for it. The visual effect of the “+” going orange is good I think. You could I suppose add something like this in the CSS:

h2 a:focus::after {
  content: "expand";
}

Or do it via JS.

Since setTimeout is a window method I do not actually need to say window.blah for those, correct?
That’s right.

11

OK, there’s a fair bit of code repetition that can be avoided here. You can also simplify things a lot by merging the H2 and H3 collections into one (the Array.concat bit). Admittedly it’s less readable than it was, but this could be remedied by replacing the ternary operations with if…else blocks, and perhaps declaring a few more variables (like you had before). Anyway, it works. :slight_smile:

var LinkLijst = {
    init: function() {
        var mainUL = document.getElementById('beschrijflinks'),
            h2s = mainUL.getElementsByTagName('h2'),
            h3s = mainUL.getElementsByTagName('h3'),
            headings = Array.prototype.slice.call(h2s, 0).concat(Array.prototype.slice.call(h3s, 0)),
            anchor;
        for (var i=0, ii=headings.length; i<ii; i++) {
          LinkLijst.collapse(headings[i].nodeName === 'H2' ? headings[i].parentNode : Basis.getNextSibling(headings[i]));
          anchor = Basis.createAnchor('+', 'a_'+i);
          headings[i].insertBefore(anchor, headings[i].firstChild);
          Basis.addEventListener(anchor, 'click', LinkLijst.listener);
          // Basis.addEventListener(anchor, 'focus', LinkLijst.listener);
        }
    }, //init
    
    collapse: function(chunk) {
        Basis.addClass(chunk, 'collapse');
    },

    expand: function(chunk) {
        Basis.removeClass(chunk, 'collapse');
    },

    listener: function(event) {
        var anchor = this,
            drawer = this.parentNode.nodeName === 'H2' ? this.parentNode.parentNode : Basis.getNextSibling(this.parentNode);
        var action = Basis.hasClass(drawer, 'collapse') ? 'expand' : 'collapse';
        LinkLijst.swapSign(anchor, action);
        if (this.parentNode.nodeName === 'H3' && this.parentNode.lastChild.nodeValue === 'Algemeen') {
          var nod = this.parentNode;
          while (nod) {
            if (nod.nodeName === 'UL') LinkLijst[action](nod);
            else LinkLijst.swapSign(nod.firstChild, action); // h3
            nod = Basis.getNextSibling(nod);
          }
        }
        else LinkLijst[action](drawer);
        Basis.preventDefault(event);
    },

    swapSign: function(anchor, action) {
      anchor.firstChild.nodeValue = action === 'expand' ? '-' : '+';
    },

};


Basis.begin(LinkLijst);

I made a small change to the Basis.getPrevSibling and getNextSibling. At the moment they throw an error if there is no next sibling, so simply changing the “while” line to this addresses the problem:

while(sib && sib.nodeType !== 1)

I’m not sure about focusListener. Tabbing and pressing enter works quite well, except for the tabbing to invisible links. Perhaps it’s OK for the onfocus event to expand things only when it’s the H2s receiving focus. I see now what you mean about the interference. Focus happens before click (when you click) so you could check if the click event is happening immediately after focus (and stop it). Still, I’m not sure it’s a good idea (you can use the main listener with focus too).
Oh, and by the way, in a for loop, it’s normally a good idea to use the var keyword, otherwise you’re declaring i and ii (etc.) as globals.

12

This is where JavaScript is very nice. You can refer to an object’s property using dot or bracket notation.

Ok. So because they are equivalent (here at least) it was just a preference of yours, correct?

Yes, but even if it’s null the check for sib.nodeType is still performed to ascertain whether to continue with the current iteration or stop the loop, and that’s what throws the error (sib.nodeType is undefined). That’s why checking that sib is actually “something” first is necessary.
Got it.

It means “find out if signNode.nodeValue equals ‘+’ (signNode.nodeValue == ‘+’ ?). If so, set signNode.nodeValue to ‘-’. Otherwise (the : ‘+’ bit), set it to ‘+’.”

Or, “set signNode.nodeValue to the result of the ternary operation”.

Actually, that was the part I did understand, BUT while looking at it again, I think I see what the first part is…
samevariable = samevariable == x…
is making it do the initial iffing of the ternary op.
samevariable is (if it’s now x, it’s now either x or y).

OK, now to solve the focus issue, I think I came up with a nice solution. We simply temporarily remove the siblings of the headings until they are ready to be shown. We store them as properties of the heading Elements. That way you can tab through only links that are actually present in the DOM. When you press tab, it only cycles through the ‘+’ signs that are showing, and you open the items with “enter”.

Hm, interesting. That may or may not be more intuitive for keyboarders (I’m glad I have + and - signs to help). Since setTimeout is a window method I do not actually need to say window.blah for those, correct?
This is actually good because I was going to go on to playing with SetTimeout next (for a mega dropdown delay) and I hadn’t gotten there in the JSLine or SimplyJavascript sections yet (I’d read through them and skimmed them but not actually built anything that worked with them, lawlz).

I will give all these things a go today. Thanks again for the input.

13

What do you mean that focus interferes with click? The only problem I see is that you have to tab through lots of invisible links until the next “+” goes orange. The solution to this problem, I think, is to create the links that aren’t visible only when the sub-category is expanded. And then remove them upon collapse… which is a bit of a drag to code.

I’ve also discovered that “Algemeen” means “general”. I can see what’s going on regarding clicking this, but it’s still unusual behaviour to expand everything else in the parent category.