Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Set aria-selected="false" on deselected items #76

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Set aria-selected="false" on deselected items #76

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2348d84
Set aria-selected="false" on items which are initially not selected, …
Aug 25, 2016
9b3ea9c
Add unit tests.
Aug 25, 2016
12b17ac
Set aria-selected="false" on items which are initially not selected, …
Aug 25, 2016
75bbb94
Remove console statement.
Sep 6, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 18 additions & 10 deletions iron-menu-behavior.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@
},

attached: function() {
this._resetTabindices();
this._resetItemAttributes();
},

/**
Expand All @@ -90,16 +90,25 @@
},

/**
* Resets all tabindex attributes to the appropriate value based on the
* Reset attributes of the menu's new/changed collection of items:
*
* Reset all tabindex attributes to the appropriate value based on the
* current selection state. The appropriate value is `0` (focusable) for
* the default selected item, and `-1` (not keyboard focusable) for all
* other items.
*
* Reset the aria-selected attribute based on the current selection state.
* As _applySelection notes, we need to set aria-selected to false for
* unselected items, even in their initial state, not just when they become
* deselected.
*/
_resetTabindices: function() {
_resetItemAttributes: function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One underscore indicates protected for us (two indicates private). This technically constitutes a breaking API change. If we can verify it's not being overridden in elements we control that depend on this behavior, it's probably fine..

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can see, the only references to _resetTabindices are in iron-menu-behavior.html itself. This also seems like an unusual function for someone to override.

If you'd prefer to play it safe, I could preserve the _resetTabindices name, by either: 1) using the old name for the new function (which does more than reset tab indices) or, 2) creating a new _resetItemAttributes that calls both the old _resetTabindicies and does the new work.

var selectedItem = this.multi ? (this.selectedItems && this.selectedItems[0]) : this.selectedItem;

this.items.forEach(function(item) {
item.setAttribute('tabindex', item === selectedItem ? '0' : '-1');
var isSelected = item === selectedItem;
item.setAttribute('tabindex', isSelected ? '0' : '-1');
item.setAttribute('aria-selected', isSelected);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this is technically correct, it's worth noting that this will double the number of calls to setAttribute when attached. Have we considered the performance implications?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tab sets usually only have a small number of tabs. In the typical case of 3–7 tabs per tab set, I wouldn't expect an additional setAttribute per tab to have a noticeable impact on perf. Some tab-heavy designs (multiple tabbed regions on a single page, or tabs with tabs) might have more tabs — tens of tabs per page, maybe — but I'd still imagine that other costs would dominate.

It's possible that some attribute setting could be done lazily. E.g., with both attributes here, it might be possible to only set those attributes for the element with the focus and the elements on either side of it. But I'd imagine that could easily miss cases, and it seems safer to set the attributes when attached.

}, this);
},

Expand Down Expand Up @@ -193,11 +202,10 @@
* selected state, otherwise false.
*/
_applySelection: function(item, isSelected) {
if (isSelected) {
item.setAttribute('aria-selected', 'true');
} else {
item.removeAttribute('aria-selected');
}
// Per the ARIA spec at https://www.w3.org/TR/wai-aria-1.1/#tab:
// "inactive tab elements [should] have their aria-selected attribute set
// to false".
item.setAttribute('aria-selected', isSelected);
Polymer.IronSelectableBehavior._applySelection.apply(this, arguments);
},

Expand Down Expand Up @@ -226,7 +234,7 @@
*/
_onIronItemsChanged: function(event) {
if (event.detail.addedNodes.length) {
this._resetTabindices();
this._resetItemAttributes();
}
},

Expand Down
23 changes: 23 additions & 0 deletions test/iron-menu-behavior.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,6 +497,29 @@
done();
});
});

test('aria-selected attribute for items defaults to false', function() {
var menu = fixture('basic');
var selectedValues = menu.items.map(function(item) {
return item.getAttribute('aria-selected');
});
assert.deepEqual(selectedValues, ['false', 'false', 'false']);
});

test('aria-selected attribute reflects item selection state', function() {
var menu = fixture('basic');
var item0 = menu.items[0];
var item1 = menu.items[1];
assert.equal(item0.getAttribute('aria-selected'), 'false');
assert.equal(item1.getAttribute('aria-selected'), 'false');
menu.selected = 0;
assert.equal(item0.getAttribute('aria-selected'), 'true');
assert.equal(item1.getAttribute('aria-selected'), 'false');
menu.selected = 1;
assert.equal(item0.getAttribute('aria-selected'), 'false');
assert.equal(item1.getAttribute('aria-selected'), 'true');
});

});
</script>
</body>
Expand Down