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

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 all 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
277 changes: 165 additions & 112 deletions Combobox.js
View file
Open in desktop

Large diffs are not rendered by default.

7 changes: 4 additions & 3 deletions Combobox/ComboPopup.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
<template role="presentation"
requires="deliteful/LinearLayout, deliteful/Button">
<d-linear-layout style="height: 100%">
<div class="d-combo-popup-header">{{header}}</div>
<input d-hidden="{{!(this.combobox.autoFilter && this.combobox.selectionMode !== 'multiple')}}"
<div class="d-combo-popup-header" id="{{_tag}}-{{widgetId}}-header">{{header}}</div>
<input d-hidden="{{this.combobox._inputReadOnly}}"
aria-labelledby="{{_tag}}-{{widgetId}}-header"
Copy link
Member

Choose a reason for hiding this comment

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

Note: This confused me at first since tests/functional/ComboPopup.js already checks for a label for the input:

var checkListInPopup = function (remote, comboId, hasFilterInput, isMultiSelect) {
...
		.findByCssSelector("label[for='" + comboId + "-input']")
		.getVisibleText()
		.then(function (value) {
			assert.strictEqual(value, label);
			label = null;
		})

It turns out that test is useless though because it's checking the <label> for the <input> in the original Combobox rather than for the <input> in the ComboPopup.

So, I see why you added this. It's unusual to use aria-labelledby rather than <label> though.

Copy link
Member

Choose a reason for hiding this comment

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

PS: Oh, I just realized that it isn't checking the <label> for the <input> in the original Combobox but rather just getting the value, to be used later.

Copy link
Member

Choose a reason for hiding this comment

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

Filed #686.

attach-point="inputNode"
class="d-combobox-input"
role="combobox" autocomplete="off" autocapitalize="none" autocorrect="off"
Expand All @@ -17,4 +18,4 @@
label="{{combobox.okMsg}}" on-click="{{okHandler}}"></button>
</d-linear-layout>
</d-linear-layout>
</template>
</template>
15 changes: 12 additions & 3 deletions Combobox/ComboPopup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ define([
.removeClass("d-hidden");
}
this.combobox._prepareInput(this.inputNode);
this.combobox.observe(function (oldValues) {
if ("opened" in oldValues) {
this.inputNode.setAttribute("aria-expanded", this.combobox.opened);
}
}.bind(this));
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused, isn't this.combobox.opened always true whenever ComboPopup is shown?

Also (minor thing), the checkin comment since aria-described but the code change is for aria-labelledby.

Copy link
Member

@wkeese wkeese Jan 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

aria-expanded should indicate whether or not the list is displayed, which (on the master branch) is controlled by Combobox#_togglePopupList(), or you can get the info from this.combobox.list.getAttribute("d-shown").

Usually the list is displayed, but sometimes it isn't, like the second to last test case in ComboPopup.html where hasDownArrow=false minFilterChars=3. The list is only displayed after typing 3 characters.

PS: Perhaps ComboPopup should have a displayList property, and then Combobox#_togglePopupList() should set that property to true/false.

Copy link
Member

Choose a reason for hiding this comment

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

Filed #687.

}
}
},
Expand All @@ -67,7 +72,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,15 +81,19 @@ 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.

},

/**
* Called by HasDropDown in order to get the focus on the widget's list.
* @protected
*/
focus: function () {
if (this.combobox.autoFilter && this.combobox.selectionMode === "single") {
if (!this.combobox._inputReadOnly) {
this.inputNode.focus();
} else {
// first check if list is not hidden.
Expand All @@ -93,7 +102,7 @@ define([
var id = this.combobox.list.getIdentity(
this.combobox.list.selectedItems.length > 0 ? this.combobox.list.selectedItems[0] : "");
var renderer = (id && id !== -1) ? this.combobox.list.getRendererByItemId(id) :
this.combobox.list.getRenderers()[0];
this.combobox.list.getItemRenderers()[0];
this.combobox.list.navigateTo(renderer);
}
}
Expand Down
6 changes: 4 additions & 2 deletions Combobox/Combobox.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,13 @@
attach-point="inputNode,focusNode"
autocomplete="off" autocorrect="off" autocapitalize="none"
aria-autocomplete="list"
aria-expanded="{{opened}}"
type="text"
readonly="{{this._inputReadOnly ? 'readonly' : ''}}">
value="{{displayedValue}}"
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>
29 changes: 24 additions & 5 deletions samples/Combobox.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,15 @@

<script type="text/javascript">
require.config({
baseUrl: "../.."
baseUrl: "../..",
waitSeconds: 60 // for debugging...
});
</script>

<script type="text/javascript">
require([
"requirejs-dplugins/jquery!event",
"dstore/Memory",
"deliteful/list/List",
"deliteful/tests/functional/SlowStore",
"deliteful/Combobox",
Expand All @@ -28,7 +30,7 @@
"deliteful/Checkbox",
"delite/theme!delite/themes/{{theme}}/global.css", // page level CSS
"requirejs-domready/domReady!"
], function ($, List, SlowStore, Combobox) {
], function ($, Memory, List, SlowStore, Combobox) {

startsWith.on("change", function () {
comboTeams.filterMode = "startsWith";
Expand Down Expand Up @@ -94,6 +96,17 @@
};
container.appendChild(comboTeamsAutoComplete);

var comboEmptyStore = new Combobox({
id: "comboEmptyStore",
selectionMode: "single",
autoFilter: true,
hasDownArrow: true,
list: new List({showNoItems: true}),
source: new Memory()
});

document.getElementById("comboEmptyStoreContainer").appendChild(comboEmptyStore);

document.body.style.display = "";
});
</script>
Expand All @@ -111,7 +124,7 @@
<br/><br/>
<p>
<label for="comboTeams-input">Your favorite team (single choice):</label><br/>
<span>[options: default; displayedValue:"Italy" (value: Italy)]</span>
<span>[options: default; value: Italy]</span>
</p>
<d-combobox selectionMode="single" autoFilter="true" id="comboTeams" value="Italy">
<d-list righttextAttr="world-cup-victories" categoryAttr="region" showNoItems="true">
Expand Down Expand Up @@ -170,7 +183,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 +232,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 All @@ -232,6 +245,11 @@
</d-list>
</d-combobox>

<p>
<label for="comboEmptyStoreContainer-input">Combobox with an empty store</label>
<br/>
<div id="comboEmptyStoreContainer"></div>

<p>
<button is="d-button" onclick="showResults()">Done</button>
</p>
Expand All @@ -242,6 +260,7 @@ <h4>The below is using a `slowStore`, that mimics a `dStore/Request`</h4>
<span>[hasDropDown:false, source: SlowStore, filterDelay:500ms]</span>
</p>
<div id="comboTeamsAutoCompleteContainer"></div>

<br/>
</body>
</html>
23 changes: 14 additions & 9 deletions tests/functional/ComboPopup.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
<script type="text/javascript">
// global variables used by the automatic functional tests test:
var ready = false; // set to true when the test page is ready
var storeTestingInfo;

require.config({
config: {
Expand All @@ -37,7 +38,7 @@
register.deliver();

// Store information for use in the automatic functional test
var storeTestingInfo = function (combo) {
storeTestingInfo = function (combo) {
if (!combo.init) {
combo._inputEventCounter = 0;
combo._changeEventCounter = 0;
Expand Down Expand Up @@ -67,17 +68,20 @@
};

// single selection mode, autoFilter=true
combo = document.getElementById("combo1");
storeTestingInfo(document.getElementById("combo1"));

// single selection mode, autoFilter=true
combo = document.getElementById("combo2");
storeTestingInfo(document.getElementById("combo2"));

// multiple selection mode
combo = document.getElementById("combo3");
storeTestingInfo(document.getElementById("combo3"));

// auto complete mode, min filter char = 3
storeTestingInfo(document.getElementById("combo4"));

// auto complete mode, min filter char = 0
storeTestingInfo(document.getElementById("combo5"));

// Used by the automatic functional test, for comboboxes in both
// single and multiple selection modes.
getComboState = function (comboId) {
Expand All @@ -95,6 +99,7 @@
combo.inputNode.getAttribute("aria-activedescendant"));
return {
inputNodeValue: combo.inputNode.value,
popupInputNodeValue: combo.opened ? combo.dropDown.inputNode.value : "",
widgetValue: combo.value,
valueNodeValue: combo.valueNode.value,
opened: combo.opened,
Expand Down Expand Up @@ -142,7 +147,7 @@ <h1>
</h1>

<label for="combo1-input">
Simple Combobox popup
id = combo1 - Simple Combobox popup
</label>
<d-combobox selectionMode="single" autoFilter="false" id="combo1" name="combo1" value="France">
<d-list righttextAttr="sales" categoryAttr="region">
Expand Down Expand Up @@ -187,7 +192,7 @@ <h1>
</d-combobox>

<label for="combo2-input">
Combobox popup with &lt;input&gt; (autoFilter=true)
id = combo2 - Combobox popup with &lt;input&gt; (autoFilter=true)
</label>
<d-combobox selectionMode="single" autoFilter="true" id="combo2" name="combo2" value="France">
<d-list righttextAttr="sales" categoryAttr="region" showNoItems="true">
Expand Down Expand Up @@ -232,7 +237,7 @@ <h1>
</d-combobox>

<label for="combo3-input">
Combobox with OK/cancel button (multiselect)
id = combo3 - Combobox with OK/cancel button (multiselect)
</label>
<d-combobox selectionMode="multiple" autoFilter="false" id="combo3" name="combo3">
<d-list righttextAttr="sales" categoryAttr="region">
Expand Down Expand Up @@ -277,7 +282,7 @@ <h1>
</d-combobox>

<label for="combo4-input">
Combobox popup with &lt;input&gt; autoFilter=true, hasDownArrow=false, minFilterChars=3, "AutoComplete" mode
id = combo4 - Combobox popup with &lt;input&gt; autoFilter=true, hasDownArrow=false, minFilterChars=3, "AutoComplete" mode
</label>
<d-combobox selectionMode="single" autoFilter="true" hasDownArrow="false" id="combo4" name="combo4" minFilterChars="3">
<d-list righttextAttr="sales" categoryAttr="region" showNoItems="true">
Expand Down Expand Up @@ -322,7 +327,7 @@ <h1>
</d-combobox>

<label for="combo5-input">
Combobox popup with &lt;input&gt; (autoFilter=true), hasDownArrow=false, minFilterChars=0, "AutoComplete" mode
id = combo5 - Combobox popup with &lt;input&gt; (autoFilter=true), hasDownArrow=false, minFilterChars=0, "AutoComplete" mode
</label>
<d-combobox selectionMode="single" autoFilter="true" hasDownArrow="false" id="combo5" name="combo5" minFilterChars="0">
<d-list righttextAttr="sales" categoryAttr="region" showNoItems="true">
Expand Down
Loading