Skip to content

Commit

Permalink
Various improvements and fixes, mostly per feedback from wkeese.
Browse files Browse the repository at this point in the history
  • Loading branch information
Adrian Vasiliu committed Jan 29, 2015
1 parent 485ba54 commit c70321a
Show file tree
Hide file tree
Showing 2 changed files with 75 additions and 48 deletions.
99 changes: 61 additions & 38 deletions Combobox.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -385,47 +385,67 @@ define([
this._initValue();
},

/**
* Handles navigation for both keyboard and mouse interaction.
* @param {Node} The DOM node targeted by the navigation.
* @param {boolean} keyboard true if interaction triggered by key event, false otherwise.
* @private
*/
_navigationHandler: function (target, keyboard) {
var input = this._popupInput || this.inputNode;
var rend = target ? this.list.getEnclosingRenderer(target) : null;
if (this.selectionMode === "single") {
if (rend) {
if (keyboard) { // "keynav-child-navigated" event triggerred by key event
if (!this.list.isSelected(rend.item)) {
this.list.setSelected(rend.item, true);
this._updateScroll(rend.item, true);
}
} else { // mouse interaction ("click" event)
this.defer(function () {
// deferred such that the user can see the selection feedback
// before the dropdown closes.
this.closeDropDown(true/*refocus*/);
}.bind(this), 100); // worth exposing a property for the delay?
}
input.setAttribute("aria-activedescendant", target.id);
} else {
input.removeAttribute("aria-activedescendant");
}
} else if (this.selectionMode === "multiple") {
if (rend) {
if (keyboard) {
this._updateScroll(rend.item);
}
input.setAttribute("aria-activedescendant", target.id);
} else {
input.removeAttribute("aria-activedescendant");
}
}
},

/**
* Initializes event handlers for item navigation.
* @private
*/
_initHandlers: function () {
if (this._initHandlersDone) {
return; // set handlers only once
}
this._initHandlersDone = true;

// Keyboard navigation support
// Keyboard navigation
this.list.on("keynav-child-navigated", function (evt) {
/* jshint maxcomplexity: 12 */
var input = this._popupInput || this.inputNode;
var rend = evt.newValue ? this.list.getEnclosingRenderer(evt.newValue) : null;
if (this.selectionMode === "single") {
if (rend) {
if (!this.list.isSelected(rend.item)) {
if (evt.triggerEvent &&
(evt.triggerEvent.type === "keydown" || evt.triggerEvent.type === "keypress")) {
this.list.setSelected(rend.item, true);
this._updateScroll(rend.item, true);
} else {
this.defer(function () {
// deferred such that the user can see the selection feedback
// before the dropdown closes.
this.closeDropDown(true/*refocus*/);
}.bind(this), 100); // worth exposing a property for the delay?
}
}
input.setAttribute("aria-activedescendant", evt.newValue.id);
} else {
input.removeAttribute("aria-activedescendant");
}
} else if (this.selectionMode === "multiple") {
if (rend) {
if (rend.item && evt.triggerEvent &&
(evt.triggerEvent.type === "keydown" || evt.triggerEvent.type === "keypress")) {
this._updateScroll(rend.item);
}
input.setAttribute("aria-activedescendant", evt.newValue.id);
} else {
input.removeAttribute("aria-activedescendant");
}
if (!(evt.triggerEvent &&
(evt.triggerEvent.type === "keydown" || evt.triggerEvent.type === "keypress"))) {
return; // navigation not triggered by keyboard events
}
this._navigationHandler(evt.newValue, true);
}.bind(this));

// Mouse navigation
this.list.on("click", function (evt) {
this._navigationHandler(evt.target, false);
}.bind(this));

// React to programmatic changes of selected items
Expand All @@ -439,11 +459,7 @@ define([
this.value = selectedItem ? this._getItemValue(selectedItem) : "";
this.handleOnInput(this.value); // emit "input" event
} else if (this.selectionMode === "multiple") {
// if _useCenteredDropDown() is true, let the dropdown's OK/Cancel
// buttons do the job
if (!this._useCenteredDropDown()) {
this._validateMultiple(this._popupInput || this.inputNode);
}
this._validateMultiple(this._popupInput || this.inputNode);
}
}
}.bind(this));
Expand Down Expand Up @@ -612,6 +628,9 @@ define([
this.closeDropDown(true/*refocus*/);
}
} else if (evt.keyCode === keys.SPACE) {
// Simply forwarding the key event to List doesn't allow toggling
// the selection, because List's mechanism is based on the event target
// which here is the input element outside the List. TODO: see deliteful #500.
if (this.selectionMode === "multiple") {
var rend = this.list.getEnclosingRenderer(this.list.navigatedDescendant);
var item = rend.item;
Expand Down Expand Up @@ -735,6 +754,10 @@ define([
* @private
*/
_updateScroll: function (item, navigate) {
// Since List is in focus-less mode, it does not give focus to
// navigated items, thus the browser does not autoscroll.
// TODO: see deliteful #498

if (!item) {
var selectedItems = this.list.selectedItems;
item = selectedItems && selectedItems.length > 0 ?
Expand Down
24 changes: 14 additions & 10 deletions tests/unit/Combobox.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -446,24 +446,26 @@ define([
var checkAfterClickItem = function (changeCounterExpectedValue, inputCounterExpectedValue,
itemName, expectedValue) {
assert.strictEqual(inputCounter, inputCounterExpectedValue,
"inputCounter after clicking " + itemName);
"inputCounter after clicking " + itemName + " (single)");
assert.strictEqual(changeCounter, changeCounterExpectedValue,
"changeCounter after clicking " + itemName);
"changeCounter after clicking " + itemName + " (single)");
changeCounter = 0; // reinit
inputCounter = 0; // reinit
assert.strictEqual(changeValue, expectedValue,
"Wrong value when receiving change event after clicking " + itemName);
"Wrong value when receiving change event after clicking " + itemName +
" (single)");
assert.strictEqual(inputValue, expectedValue,
"Wrong value when receiving input event after clicking " + itemName);
"Wrong value when receiving input event after clicking " + itemName +
" (single)");
changeValue = null; // reinit
inputValue = null; // reinit
};
var item2 = combo.list.getItemRenderers()[2];
item2.click(); // automatically closes the dropdown (asynchronously)

// ComboBox is using Stateful.observe to listen to selection change on the list
// Combobox is using Stateful.observe to listen to selection change on the list
// and update its value, this means the timer below must not only cover for the 100 ms delay on
// ComboBox but also the Sateful.observe asynchronism
// Combobox but also the Sateful.observe asynchronism
// Higher than the 100 delay of Combobox' defer when closing the dropdown
var delay = 400;
setTimeout(d.rejectOnError(function () {
Expand Down Expand Up @@ -533,13 +535,15 @@ define([
var checkAfterClickItem = function (changeCounterExpectedValue, inputCounterExpectedValue, itemName,
expectedChangeValue, expectedInputValue) {
assert.strictEqual(inputCounter, inputCounterExpectedValue,
"After clicking " + itemName);
"After clicking " + itemName + " (multiple)");
assert.strictEqual(changeCounter, changeCounterExpectedValue,
"After clicking " + itemName);
"After clicking " + itemName + " (multiple)");
assert.deepEqual(changeValue, expectedChangeValue,
"Wrong value when receiving change event after clicking " + itemName);
"Wrong value when receiving change event after clicking " + itemName +
" (multiple)");
assert.deepEqual(inputValue, expectedInputValue,
"Wrong value when receiving input event after clicking " + itemName);
"Wrong value when receiving input event after clicking " + itemName +
" (multiple)");
};
var item2 = combo.list.getItemRenderers()[2];
item2.click();
Expand Down

1 comment on commit c70321a

@wkeese
Copy link
Member

@wkeese wkeese commented on c70321a Jan 30, 2015

Choose a reason for hiding this comment

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

It's weird that you're calling a function called "_navigationHandler" on click. A click is for selecting and closing the dropdown, not for navigation.

Why not put the closeDropDown() call inside the click handler?

Also, I don't see where the code is for closing the drop down based on enter key, but presumably that should follow the same code path as the click handler.

PS: In other words, put back the old handler for "keynav-child-navigated" and then just move the closeDropDown() call to the click handler.

Please sign in to comment.