launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #14812
[Merge] lp:~sinzui/launchpad/bugs-subscriptons-wcag-love into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/bugs-subscriptons-wcag-love into lp:launchpad.
Commit message:
Fix the bugs subscription overlay's keyboard navigation.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #783595 in Launchpad itself: "JS controls for bug subscription are hard to use with screen readers"
https://bugs.launchpad.net/launchpad/+bug/783595
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/bugs-subscriptons-wcag-love/+merge/139812
The javascript controls for bug subscription are hard to use with screen
readers. The overlay does not take focus, the user can tab out of the
overlay, the accordion widget does not expand/contract as expected.
RULES
Pre-implementation: wgrant, stevenk
* Make the bug subscription widget uses the features provided by
its base classes.
* The first element is not being focused.
* The user can tab out of the overlay because the list of tabable
elements disagrees with what the browser supports...it's my
arch nemesis visibility=hidden.
* The problem is compounded by the overflow=hidden rules for
the accordion. The item is not visible to the sighted user,
but it is officially visible and was given space.
QA
* Visit https://qastaging.launchpad.net/pocket-lint/trunk
* View the Create milestone form and immediately press esc.
* Verify the overlay closes, maybe you saw the close button in the
upper right.
* View the Create milestone form and press tab.
* Verify the name field is focused.
* View the Subscribe to bug mail link
* press tab and verify the focus moves to the team select, but the
team radio is NOT selected.
* Tab a dozen times and confirm that the tabs cycle though the widget.
* Tab to "are added or changed in any way", press enter, then
tab a dozen times to cycle though the links again.
* Tab to "Bugs must match this filter" and press enter.
* Tab many times and verify that you tab into unseen links :(,
but you can persevere and tab to the Create button.
LINT
lib/lp/app/javascript/confirmationoverlay/assets/confirmationoverlay-core.css
lib/lp/app/javascript/formoverlay/formoverlay.js
lib/lp/app/javascript/formoverlay/assets/formoverlay-core.css
lib/lp/app/javascript/formoverlay/tests/test_formoverlay.js
lib/lp/app/javascript/overlay/overlay.js
lib/lp/app/javascript/overlay/tests/test_overlay.js
lib/lp/registry/javascript/structural-subscription.js
lib/lp/registry/javascript/tests/test_structural_subscription.html
lib/lp/registry/javascript/tests/test_structural_subscription.js
LoC
I have more than 10,000 lines of credit this week.
TEST
./bin/test -vvc --layer=YUITest -t subscription -t overlay lp
IMPLEMENTATION
I updated the overlay's tabbing rules to also exclude items that the
browser computes to be hidden. This fixed most of the tab problems within
the overlay if a field ever got focus. When the accordion is visible,
it is still possible to tab into a hidden link/field :(. Blind users are
fine, users with pointers are fine, users not filtering are fine, sighted
keyboard users doing filtering will be annoyed if they try to skip a
section. At least they can use the keyboard.
lib/lp/app/javascript/overlay/overlay.js
lib/lp/app/javascript/overlay/tests/test_overlay.js
OMG. The code tries to focus non-visible inputs like <input type=hidden
/>. Even when the proper elements were focuses. There was a corner-case
though in the formoverlay/overlay widget. It is possible to show the
overlay, but there is no elements that can be focused, so I removed the
default rule that hide the close button. The close button is needed is
made the focus in most overlay cases, but a few overlays hid it -- which
has been noted as a inconsistency in the past. The CSS fix ensures all
overlays will get focus and can be closed with esc before any content it
loaded.
lib/lp/app/javascript/formoverlay/formoverlay.js
lib/lp/app/javascript/formoverlay/tests/test_formoverlay.js
lib/lp/app/javascript/formoverlay/assets/formoverlay-core.css
lib/lp/app/javascript/confirmationoverlay/assets/confirmationoverlay-core.css
The SS widget was hacking content into the overlay. The SS code replaces
the supposed download form with its own. The simple fix was to require
the focus rules which I extracted to a method in the formoverlay. There
was a nasty issue where the user tabs from the personal email radio button
to the team select box, which inadvertently then set the user's choice
to team email. The documentation said the code was doing the radio change
on the select box change, so it updated the event to work with onchange
per the docs.
lib/lp/registry/javascript/structural-subscription.js
lib/lp/registry/javascript/tests/test_structural_subscription.html
lib/lp/registry/javascript/tests/test_structural_subscription.js
--
https://code.launchpad.net/~sinzui/launchpad/bugs-subscriptons-wcag-love/+merge/139812
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/bugs-subscriptons-wcag-love into lp:launchpad.
=== modified file 'lib/lp/app/javascript/confirmationoverlay/assets/confirmationoverlay-core.css'
--- lib/lp/app/javascript/confirmationoverlay/assets/confirmationoverlay-core.css 2011-08-30 15:15:11 +0000
+++ lib/lp/app/javascript/confirmationoverlay/assets/confirmationoverlay-core.css 2012-12-13 22:55:25 +0000
@@ -32,7 +32,3 @@
.yui3-lp-app-confirmationoverlay-form-header {
margin-top: 1em;
}
-
-.yui3-lp-app-confirmationoverlay a.close-button {
- visibility: hidden;
- }
=== modified file 'lib/lp/app/javascript/formoverlay/assets/formoverlay-core.css'
--- lib/lp/app/javascript/formoverlay/assets/formoverlay-core.css 2012-07-31 00:01:15 +0000
+++ lib/lp/app/javascript/formoverlay/assets/formoverlay-core.css 2012-12-13 22:55:25 +0000
@@ -64,7 +64,3 @@
.yui3-lazr-formoverlay-form-header {
margin-top: 1em;
}
-
-.yui3-lazr-formoverlay a.close-button {
- visibility: hidden;
- }
=== modified file 'lib/lp/app/javascript/formoverlay/formoverlay.js'
--- lib/lp/app/javascript/formoverlay/formoverlay.js 2012-09-11 14:30:45 +0000
+++ lib/lp/app/javascript/formoverlay/formoverlay.js 2012-12-13 22:55:25 +0000
@@ -313,10 +313,7 @@
if (this.get('centered')){
this.centered();
}
- var form_elem = Y.Node.getDOMNode(this.form_node);
- if (form_elem.elements.length > 0) {
- Y.one(form_elem.elements[0]).focus();
- }
+ this._focusChild();
}
});
},
@@ -382,6 +379,34 @@
Y.WidgetStdMod.BODY, body_node, Y.WidgetStdMod.REPLACE);
},
+
+ /**
+ * Focus the first form element or the close button if the form
+ * is not yet loaded or was removed.
+ *
+ * @method _focusChild
+ * @private
+ */
+ _focusChild: function() {
+ var all_inputs = this.form_node.all('input,select,textarea,button');
+ var sane_inputs = [];
+ all_inputs.each(function(item, index, node_list) {
+ var displayed = item.get('region').height > 0;
+ var visible = item.getComputedStyle('visibility') === 'visible';
+ var not_input_hidden = !(
+ item.get('tagName') === 'INPUT' &&
+ item.get('type') === 'hidden');
+ if (displayed && visible && not_input_hidden) {
+ sane_inputs.push(item);
+ }
+ });
+ if (sane_inputs.length > 0) {
+ sane_inputs[0].focus();
+ } else {
+ this.get('boundingBox').one('.close-button').focus();
+ }
+ },
+
/**
* Extract the form data and pass it to the user-provided submit
* callback.
=== modified file 'lib/lp/app/javascript/formoverlay/tests/test_formoverlay.js'
--- lib/lp/app/javascript/formoverlay/tests/test_formoverlay.js 2012-10-26 10:00:20 +0000
+++ lib/lp/app/javascript/formoverlay/tests/test_formoverlay.js 2012-12-13 22:55:25 +0000
@@ -123,6 +123,39 @@
"when the overlay is shown.");
},
+ test_close_button_has_focus: function() {
+ // The close button has focus if the form is not loaded,
+ // or the form is removed.
+ this.form_overlay.form_node.empty();
+ var close_button = this.form_overlay.get(
+ 'boundingBox').one('.close-button');
+ var focused = false;
+ close_button.on('focus', function(e) {
+ focused = true;
+ });
+ this.form_overlay.hide();
+ close_button.blur();
+ this.form_overlay.show();
+ Assert.isTrue(focused, "The close-button was not focused.");
+ },
+ test_focusChild_form_with_hidden_inputs: function() {
+ // When focusing a form element, the method skips non-visible
+ // form elements
+ this.form_overlay.form_node.empty();
+ this.form_overlay.form_node.append(
+ '<input type="hidden" name="a" />' +
+ '<input type="text" name="b" style="display:none" />' +
+ '<input type="text" name="c" id="c" />'
+ );
+ var visible_input = this.form_overlay.get('boundingBox').one('#c');
+ var focused = false;
+ visible_input.on('focus', function(e) {
+ focused = true;
+ });
+ this.form_overlay._focusChild();
+ Assert.isTrue(focused, "The visble input was not focused.");
+ },
+
test_form_submit_in_body_content: function() {
// The body content should include the submit button.
var body_content = this.form_overlay.getStdModNode("body");
=== modified file 'lib/lp/app/javascript/overlay/overlay.js'
--- lib/lp/app/javascript/overlay/overlay.js 2012-09-11 14:30:45 +0000
+++ lib/lp/app/javascript/overlay/overlay.js 2012-12-13 22:55:25 +0000
@@ -318,9 +318,12 @@
var all_tab_nodes = bounding_box.all(TABBABLE_SELECTOR);
var tab_nodes = new Y.NodeList([]);
all_tab_nodes.each(function(item, index, node_list) {
- if (item.get('region').height > 0) {
- // The node takes up space on the page.
- // This rule will skip empty links which are invalid.
+ var displayed = item.get('region').height > 0;
+ var visible = item.getComputedStyle(
+ 'visibility') === 'visible';
+ if (displayed && visible) {
+ // The node takes up space and can be seen on the page.
+ // This rule will skip empty links that cannot be focused.
tab_nodes.push(item);
}
});
=== modified file 'lib/lp/app/javascript/overlay/tests/test_overlay.js'
--- lib/lp/app/javascript/overlay/tests/test_overlay.js 2012-10-26 10:00:20 +0000
+++ lib/lp/app/javascript/overlay/tests/test_overlay.js 2012-12-13 22:55:25 +0000
@@ -261,6 +261,9 @@
'<input type="text" value="2"/>',
'<a style="display:none;">ignored</a>',
'<span style="display:none;"><button>nil</button></span>',
+ '<span style="visibility:hidden;">',
+ ' <button>nil</button>',
+ '</span>',
'</div>'].join(' ')
});
this.overlay.render();
=== modified file 'lib/lp/registry/javascript/structural-subscription.js'
--- lib/lp/registry/javascript/structural-subscription.js 2012-09-10 20:52:27 +0000
+++ lib/lp/registry/javascript/structural-subscription.js 2012-12-13 22:55:25 +0000
@@ -416,6 +416,9 @@
'click', clean_up);
namespace._add_subscription_overlay.on('submit', clean_up);
namespace._add_subscription_overlay.on('cancel', clean_up);
+ Y.after(
+ namespace._add_subscription_overlay._focusChild,
+ namespace._add_subscription_overlay, 'show');
}
@@ -787,7 +790,7 @@
.set('value', teams[i].link));
}
select.on(
- 'focus',
+ 'change',
function () {
Y.one('input[value="team"][name="recipient"]').set(
'checked', true);
=== modified file 'lib/lp/registry/javascript/tests/test_structural_subscription.html'
--- lib/lp/registry/javascript/tests/test_structural_subscription.html 2012-10-26 09:54:28 +0000
+++ lib/lp/registry/javascript/tests/test_structural_subscription.html 2012-12-13 22:55:25 +0000
@@ -13,6 +13,8 @@
src="../../../../../build/js/yui/yui/yui.js">
</script>
<link rel="stylesheet"
+ href="../../../app/javascript/formoverlay/assets/formoverlay-core.css" />
+ <link rel="stylesheet"
href="../../../../../build/js/yui/console/assets/console-core.css" />
<link rel="stylesheet"
href="../../../../../build/js/yui/test-console/assets/skins/sam/test-console.css" />
=== modified file 'lib/lp/registry/javascript/tests/test_structural_subscription.js'
--- lib/lp/registry/javascript/tests/test_structural_subscription.js 2012-10-26 10:00:20 +0000
+++ lib/lp/registry/javascript/tests/test_structural_subscription.js 2012-12-13 22:55:25 +0000
@@ -475,6 +475,22 @@
Assert.isTrue(called);
},
+ test_focused_form_child: function() {
+ // When the overlay is shown, the first form child is focused.
+ module.setup(this.configuration);
+ module._show_add_overlay(this.configuration);
+ var first_input = Y.Node.create('<input type="text" name="t" />');
+ var focused = false;
+ first_input.on('focus', function(e) {
+ focused = true;
+ });
+ module._add_subscription_overlay.form_node.insert(first_input, 0)
+ module._add_subscription_overlay.hide();
+ first_input.blur();
+ module._add_subscription_overlay.show();
+ Assert.isTrue(focused, "The first input was not focused.");
+ },
+
test_setup_overlay_missing_content_box: function() {
// Pass in a content_box with a missing id to trigger an error.
this.configuration.content_box =
@@ -494,6 +510,10 @@
var accordion_wrapper = Y.one('#accordion-wrapper');
Assert.isTrue(filter_wrapper.hasClass('lazr-closed'));
Assert.isTrue(accordion_wrapper.hasClass('lazr-closed'));
+ var close_link = Y.one('a.close-button');
+ Y.Assert.isTrue(close_link.get('region').height > 0);
+ Y.Assert.isTrue(
+ close_link.getComputedStyle('visibility') == 'visible');
},
test_added_or_changed_toggles: function() {
@@ -734,6 +754,7 @@
tearDown: function() {
window.LP = this.original_lp;
remove_test_node();
+ Y.one('#request-notifications').empty();
delete this.content_node;
},
@@ -1142,6 +1163,7 @@
tearDown: function() {
window.LP = this.original_lp;
remove_test_node();
+ Y.one('#request-notifications').empty();
delete this.content_node;
},
@@ -1207,6 +1229,13 @@
};
},
+ tearDown: function() {
+ window.LP = this.original_lp;
+ remove_test_node();
+ Y.one('#request-notifications').empty();
+ delete this.content_node;
+ },
+
test_description_is_added: function() {
// If we add a subscription on a page that doesn't display
// subcription details then we need to add an "informational
@@ -1294,6 +1323,7 @@
tearDown: function() {
window.LP = this.original_lp;
remove_test_node();
+ Y.one('#request-notifications').empty();
delete this.content_node;
},
@@ -1396,6 +1426,7 @@
tearDown: function() {
window.LP = this.original_lp;
remove_test_node();
+ Y.one('#request-notifications').empty();
delete this.content_node;
},
@@ -1452,6 +1483,7 @@
tearDown: function() {
window.LP = this.original_lp;
remove_test_node();
+ Y.one('#request-notifications').empty();
delete this.content_box;
},
@@ -1819,6 +1851,7 @@
tearDown: function() {
window.LP = this.original_lp;
remove_test_node();
+ Y.one('#request-notifications').empty();
delete this.content_node;
},
