7

Have following listener for keyboard ArrowDown event(it's key code is 40):

window.onload = function() {    
var itemsContainer = document.getElementById('cities-drop');
document.addEventListener('keyup',function(event){
if (event.keyCode == 40 && itemsContainer.style.display=='block') {
event.preventDefault();
    for (var i=0;i<itemsContainer.children.length-1;i++){
        if (itemsContainer.getAttribute('class').substr('hovered')!=-1){
            itemsContainer.children[i].setAttribute('class','');
            itemsContainer.children[i].nextSibling.setAttribute('class','hovered');
                //break;
                }
            }
        }
    });

in this case hovering jumps to last element in list, after ArrowDown pressed.

In case break is uncommented,it jumps to the second element and doesn't jumping any more.

Can't get the principle,how to do, that listener always listens...

EDIT live demo
perhaps,it's a matter of closure,but i'm not certain

sergionni
  • 13,290
  • 42
  • 132
  • 189

3 Answers3

4

After looking at your code and realizing what you're trying to do, I think your error is using substr where you should be using indexOf. Here is the updated line:

if (itemsContainer.getAttribute('class').indexOf('hovered') != -1)


More detail: What you were actually doing was using a substr with a string value for the start index. Not sure what the result of that would be, but apparently it's not -1, since the conditional was returning true every time, causing the next element to be hovered EVERY time, all the way down to the bottom of the list. With the break statement in there, it was executing the if-statement at the first element (causing the second element to be 'hovered'), and then quitting.

I would leave the break statement in there after correcting your code, so that the loop stops after it finds its match, and doesn't loop through the rest of the items unnecessarily.

EDIT:

I found a couple other issues in your code as well. Here is an example that works for me in IE and FF, at least (haven't tested in Safari, Opera or Chrome):

<html>
<head>
    <style type="text/css">
        .hovered
        {
            color:red;
        }
    </style>
    <script type="text/JavaScript">
        function move(event)
        {
            var itemsContainer = document.getElementById('cities-drop');
            if (event.keyCode == 40 && itemsContainer.style.display == 'block')
            {
                if (event.preventDefault)
                    event.preventDefault();
                if (event.cancelBubble)
                    event.cancelBubble();
                if (event.stopImmediatePropagation)
                    event.stopImmediatePropagation();

                for (var i=0; i<itemsContainer.children.length-1; i++)
                {
                    if (itemsContainer.children[i].className.indexOf('hovered') != -1)
                    {
                        itemsContainer.children[i].className = "";
                        itemsContainer.children[i+1].className = "hovered";
                        break;
                    }
                }
            }
        };
    </script>
</head>
<body onkeydown="move(event)">
    <div id="cities-drop" style="display:block;">
        <p class="hovered">Item 1</p>
        <p>Item 2</p>
        <p>Item 3</p>
        <p>Item 4</p>
        <p>Item 5</p>
        <p>Item 6</p>
        <p>Item 7</p>
    </div>
</body>
</html>

Detail: For me, in IE9, the function was never being called. Instead, I just made it a regular function and added an onkeydown event to the body tag.

Next, for cross-browser compatibility, you should check to make sure that event.preventDefault exists before using it. I was getting a JS error in IE.

In your if-statement, you had itemsContainer.getAttribute('class'). Firstly, you needed to use itemsContainer.children[i]. Secondly, .getAttribute('class') didn't work for me in IE, so I switched it to just .className.

Lastly, itemsContainer.children[i].nextSibling didn't work for me, but it's simple enough to just change it to itemsContainer.children[i+1] to get the next sibling.

Travesty3
  • 14,351
  • 6
  • 61
  • 98
  • yes,it's my dummy mistake,I used `substr` uncorrectly.Anyway this didn't help me,neither `indexOf()`,nor `match()`, http://javascript.info/play/pNQ9E – sergionni Feb 01 '12 at 16:42
  • sorry for late response.It works for me in that edition ,that you written here.From other side, I needn't inline observers `onkeydown=move(event)`,so when I try with `addEventListener`,i'll give you fedback – sergionni Feb 04 '12 at 17:14
  • @sergionni: Please keep in mind that `addEventListener` is not supported in IE, which is still one of the most common browsers used. You will get a Javascript error of `Object doesn't support this property or method` in Internet Explorer. If you must use this method, you should perform a check to see if the function exists first. See http://stackoverflow.com/a/1695383 for an example. – Travesty3 Feb 06 '12 at 01:31
  • yes, I know about `attachEvent` and `addEventListener`.It's not an issue. The main issue is that I need listeners at all,instead of "inline observers". – sergionni Feb 06 '12 at 09:07
  • @sergionni: I'm confused as to what you mean by "inline observer"...attaching an event using the inline method will still perform the function the same way as using `addEventListener`. The only difference that I know of is that if you attempt to add another action to the same event with the inline method, the previous action will be overwritten, whereas with `addEventListener`, both actions will execute. So the inline method is still an event listener, just the same. – Travesty3 Feb 06 '12 at 14:21
  • I mean,that I don't want to use construction such as `onkeydown="move(event)"` in my HTML.`addEventListener` is more flexible – sergionni Feb 06 '12 at 16:25
  • @sergionni: Fair enough. Would you mind accepting the answer before the bounty expires? I can still help in chat or something if you need further help beyond this. Thanks. – Travesty3 Feb 07 '12 at 15:44
  • hi,let's talk later in chat.anyway,you answer is the closest))) – sergionni Feb 09 '12 at 13:33
3

There are a few things I can see that might be a problem. First of all, you update itemsContainer.children[i].nextSibling which is itemsContainer.children[i+1]. That's why always the last element it selected if you skip the break. itemsComtainer[i+1] will always be hovered if there is on item matching the class.

Second problem is what Travesty3 is pointing out in his answer.

I also changed the if condition to check if the hovered class is on one of the children and not the container itself.

if (itemsContainer.children[i].getAttribute('class').match('hovered'))

I've modified the event handler with the following lines of code, and this seems to work fine:

document.addEventListener('keyup',function(event){
            if (event.keyCode === 40 && itemsContainer.style.display==='block') {
                event.preventDefault();
                for (var i=0,l=itemsContainer.children.length;i<l;++i){
                    if (itemsContainer.children[i].getAttribute('class').match('hovered')){
                        itemsContainer.children[i].setAttribute('class','');
                        itemsContainer.children[i+1].setAttribute('class','hovered');
                        break;
                    }
                }
            }
        });

Keep in mind that making such a drop down control is quite a lot of work. The user expects to navigate it using the keyboard. To make a great user experience, you should handle a number of keys, such as the arrow keys, tab to focus the control, space to open and close it, alpha-numeric input to focus on first matched element, etc.

If user experience is important, I'd recommend using a framework and a plugin for this. I personally prefer jquery and jquery ui, and there are a number of plugins for select drop downs. Another advantage is that if javascript is disabled by the client, or your javascript would err for some reason, most plugins fall back to a regular native select element, which would still work fine functionally.

I have used this plugin for a selectbox myself for a simple drop down: http://www.abeautifulsite.net/blog/2011/01/jquery-selectbox-plugin/

Edit: I'm revoking this recommendation, as it doesn't work if multiple elements have the same name. If this is important, you should check out the Filament Group's selectmenu plugin instead: http://filamentgroup.com/lab/jquery_ui_selectmenu_an_aria_accessible_plugin_for_styling_a_html_select/ //Edit

...and the jquery autocomplete plugin for a combobox also supporting written input: http://jqueryui.com/demos/autocomplete/

Jørgen
  • 8,820
  • 9
  • 47
  • 67
  • It seems like you forgot to update the if statement: if (itemsContainer.children[i].getAttribute('class').match('hovered')){ – Jørgen Feb 01 '12 at 18:25
3

You can try a simpler approach instead of using a loop:

window.onload = function() {    
    var itemsContainer = document.getElementById('cities-drop');

    document.addEventListener('keyup',function(event) {
        if (event.keyCode == 40 && itemsContainer.style.display=='block') {
            event.preventDefault();

            var previousHoveredChoice = itemsContainer.querySelector('.hovered');
            previousHoveredChoice.className = '';

            var currentHoveredChoice = previousHoveredChoice.nextSibling;
            if (currentHoveredChoice) {
                currentHoveredChoice.className = 'hovered';
            }
        }
    });

    //following code is copy-pasted from the live example 
    //just to close the onload function handler in this solution
    document.addEventListener('keyup',function(event){
        if (event.keyCode == 27) {

            if (document.getElementById('cities-drop').style.display=='block'){
                document.getElementById('cities-drop').style.display='none';
            }
        }

    });
    //end of copy-pasted code
};
Dimitar Bonev
  • 3,326
  • 1
  • 15
  • 16