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

Combobox: multiple fixes. #674

Closed
wants to merge 11 commits into from
Closed

Combobox: multiple fixes. #674

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b4e53a5
Fix broken placeholder.
brunano21 Nov 28, 2016
b0199b3
Fix Cancel handler for ComboPopup, which wasnt clearing eventual sele…
brunano21 Nov 29, 2016
ff26def
Do not forward selection event to the list if dropdown is not opened.
brunano21 Nov 30, 2016
23c5824
Fix autocomplete mode bug: when inputNode was empty and in mobile, th…
brunano21 Dec 1, 2016
80e6270
In not-autocomplete mode (hasDownArrow=true), set the popup's inputNo…
brunano21 Dec 2, 2016
3f36ca1
Refocus inputNode if user clicks on list's item.
brunano21 Dec 5, 2016
50a0edf
Focus any previously focus list's item when opening the dropdown.
brunano21 Dec 5, 2016
95a0fb2
Set aria-expanded=true|false attribute when the popup opens and close…
brunano21 Dec 6, 2016
4d46111
Fix broken readOnly (at creation time). Add tests.
brunano21 Dec 1, 2016
49fabb6
jshint fixes.
brunano21 Dec 13, 2016
1ff8cb7
Add aria-expanded and aria-describedby attributes for mobile version …
brunano21 Dec 15, 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
147 changes: 102 additions & 45 deletions Combobox.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,11 +283,18 @@ define([
minFilterChars: 1,

/**
* Displayed value, when Combobox.value is set at the creation time.
* InputNode's value. Can be used to display something at the creation time. After that,
* each user interaction will override this with the selected item label.
* @type {string}
*/
displayedValue: "",

// Flag used for skipping consecutive validations, if one already run.
_justValidated: false,

// Flag used for post initializing the widget value, if the list has not been created yet.
_processValueAfterListInit: false,

createdCallback: function () {
// Declarative case (list specified declaratively inside the declarative Combobox)
var list = this.querySelector("d-list");
Expand Down Expand Up @@ -375,7 +382,7 @@ define([
/* jshint maxcomplexity: 17 */
refreshRendering: function (oldValues) {
var updateReadOnly = false;
if ("list" in oldValues) {
if ("list" in oldValues && this.list) {
this._initList();
}
if ("selectionMode" in oldValues) {
Expand All @@ -395,11 +402,23 @@ define([
}
if ("value" in oldValues) {
if (!this._justValidated) {
this._validateInput(false, true);
} else {
delete this._justValidated;
if (this.list) {
// INFO: if list is already created and attached, then we can validate the `value` value
this._validateInput(false);
} else {
// INFO: otherwise we have to delay its evaluation.
this._processValueAfterListInit = true;
}
}
}
if ("_justValidated" in oldValues) {
if (this._justValidated) {
this._justValidated = false;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This is confusing to me, why don't you just do:

this._justValidated = false;

all the time, without the if() statements?

if ("displayedValue" in oldValues) {
this.inputNode.value = this.displayedValue;
}
Copy link
Member

Choose a reason for hiding this comment

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

How about just saying value={{displayedValue}} in the template?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have already tried this solution before, but it wasn't working and I couldnt explain the reason. Do you have any hint maybe?

},

/**
Expand Down Expand Up @@ -452,31 +471,34 @@ define([
},

_initList: function () {
if (this.list) {
// TODO
// This is a workaround waiting for a proper mechanism (at the level
// of delite/Store - delite/StoreMap) to allow a store-based widget
// to delegate the store-related functions to a parent widget (delite#323).
if (!this.list.attached) {
this.list.attachedCallback();
}
// TODO
// This is a workaround waiting for a proper mechanism (at the level
// of delite/Store - delite/StoreMap) to allow a store-based widget
// to delegate the store-related functions to a parent widget (delite#323).
if (!this.list.attached) {
this.list.attachedCallback();
}

// Class added on the list such that Combobox' theme can have a specific
// CSS selector for elements inside the List when used as dropdown in
// the combo.
$(this.list).addClass("d-combobox-list");
// Class added on the list such that Combobox' theme can have a specific
// CSS selector for elements inside the List when used as dropdown in
// the combo.
$(this.list).addClass("d-combobox-list");

// The drop-down is hidden initially
$(this.list).addClass("d-hidden");
// The drop-down is hidden initially
$(this.list).addClass("d-hidden");

// The role=listbox is required for the list part of a combobox by the
// aria spec of role=combobox
this.list.type = "listbox";
// The role=listbox is required for the list part of a combobox by the
// aria spec of role=combobox
this.list.type = "listbox";

this.list.selectionMode = this.selectionMode === "single" ?
"radio" : "multiple";
this.list.selectionMode = this.selectionMode === "single" ?
"radio" : "multiple";

this._initHandlers();
this._initHandlers();

if (this._processValueAfterListInit) {
this.notifyCurrentValue("value");
delete this._processValueAfterListInit;
}
},

Expand Down Expand Up @@ -517,6 +539,8 @@ define([
}
}.bind(this), 100); // worth exposing a property for the delay?
}
} else {
this.focusNode.focus(); // put the focus back to the inputNode.
Copy link
Member

@wkeese wkeese Dec 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

This change should be tested (that only passes after your change to Combobox.js). Probably just updating one of the existing tests to check focus after clicking a list item.

Actually, I loaded cwithout your changes, and tried both the first and the second Combobox on that page, and it seems like clicking a list's item already refocuses the Combobox if autoFilter===false, or the Combobox's <input> if autoFilter===true??

Also, I'm confused about what happens when:

  1. The first Combobox in Combobox-decl.html where you can't focus the <input>. What happens then?
  2. Multi-select mode. In that case don't you want to keep focus on the list item?
  3. ComboPopup. Does this code run in the ComboPopup case, and if so, doesn't focus go to the ComboPopup's <input> or the original ComboBox <input>? And what if the ComboPopup isn't showing an <input>, like the first example in ComboPopup.html?

}
}.bind(this)),

Expand Down Expand Up @@ -647,8 +671,11 @@ define([
// inputNode does not contain text
if (!this.hasDownArrow) {
// in auto complete mode
this.closeDropDown();
this._toggleComboPopupList();
if (this._isMobile) {
this._toggleComboPopupList();
} else {
this.closeDropDown();
}
Copy link
Member

Choose a reason for hiding this comment

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

You should have a test and a ticket for the bug you are fixing. I also don't understand the commit comment. It says:

when inputNode was empty and in mobile, the popup used to close by itself

That makes it sound like ComboPopup would close immediately after opening, since when the ComboPopup opens the inputNode is empty. Actually, I'm not sure if inputNode is the <input> in the original ComboBox or the <input> in the ComboPopup.

return false;
}
}
Expand All @@ -659,8 +686,9 @@ define([
* Toggles the list's visibility when ComboPopup is used (so in mobile)
*/
_toggleComboPopupList: function () {
if (this._useCenteredDropDown()) {
this.list.setAttribute("d-shown", "" + this.inputNode.value.length >= this.minFilterChars);
if (this._isMobile) {
this.list.setAttribute("d-shown",
"" + this.dropDown.inputNode.value.length >= this.minFilterChars);
Copy link
Member

Choose a reason for hiding this comment

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

I don't see anywhere that you're setting this._isMobile. I think I saw it in an earlier version of this PR, but I don't see it now, and I definitely don't see it in this commit.

Could you create an initial commit with just that refactor (changing _useCenteredDropDown() to _isMobile) and then I'll push that first and then move on to your bug fix commits?

Copy link
Member

Choose a reason for hiding this comment

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

PS: Just noticed the rest of the _isMobile refactor is in your later commit titled "Fix broken readOnly (at creation time). Add tests."

Anyway, all the _isMobile changes should be consolidated into a single commit without any other changes.

this.list.emit("delite-size-change");
}
},
Expand All @@ -684,7 +712,7 @@ define([
// save what user typed at each keystroke.
this.value = inputElement.value;
if (this._useCenteredDropDown()) {
this.inputNode.value = inputElement.value;
this.displayedValue = inputElement.value;
Copy link
Member

Choose a reason for hiding this comment

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

I filed #688 for the remaining task to keep displayedValue in sync w/the actual displayed value.

}
this.handleOnInput(this.value); // emit "input" event.

Expand Down Expand Up @@ -724,7 +752,7 @@ define([
if (this.opened) {
this.closeDropDown(true/*refocus*/);
}
} else if (evt.key === "Spacebar") {
} else if (evt.key === "Spacebar" && this.opened) {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I pushed this as 7d08bcc.

// 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.
Expand All @@ -750,28 +778,30 @@ define([
}.bind(this), inputElement);
},

_validateInput: function (userInteraction, init) {
_validateInput: function (userInteraction) {
if (this.selectionMode === "single") {
this._validateSingle(userInteraction, init);
this._validateSingle(userInteraction);
} else {
this._validateMultiple(userInteraction, init);
this._validateMultiple(userInteraction);
}
this._justValidated = true;
this.notifyCurrentValue("_justValidated");
},

_validateSingle: function (userInteraction, init) {
_validateSingle: function (userInteraction) {
if (userInteraction) {
var selectedItem = this.list.selectedItem;
// selectedItem non-null because List in radio selection mode, but
// the List can be empty, so:
this.inputNode.value = selectedItem ? this._getItemLabel(selectedItem) : "";
this.displayedValue = selectedItem ? this._getItemLabel(selectedItem) : "";
this.value = selectedItem ? this._getItemValue(selectedItem) : "";
} else if (init) {
this.inputNode.value = this.displayedValue !== "" ? this.displayedValue : this.value;
} else {
var item = this._retrieveItemFromSource(this.value);
this.displayedValue = item ? item[this.list.labelAttr || this.list.labelFunc] : this.value;
}
},

_validateMultiple: function (userInteraction, init) {
_validateMultiple: function (userInteraction) {
var n;
if (userInteraction) {
var selectedItems = this.list.selectedItems;
Expand All @@ -795,7 +825,7 @@ define([
// make sure this is already done when FormValueWidget.handleOnInput() runs.
this.valueNode.value = value;
this.handleOnInput(this.value); // emit "input" event
} else if (init) {
} else {
var items = [];
if (typeof this.value === "string") {
if (this.value.length > 0) {
Expand All @@ -809,11 +839,12 @@ define([
} // else empty array. No pre-set values.
n = items.length;
if (n > 1) {
this.inputNode.value = string.substitute(this.multipleChoiceMsg, {items: n});
this.displayedValue = string.substitute(this.multipleChoiceMsg, {items: n});
} else if (n === 1) {
this.inputNode.value = this.displayedValue !== "" ? this.displayedValue : items[0];
var item = this._retrieveItemFromSource(items[0]);
this.displayedValue = item ? item[this.list.labelAttr || this.list.labelFunc] : this.value;
} else {
this.inputNode.value = this.multipleChoiceNoSelectionMsg;
this.displayedValue = this.multipleChoiceNoSelectionMsg;
}
}
},
Expand Down Expand Up @@ -848,6 +879,21 @@ define([
this.openDropDown();
},

_retrieveItemFromSource: function (key) {
var item = null,
_source = this.source || (this.list && this.list.source);
if (_source) {
if (Array.isArray(_source)) {
item = _source.filter(function (i) {
return i[this.list.valueAttr || this.list.labelAttr] === key;
}.bind(this))[0];
} else {
item = _source.getSync && _source.getSync(key);
}
}
return item;
},
Copy link
Member

Choose a reason for hiding this comment

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

It's strange to have code that only works with a sync store, especially considering that you are working on auto-complete which is asynchronous.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I can make this part specific for only when in not auto-complete mode (hasDownArrow=false). Do you agree?


/**
* Sets the new list's query.
* This method can be overridden when using other store types.
Expand Down Expand Up @@ -929,6 +975,12 @@ define([
// mobile version
if (!this.hasDownArrow) {
this._toggleComboPopupList();
} else {
// INFO: display into the popup inputNode any pre-selected item,
// only if the inputNode is visible, though.
if (!this._inputReadOnly) {
this.dropDown.inputNode.value = this.displayedValue;
}
}
this.dropDown.focus();
}
Expand Down Expand Up @@ -960,8 +1012,10 @@ define([
_dropDownKeyUpHandler: dcl.superCall(function (sup) {
return function () {
if (this.hasDownArrow) {
if (this.inputNode.value.length === 0 && this.minFilterChars === 0) {
this.list.query = this._getDefaultQuery();
if (this.inputNode.value.length === 0) {
if (this.minFilterChars === 0) {
this.list.query = this._getDefaultQuery();
}
Copy link
Member

Choose a reason for hiding this comment

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

I guess this is the code change to "Restore down-arrow key original behaviour, even if the inputNode is empty."?

It doesn't seem quite right. For example:

  • What if the list hasn't been created yet? Didn't we agree to always set the list's query at the same time as we create the list?
  • What if the user typed a different key than the down arrow, such as backspace?

Copy link
Contributor Author

@brunano21 brunano21 Dec 8, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

  • What if the list hasn't been created yet? Didn't we agree to always set the list's query at the same time as we create the list?
    --- Sure, I should check if the list is already created. However this.list.query and this.list.source are set at the same time, because the _dropDownKeyUpHandler function calls its sup function which calls openDropDown (where this.list.source gets assigned. So, what's wrong?

  • What if the user typed a different key than the down arrow, such as backspace?
    --- What's your point? In this case the above method (_dropDownKeyUpHandler) runs only when the user press the Down Arrow Key (see: https://github.com/ibm-js/delite/blob/master/HasDropDown.js#L400-L412). So, what's the problem with backspace?

} else if (this.inputNode.value.length < this.minFilterChars) {
return;
}
Expand All @@ -979,6 +1033,9 @@ define([
// 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) {
item = this.list.navigatedDescendant;
}

if (!item) {
var selectedItems = this.list.selectedItems;
Expand Down
6 changes: 5 additions & 1 deletion Combobox/ComboPopup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ define([
* @protected
*/
okHandler: function () {
this.combobox._validateMultiple(this.combobox.inputNode);
// NOTE: no need to validate since it's handled by the `selection-change` listener
this.combobox.closeDropDown();
},

Expand All @@ -76,7 +76,11 @@ define([
* @protected
*/
cancelHandler: function () {
// INFO: resetting any selected items.
this.combobox.list.selectedItems = [];
this.combobox.closeDropDown();
// cont: then ask to validate, so widget's value and inputNode get updated as well.
this.combobox._validateMultiple(true);
Copy link
Member

Choose a reason for hiding this comment

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

Won't this code call _validateMultiple() even for single-selection Comboboxes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, but I don't understand what you are referring to. cancelHandler function runs only when user press "Cancel" button, which is visible only if selectionMode=multiple.
In selectionMode=single we don't have any button shown.

Copy link
Member

Choose a reason for hiding this comment

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

Oh OK I didn't realize that.

},

/**
Expand Down
5 changes: 3 additions & 2 deletions Combobox/Combobox.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,11 @@
autocomplete="off" autocorrect="off" autocapitalize="none"
aria-autocomplete="list"
type="text"
readonly="{{this._inputReadOnly ? 'readonly' : ''}}">
readonly="{{this._inputReadOnly ? 'readonly' : ''}}"
placeholder="{{searchPlaceHolder}}">
<input class="d-hidden"
attach-point="valueNode"
readonly name="{{name}}" value="{{value}}">
readonly name="{{name}}">
<span class="d-combobox-arrow" d-shown="{{hasDownArrow}}"
attach-point="buttonNode" aria-hidden="true"></span>
</template>
4 changes: 2 additions & 2 deletions samples/Combobox.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@

<p>
<label for="comboTeams3-input">Your favorite team (single choice):</label><br/>
<span>[options: hasDownArrow=false, filterDelay=750ms, minFilterChars=0]</span>
<span>[options: hasDownArrow=true, filterDelay=750ms, minFilterChars=0]</span>
</p>
<d-combobox selectionMode="single" autoFilter="true" id="comboTeams3" hasDownArrow="true" filterDelay="750" minFilterChars="0">
<d-list righttextAttr="world-cup-victories" categoryAttr="region" showNoItems="true">
Expand Down Expand Up @@ -219,7 +219,7 @@
</d-combobox>

<p>
<label for="comboPlayersWithValuesDecl-input">Your favorite players (multiple choice - declaratively ):</label>
<label for="comboPlayersWithValuesDecl-input">Your favorite players (multiple choice - declaratively):</label>
<br/>Pre-set values = Hagi, Buffon
</p>
<d-combobox selectionMode="multiple" id="comboPlayersWithValuesDecl" value="Hagi,Buffon">
Expand Down
Loading