launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/person-picker-tab-key into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #838122 in Launchpad itself: "Person picker isn't usable with the keyboard"
  https://bugs.launchpad.net/launchpad/+bug/838122

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/person-picker-tab-key/+merge/75771

Enable the tab key to navigate the picker results.

    Launchpad bug: https://bugs.launchpad.net/bugs/838122
    Pre-implementation: wallyworld, wgrant

Users cannot use the tab key to access the picker results. After a search,
the tab key walks though the filters, then jumps to the next page in the
paginator

--------------------------------------------------------------------

RULES

    * The tab key moves the focus to the next visible element that supports
      the tabindex attribute. This includes <a>, <input>, <select>,
      and <button>. The picker results do not have any visible anchors.
    * The ideal case is that the tab key navigates to the user's launchpad-id
      then to the expander.
      * Wrap the li title in an anchor with the js-action class and use
        preventDefault() to ensure the parent's on click action works.
      * pass 'true' to the expander's setUp() method to linkify the icon text.


QA

    Screenshots:
    * http://people.canonical.com/~curtis/target-picker.png
    * http://people.canonical.com/~curtis/person-picker.png

    QAStaging/Dev
    * Search for a user using the person picker.
    * Press the tab key to walk through the filters
    * Verify that the user's launchpad id is the focus.
    * Press tab again
    * Verify that the expander is the focus
    * Press tab again
    * Verify that the next user's launchpad-id is the focus.
    * Use tab to select a user and press enter to choose.


LINT

    lib/lp/app/javascript/picker/picker.js
    lib/lp/app/javascript/picker/tests/test_picker.js


TEST

    ./bin/test -vv --layer=YUITest


IMPLEMENTATION

This was a pretty trivial fix. Wrapping the title text made fix the tab key
and made it clear that the text had an action. The expander was missing
the link because of an oversight. Linkifying the expander also conveys that
the text has an action. Lint reported that the Picker was not fully defined,
I declared it, before defining it.
    lib/lp/app/javascript/picker/picker.js
    lib/lp/app/javascript/picker/tests/test_picker.js
-- 
https://code.launchpad.net/~sinzui/launchpad/person-picker-tab-key/+merge/75771
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/person-picker-tab-key into lp:launchpad.

=== modified file 'lib/lp/app/javascript/picker/picker.js'
--- lib/lp/app/javascript/picker/picker.js	2011-09-16 03:52:55 +0000
+++ lib/lp/app/javascript/picker/picker.js	2011-09-16 16:33:56 +0000
@@ -66,7 +66,8 @@
     CURRENT_FILTER_VALUE = 'current_filter_value';
 
 
-var Picker = function () {
+var Picker;
+Picker = function () {
     Picker.superclass.constructor.apply(this, arguments);
 
     Y.after(this._renderUIPicker, this, RENDERUI);
@@ -389,8 +390,12 @@
      * @param data a json data object with the details to render
      */
     _renderTitleUI: function(data) {
-        var li_title = Y.Node.create(
-            '<span></span>').addClass(C_RESULT_TITLE);
+        var li_title = Y.Node.create('<a href="#"></a>')
+            .addClass(C_RESULT_TITLE)
+            .addClass('js-action');
+        li_title.on('click', function (e, value) {
+                e.preventDefault();
+                }, this, data);
         if (data.title === undefined) {
             // Display an empty element if data is empty.
             return li_title;
@@ -583,7 +588,7 @@
                 li.appendChild(li_details);
                 li.expander = new Y.lp.app.widgets.expander.Expander(
                     li_desc, li_details, {group_id: expander_id});
-                li.expander.setUp();
+                li.expander.setUp(true);
                 li_title.on('click', function (e, value) {
                     this.fire(SAVE, value);
                     }, this, data);

=== modified file 'lib/lp/app/javascript/picker/tests/test_picker.js'
--- lib/lp/app/javascript/picker/tests/test_picker.js	2011-09-16 03:52:55 +0000
+++ lib/lp/app/javascript/picker/tests/test_picker.js	2011-09-16 16:33:56 +0000
@@ -134,6 +134,8 @@
         var li = bb.one('.yui3-picker-results li');
         var title_el = li.one('.yui3-picker-result-title');
         Assert.isNotNull(title_el, "Missing title element");
+        Assert.areEqual('A', title_el.get('tagName'), 'Tab key is broken.');
+        Assert.isTrue(title_el.hasClass('js-action'));
         Assert.areEqual(
             'Joe Schmo\u00a0(Another Joe)', title_el.get('text'),
             'Unexpected title value.');
@@ -196,6 +198,9 @@
         ]);
         var bb = this.picker.get('boundingBox');
         var li = bb.one('.yui3-picker-results li');
+        var expander_action = li.expander.icon_node.get('firstChild');
+        Assert.areEqual(
+            'A', expander_action.get('tagName'), 'Tab key is broken.');
         var details = li.expander.content_node;
         Assert.areEqual(
             'joe on irc.freenode.net<br>Member since 2007',