Closed Bug 953658 Opened 8 years ago Closed 8 years ago

Groups not collapsable/expandable by keyboard

Categories

(Instantbird :: Contacts window, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: benediktp, Assigned: romain)

Details

Attachments

(1 file, 2 obsolete files)

*** Original post on bio 212 at 2009-08-12 12:18:00 UTC ***

The groups can't be collapsed/expanded using the keyboard only as it seems.

I couldn't find a handler in the code, so I think it's not possible at all right now -> bug.
OS: Windows XP → All
Hardware: x86 → All
Attached patch patch1 (obsolete) — Splinter Review
*** Original post on bio 212 as attmnt 199 by pvdeejay AT gmail.com at 2009-08-17 18:05:00 UTC ***

The functionality is implemented by this patch.
Other enhancements can be made, that assistive technologies get an idea on the state of the group item and level an item is nested in but it's beyond this bug I believe.
Attached patch patch2 (obsolete) — Splinter Review
*** Original post on bio 212 as attmnt 200 by pvdeejay AT gmail.com at 2009-08-18 04:42:00 UTC ***

I haven't mentioned the keys used to control group expanding / collapsing buddy list groups.
Originally I have bound left and right arrow keys. Later on we have further discussed ability to use numpad plus and numpad minus in the IRC channel.
I have also added enter key.
Numpad keys don't work on my system so it needs to be tested.
Comment on attachment 8351943 [details] [diff] [review]
patch1

*** Original change on bio 212 attmnt 199 by pvdeejay AT gmail.com at 2009-08-18 04:42:51 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351943 - Attachment is obsolete: true
*** Original post on bio 212 at 2009-08-18 10:10:57 UTC ***

I changed your code as follows. The switch/case wasn't practical since + and - are treated as normal characters and their value is not stored in aEvent.keyCode but aEvent.charCode. 
By the way it doesn't distinguish between normal + or - keys and keypad. Maybe someone could create a diff off it (The diffs I create here on Windows are strange..)


  // Handle key pressing
  keyPress: function bl_keyPress(aEvent) {
    var item = document.getElementById("buddylistbox").selectedItem;    
    if ( aEvent.keyCode == aEvent.DOM_VK_RETURN || 
         aEvent.keyCode == aEvent.DOM_VK_ENTER ){
        // If Enter or Return is pressed, open a new conversation
        if ( item.localName == "buddy" )
          item.openConversation();
        else
          item.close();
    }
    // 45 is charCode for -  
    else if ( aEvent.keyCode == aEvent.DOM_VK_LEFT || 
              aEvent.charCode == 45 ){
      if ( item.localName == "group" && !item.hasAttribute("closed") )
          item.close();
    }
    // 43 is charCode for +
    else if ( aEvent.keyCode == aEvent.DOM_VK_RIGHT ||
              aEvent.charCode == 43 ){
      if ( item.localName == "group" && item.hasAttribute("closed") )
           item.close();
    }
    return;
  },
*** Original post on bio 212 at 2009-08-18 10:39:35 UTC ***

Just noticed that I missed the check for the DOM_VK values, just in case that it works on any platform.

   // Handle key pressing
   keyPress: function bl_keyPress(aEvent) {
     var item = document.getElementById("buddylistbox").selectedItem;    
     if ( aEvent.keyCode == aEvent.DOM_VK_RETURN || 
          aEvent.keyCode == aEvent.DOM_VK_ENTER ){
       // If Enter or Return is pressed, open a new conversation
       if ( item.localName == "buddy" )
         item.openConversation();
       else
         item.close();
     }
     // 45 is charCode for -  
     else if ( aEvent.keyCode == aEvent.DOM_VK_LEFT ||
               aEvent.keyCode == aEvent.DOM_VK_SUBTRACT || 
               aEvent.charCode == 45 ){
       if ( item.localName == "group" && !item.hasAttribute("closed") )
           item.close();
     }
     // 43 is charCode for +
     else if ( aEvent.keyCode == aEvent.DOM_VK_RIGHT ||
               aEvent.keyCode == aEvent.DOM_VK_ADD ||
               aEvent.charCode == 43 ){
       if ( item.localName == "group" && item.hasAttribute("closed") )
            item.close();
     }
     return;
   },
*** Original post on bio 212 at 2009-08-18 11:27:17 UTC ***

Given http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/tree.xml#709 , I think we should drop out any + or - support.

Moreover VK_ADD and VK_SUBTRACT doesn't work on Linux either, so it seems that we don't have a way to distinguish between numpad_+ and +
I prefer switch case implementation here, it is more readable (with an additional line break after "break;" instructions).
Attached patch proposed patchSplinter Review
*** Original post on bio 212 as attmnt 202 at 2009-08-18 22:36:00 UTC ***

Adding extra line breaks, removing VK_ADD and VK_SUBTRACT, and adding a test on localName when the key is ENTER or RETURN (in case we implement other things that buddy and group in the future).
Attachment #8351946 - Flags: review?(florian)
Comment on attachment 8351944 [details] [diff] [review]
patch2

*** Original change on bio 212 attmnt 200 at 2009-08-18 22:36:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351944 - Attachment is obsolete: true
Assignee: nobody → bugzilla
Status: NEW → ASSIGNED
Comment on attachment 8351946 [details] [diff] [review]
proposed patch

*** Original change on bio 212 attmnt 202 at 2009-08-21 10:39:58 UTC ***

This looks good.
By the way, why do we have a 'return;' at the end of the method?
Attachment #8351946 - Flags: review?(florian) → review+
*** Original post on bio 212 at 2009-08-21 21:19:23 UTC ***

I edited the patch a bit before pushing it so that the method doesn't cause JavaScript errors when there's no selected item (for example because the buddy list is empty).

Pushed https://hg.instantbird.org/instantbird/rev/58f060743fb2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.2b1
*** This had the wrong assignee on BIO and failed to properly import into BMO. Reassigning. ***
Assignee: bugzilla → romain
You need to log in before you can comment on or make changes to this bug.