Closed Bug 955087 Opened 10 years ago Closed 10 years ago

Improve keyboard accessibility of the contact list

Categories

(Instantbird Graveyard :: Contacts window, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

Details

Attachments

(1 file, 7 obsolete files)

*** Original post on bio 1658 at 2012-08-21 22:07:00 UTC ***

As seen in https://bugzilla.mozilla.org/show_bug.cgi?id=778709 for the status selector: makes the focus on the status selector visible (important
when tabbing through the controls of the window) and lets the user open the
status type popup with the down arrow while the status selector is focused.
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1658 as attmnt 1829 at 2012-08-22 17:07:00 UTC ***

- Makes avatar focusable and changeable via keyboard
- Remove focusability from conversations on hold when that listbox is empty
- Add focus borders to all focusable elements that didn't show their state
- The down arrow on the status selector now opens the status selection popup

Might need followups on OS != Linux (certainly for Win-aero).
Attachment #8353588 - Flags: review?(florian)
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Summary: Make focus visible on all editable elements and improve status selector keyboard accessibility → Improve keyboard accessibility of the contact list
Comment on attachment 8353588 [details] [diff] [review]
Patch

*** Original change on bio 1658 attmnt 1829 at 2012-08-29 22:10:38 UTC ***

I think I would prefer if the status type icon wasn't focusable, and using the down arrow key to open the status type popup when the status message is focused.

For the status message and the display name's focus rings, I suggest doing this for Mac:
#statusMessage:-moz-focusring:not([editing]) {
%ifdef XP_MACOSX
  margin: 30px -2px -1px;
  border: 2px solid rgba(0,0,0,0.15);
  border-radius: 5px;
%else
  margin: 31px -1px -1px;
  border: dotted 1px -moz-dialogtext;
%endif
}

#displayName:-moz-focusring:not([editing]) {
%ifdef XP_MACOSX
  margin: -1px -1px 16px -2px;
  border: 2px solid rgba(0,0,0,0.15);
  border-radius: 5px;
%else
  margin: -1px -1px 16px;
  border: dotted 1px -moz-dialogtext;
%endif
}

I first tried with -moz-focus-ring as the color but it looks really ugly above a toolbar background without a real textbox.

Mic may be able to help you for the Windows theming and/or you may be able to re-use things from https://bugzilla.mozilla.org/attachment.cgi?id=650953&action=diff
Attachment #8353588 - Flags: review?(florian) → review-
*** Original post on bio 1658 at 2012-08-29 22:39:58 UTC ***

(In reply to comment #2)

> I first tried with -moz-focus-ring as the color

I meant -moz-mac-focusring sorry.
*** Original post on bio 1658 at 2012-08-30 00:30:04 UTC ***

(In reply to comment #2)
> I think I would prefer if the status type icon wasn't focusable, and using the
> down arrow key to open the status type popup when the status message is
> focused.

I am a bit worried this will not be discoverable. Who is going to guess this (for accessibility and otherwise)? This is no problem for TB where there is a downward chevron, http://www.h-online.com/imgs/43/9/1/0/5/5/1/2012-08-24-13-05-57-5f7cfe-63b2430d93d20b8d.png
*** Original post on bio 1658 at 2012-08-30 16:03:59 UTC ***

(In reply to comment #4)
> I am a bit worried this will not be discoverable. Who is going to guess this
> (for accessibility and otherwise)?

clokep also thinks it's pretty standard. 

James, is there a way to indicate this (in effect 'you can press the down arrow to change status') via a11y flags? Or is that not necessary?
*** Original post on bio 1658 by James Teh <jamie AT nvaccess.org> at 2012-08-31 00:20:56 UTC ***

(In reply to comment #5)
> James, is there a way to indicate this (in effect 'you can press the down arrow
> to change status') via a11y flags? Or is that not necessary?
It should have an accessible role of button (in v1.2, it's link) and it should expose the hasPopup state. If this isn't happening already, you can achieve it with attributes role="button" aria-haspoup="true". Note that the way Gecko exposes this is a bit ambiguous; it's not clear whether it's a button that opens a menu when pressed or a button with a secondary menu (as in this case), but exposing this is better than nothing.
Attached patch Patch v2 (obsolete) — Splinter Review
*** Original post on bio 1658 as attmnt 1880 at 2012-09-01 12:59:00 UTC ***

OK, so this 
- adds/removes the a11y roles when appropriate
- changes the status popup in the way flo suggested
- changes the status message focus ring to incorporate the statustype icon to match this behaviour.
- ensures (when possible) that there is always a listitem selected when tabbing through the contact list via the keyboard. (I did not want to do this generally whenever the contact list has focus because it looks ugly.)
Attachment #8353638 - Flags: review?(florian)
Attachment #8353638 - Flags: feedback?(bugzilla)
Comment on attachment 8353588 [details] [diff] [review]
Patch

*** Original change on bio 1658 attmnt 1829 at 2012-09-01 12:59:45 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353588 - Attachment is obsolete: true
*** Original post on bio 1658 at 2012-09-01 13:13:58 UTC ***

> Mic may be able to help you for the Windows theming and/or you may be able to
> re-use things from
> https://bugzilla.mozilla.org/attachment.cgi?id=650953&action=diff
Since I can't test this anyway, I think one might as well do this in a follow-up bug. It's not like anything will break beyond the existing brokenness while those rules are absent.
*** Original post on bio 1658 by James Teh <jamie AT nvaccess.org> at 2012-09-03 05:30:11 UTC ***

(In reply to comment #1)
> - Makes avatar focusable and changeable via keyboard
I assume this is @id="userIcon". Some issues:
1. This is unlabelled according to accessibility. Perhaps it could be given a label like "Change Icon" or similar. If you don't want this to be visible and you have no other way of doing it, use the aria-label attribute.
2. How often is this likely to be used by keyboard users? Would it make more sense to include it in the Contact context menu (thus freeing up a tab stop)? The tab order is hardly unmanageable right now, so it's not really important, but thought I'd ask.
3. Pressing down arrow on this button opens the status type menu. This doesn't make sense to me, but perhaps I'm missing something.

> - Remove focusability from conversations on hold when that listbox is empty
Nice. Works well.

Here's str for one other bug:
1. Tab to the status type button.
2. Press enter.
3. Press escape.
4. Press down arrow to open the menu.
5. Press escape to close it.
Expected: Focus should be on the status type button.
Actual: Focus is on the avatar button.
Comment on attachment 8353638 [details] [diff] [review]
Patch v2

*** Original change on bio 1658 attmnt 1880 at 2012-09-03 09:34:24 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353638 - Flags: review?(florian)
Attachment #8353638 - Flags: feedback?(bugzilla)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1658 as attmnt 1892 at 2012-09-03 10:02:00 UTC ***

Fixes the issues reported by Jamie.
Attachment #8353650 - Flags: review?(florian)
Attachment #8353650 - Flags: feedback?(bugzilla)
Comment on attachment 8353638 [details] [diff] [review]
Patch v2

*** Original change on bio 1658 attmnt 1880 at 2012-09-03 10:02:30 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353638 - Attachment is obsolete: true
Comment on attachment 8353650 [details] [diff] [review]
Patch

*** Original change on bio 1658 attmnt 1892 by jamie AT nvaccess.org at 2012-09-03 10:52:18 UTC ***

The issues I mentioned are fixed. Thanks!

Unfortunately, I discovered one more (my apologies for not spotting it the first time):
The aria-haspopup attribute should be removed (along with the role attribute) when editing the status message. Of course, it should be reinstated when editing is done.

Marking f+, as this will be great with the above fix.
Attachment #8353650 - Flags: feedback?(bugzilla) → feedback+
*** Original post on bio 1658 by James Teh <jamie AT nvaccess.org> at 2012-09-03 10:54:50 UTC ***

It's also worth noting that only enter works to activate these controls; space doesn't work. This may confuse some keyboard users, as space normally activates buttons. I'm guessing this is just the default behaviour of these particular XUL controls and it's a very minor point; I've seen other buttons that don't respond to space either, so many users may try both.
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1658 as attmnt 1893 at 2012-09-03 10:56:00 UTC ***

Right, I should have spotted that too!
Attachment #8353651 - Flags: review?(florian)
Comment on attachment 8353650 [details] [diff] [review]
Patch

*** Original change on bio 1658 attmnt 1892 at 2012-09-03 10:56:58 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353650 - Attachment is obsolete: true
Attachment #8353650 - Flags: review?(florian)
*** Original post on bio 1658 at 2012-09-03 11:00:16 UTC ***

(In reply to comment #12)
> It's also worth noting that only enter works to activate these controls; space
> doesn't work. This may confuse some keyboard users, as space normally activates
> buttons. I'm guessing this is just the default behaviour of these particular
> XUL controls and it's a very minor point; I've seen other buttons that don't
> respond to space either, so many users may try both.
Actually it could easily be added that pressing space also triggered editing the display name/status message. But I'm not sure that "feels right"? Another alternative would be to start the editing whenever the user starts typing when the element has focus.
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1658 as attmnt 1894 at 2012-09-03 12:02:00 UTC ***

It turns out the documentation for role=button explicitly states that one should add a handler for the space key to match the usual behaviour of buttons.
Attachment #8353652 - Flags: review?(florian)
Comment on attachment 8353651 [details] [diff] [review]
Patch

*** Original change on bio 1658 attmnt 1893 at 2012-09-03 12:02:48 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353651 - Attachment is obsolete: true
Attachment #8353651 - Flags: review?(florian)
Attached patch Patch (reactivated) (obsolete) — Splinter Review
*** Original post on bio 1658 as attmnt 1895 at 2012-09-03 18:53:00 UTC ***

Update OS X CSS rule (please test)

OK, hopefully this is it ;)
Attachment #8353653 - Flags: review?(florian)
Comment on attachment 8353652 [details] [diff] [review]
Patch

*** Original change on bio 1658 attmnt 1894 at 2012-09-03 18:53:15 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353652 - Attachment is obsolete: true
Attachment #8353652 - Flags: review?(florian)
Comment on attachment 8353653 [details] [diff] [review]
Patch (reactivated)

*** Original change on bio 1658 attmnt 1895 at 2012-10-02 22:06:49 UTC ***

>   statusMessageKeyPress: function bl_statusMessageKeyPress(aEvent) {

>+      case aEvent.DOM_VK_TAB:
>+        if (aEvent.shiftKey)
>+          break;
>+        // Ensure some item is selected when navigating by keyboard.
>+        if (!this.selectFirstItem("convlistbox"))
>+          this.selectFirstItem("buddylistbox");

So here you try both lists...

>+  userIconKeyPress: function bl_userIconKeyPress(aEvent) {

>+      case aEvent.DOM_VK_TAB:
>+        if (!aEvent.shiftKey)
>+          break;
>+        // Ensure a contact is selected when navigating by keyboard.
>+        this.selectFirstItem("buddylistbox");

... but here only one.

Is there a reason for this difference?

>   keyPress: function bl_keyPress(aEvent) {
>     let target = aEvent.target;
>     while (target && target.localName != "richlistbox")
>       target = target.parentNode;
>+    if (aEvent.keyCode == aEvent.DOM_VK_TAB) {
>+      // Ensure some item is selected when navigating by keyboard.
>+      if (target.id == "convlistbox" &&  !aEvent.shiftKey)

Nit: remove the duplicated space here.

I haven't tried the patch, but the code looks good otherwise.
Attachment #8353653 - Flags: review?(florian) → review-
*** Original post on bio 1658 at 2012-10-10 19:41:27 UTC ***

(In reply to comment #17)
> Comment on attachment 8353653 [details] [diff] [review] (bio-attmnt 1895) [details]
> Patch
> 
> >   statusMessageKeyPress: function bl_statusMessageKeyPress(aEvent) {
> So here you try both lists...
> ... but here only one.
> 
> Is there a reason for this difference?
TAB from the status message goes to the conversations on hold if there are any, to the contacts if not. Shift-TAB from the user icon is the reverse direction and always goes to the contacts.
*** Original post on bio 1658 at 2012-10-10 21:34:35 UTC ***

(In reply to comment #18)

> > Is there a reason for this difference?
> TAB from the status message goes to the conversations on hold if there are any,
> to the contacts if not. Shift-TAB from the user icon is the reverse direction
> and always goes to the contacts.

Is this based on the assumption that if the contact list is empty, there can't be any conversation on hold?
If so, that's frequently wrong if you put conversations on hold (or use hide auto-join), didn't check "show offline buddies" and switch to the offline status.
*** Original post on bio 1658 at 2012-10-10 21:39:41 UTC ***

(In reply to comment #19)
> If so, that's frequently wrong if you put conversations on hold (or use hide
> auto-join), didn't check "show offline buddies" and switch to the offline
> status.
Good catch. Of course offline *conversations* aren't hidden.
Attached patch Patch with fixes (obsolete) — Splinter Review
*** Original post on bio 1658 as attmnt 1944 at 2012-10-10 21:44:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353701 - Flags: review?(florian)
Comment on attachment 8353653 [details] [diff] [review]
Patch (reactivated)

*** Original change on bio 1658 attmnt 1895 at 2012-10-10 21:44:27 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353653 - Attachment is obsolete: true
Comment on attachment 8353701 [details] [diff] [review]
Patch with fixes

*** Original change on bio 1658 attmnt 1944 at 2012-10-10 23:37:06 UTC ***

This new attachment lost the ifdefs of skin/blist.css, and added unrelated changes in conversation.xml and Bubbles.
Attachment #8353701 - Flags: review?(florian) → review-
*** Original post on bio 1658 at 2012-10-11 15:22:43 UTC ***

(In reply to comment #22)
> This new attachment lost the ifdefs of skin/blist.css, and added unrelated
> changes in conversation.xml and Bubbles.
Hrm, not sure what happened there. Those unrelated changes are old too. Sorry.
Comment on attachment 8353701 [details] [diff] [review]
Patch with fixes

*** Original change on bio 1658 attmnt 1944 at 2012-10-11 16:03:05 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353701 - Attachment is obsolete: true
Comment on attachment 8353653 [details] [diff] [review]
Patch (reactivated)

*** Original change on bio 1658 attmnt 1895 at 2012-10-11 16:10:26 UTC ***

(In reply to comment #19)
> > > Is there a reason for this difference?
> > TAB from the status message goes to the conversations on hold if there are any,
> > to the contacts if not. Shift-TAB from the user icon is the reverse direction
> > and always goes to the contacts.
> Is this based on the assumption that if the contact list is empty, there can't
> be any conversation on hold?

Ah! I remember now why I did it this way at the time. The code you are pointing at ensures that a listitem is always selected (if one exists) when you tab to the listbox part of the contact window. The difference arises because "conversations on hold" is not in the tab order when there are no conversations on hold, whereas the contact listbox always is. (And we can't just skip an empty contact list, since the context menu must remain accessible.)

So, reactivating patch (latest-1) (which still has a surplus space nit) for now.
Attachment #8353653 - Attachment description: Patch → Patch (reactivated)
Attachment #8353653 - Attachment is obsolete: false
*** Original post on bio 1658 as attmnt 1975 at 2012-10-17 15:28:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353734 - Flags: review?(florian)
Comment on attachment 8353653 [details] [diff] [review]
Patch (reactivated)

*** Original change on bio 1658 attmnt 1895 at 2012-10-17 15:28:36 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353653 - Attachment is obsolete: true
Comment on attachment 8353734 [details] [diff] [review]
Patch with nit fix

*** Original change on bio 1658 attmnt 1975 at 2012-10-27 03:12:43 UTC ***

>diff --git a/chrome/en-US/locale/en-US/instantbird/instantbird.dtd b/chrome/en-US/locale/en-US/instantbird/instantbird.dtd
>index d8ac207..b02a593 100644
>--- a/chrome/en-US/locale/en-US/instantbird/instantbird.dtd
>+++ b/chrome/en-US/locale/en-US/instantbird/instantbird.dtd
>@@ -52,19 +52,19 @@
> <!ENTITY conversation.contextMenu.close "Close Tab">
> <!ENTITY chat.participants  "Participants:">
> 
> <!ENTITY errorConsoleCmd.label        "Error Console">
> <!ENTITY errorConsoleCmd.accesskey    "C">
> <!ENTITY errorConsoleCmd.commandkey   "j">
> 
> <!ENTITY preferencesCmd.label         "Options...">
>-<!ENTITY preferencesCmd.accesskey     "O"> 
>+<!ENTITY preferencesCmd.accesskey     "O">
> <!ENTITY preferencesCmdUnix.label     "Preferences">
>-<!ENTITY preferencesCmdUnix.accesskey "n"> 
>+<!ENTITY preferencesCmdUnix.accesskey "n">

This whitespace-only change doesn't apply.


(In reply to comment #24)

> Ah! I remember now why I did it this way at the time.
> [...] we can't just skip an
> empty contact list, since the context menu must remain accessible.)

Ok. Thanks for the clarification.
Attachment #8353734 - Flags: review?(florian) → review+
*** Original post on bio 1658 at 2012-10-27 03:37:03 UTC ***

http://hg.instantbird.org/instantbird/rev/93d1283e4719
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3
You need to log in before you can comment on or make changes to this bug.