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 1 commit
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
3 changes: 3 additions & 0 deletions Combobox.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1033,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: 6 additions & 0 deletions tests/functional/Combobox-decl.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,12 @@
};
};

getNavigatedDescendant = function (comboId) {
var combo = document.getElementById(comboId);
return combo.list && combo.list.navigatedDescendant
&& combo.list.navigatedDescendant.textContent.trim();
};

isAligned = function (comboId, pos) {
var combo = document.getElementById(comboId);
var popup = document.getElementById(comboId + "_dropdown_wrapper");
Expand Down
169 changes: 169 additions & 0 deletions tests/functional/Combobox.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -857,6 +857,151 @@ define([
.execute(comboId + ".closeDropDown();");
};

var checkNavigatedDescendantNoSelection = function (remote, comboId) {
var executeExpr = "return getNavigatedDescendant(\"" + comboId + "\");";
return loadFile(remote, "./Combobox-decl.html")
.execute(comboId + ".focus();")
.pressKeys(keys.ARROW_DOWN)
.sleep(500)
.execute(executeExpr)
.then(function (navigatedDescendant) {
assert.isNull(navigatedDescendant, "navigatedDescendant should be still null.");
})
.pressKeys(keys.ARROW_DOWN)
.sleep(500)
.execute(executeExpr)
.then(function (navigatedDescendant) {
assert(/^France/.test(navigatedDescendant),
"navigatedDescendant after first ARROW_DOWN: " + navigatedDescendant);
})
.pressKeys(keys.ARROW_DOWN)
.pressKeys(keys.ARROW_DOWN)
.pressKeys(keys.ARROW_DOWN)
.pressKeys(keys.ARROW_DOWN) // focus on Cananda
.sleep(500)
.execute(executeExpr)
.then(function (navigatedDescendant) {
assert(/^Canada/.test(navigatedDescendant),
"navigatedDescendant after forth ARROW_DOWN: " + navigatedDescendant);
})
.pressKeys(keys.ESCAPE)
.sleep(500) // wait for async closing
.execute(executeExpr)
.then(function (navigatedDescendant) {
// note: even if the popup it's closed, list should maintain its navigatedDescendant set.
assert(/^Canada/.test(navigatedDescendant),
"navigatedDescendant after closing the popup: " + navigatedDescendant);
})
.pressKeys(keys.ARROW_DOWN)
.sleep(500) // wait for async opening
.execute(executeExpr)
.then(function (navigatedDescendant) {
assert(/^Canada/.test(navigatedDescendant),
"navigatedDescendant after opening the popup: " + navigatedDescendant);
})
.pressKeys(keys.ARROW_DOWN)
.pressKeys(keys.ARROW_DOWN) // focus on Cananda
.sleep(500)
.execute(executeExpr)
.then(function (navigatedDescendant) {
assert(/^China/.test(navigatedDescendant),
"navigatedDescendant after two ARROW_DOWN: " + navigatedDescendant);
});
};

var checkNavigatedDescendantWithSelection = function (remote, comboId) {
var executeExpr = "return getNavigatedDescendant(\"" + comboId + "\");";
return loadFile(remote, "./Combobox-decl.html")
.execute(comboId + ".focus();")
.pressKeys(keys.ARROW_DOWN)
.sleep(500)
.execute(executeExpr)
.then(function (navigatedDescendant) {
assert.isNull(navigatedDescendant, "navigatedDescendant should be still null.");
})
.pressKeys(keys.ARROW_DOWN)
.sleep(500)
.execute(executeExpr)
.then(function (navigatedDescendant) {
assert(/^France/.test(navigatedDescendant),
"navigatedDescendant after first ARROW_DOWN: " + navigatedDescendant);
})
.pressKeys(keys.ARROW_DOWN)
.pressKeys(keys.ARROW_DOWN)
.findById(comboId + "_item2")
.click() // UK selected
.end()
.sleep(500)
.execute(executeExpr)
.then(function (navigatedDescendant) {
assert(/^UK/.test(navigatedDescendant),
"navigatedDescendant after two ARROW_DOWN: " + navigatedDescendant);
})
.pressKeys(keys.ARROW_DOWN)
.pressKeys(keys.ARROW_DOWN)
.sleep(500)
.execute(executeExpr)
.then(function (navigatedDescendant) {
assert(/^Canada/.test(navigatedDescendant),
"navigatedDescendant after other two ARROW_DOWN: " + navigatedDescendant);
})
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. The user clicks "UK" to select it, which causes the dropdown to close, and then the user re-opens the dropdown... shouldn't the focus be on "UK"?

On another note, this test keeps checking this.list.navigatedDescendant, but especially since this is a functional test, it seems like what you should really be testing for is where the focus goes, or for aria attributes like aria-selected, or something else that directly affects the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

checkNavigatedDescendantWithSelection takes combobox3 as widget to test, which is a multiple-choice combobox.
Hence, when user clicks on UK item the popup does not close.

Regarding the navigatedDescendant, yes we should be checking something else (too). i.e. aria-activedescendant which at each user interaction is set to the highlighted/focused element of the combobox's list.
However it may be worth to keep the test on the navigatedDescendant as this ensures that changes into delite/KeyNav and/or deliteful/List do not break anything.

.pressKeys(keys.ESCAPE)
.sleep(500) // wait for async closing
.execute(executeExpr)
.then(function (navigatedDescendant) {
// note: even if the popup it's closed, list should maintain its navigatedDescendant set.
assert(/^Canada/.test(navigatedDescendant),
"navigatedDescendant after closing the popup: " + navigatedDescendant);
})
.pressKeys(keys.ARROW_DOWN)
.sleep(500) // wait for async opening
.execute(executeExpr)
.then(function (navigatedDescendant) {
// note: on opening, the widget does focus on the navigated descendant
assert(/^Canada/.test(navigatedDescendant),
"navigatedDescendant after opening the popup: " + navigatedDescendant);
})
.pressKeys(keys.ARROW_DOWN)
.pressKeys(keys.ARROW_DOWN) // focus on Cananda
.sleep(500)
.execute(executeExpr)
.then(function (navigatedDescendant) {
assert(/^China/.test(navigatedDescendant),
"navigatedDescendant after two ARROW_DOWN: " + navigatedDescendant);
})
};

var checkNavigatedDescendantWithPreSelection = function (remote, comboId) {
var executeExpr = "return getNavigatedDescendant(\"" + comboId + "\");";
return loadFile(remote, "./Combobox-decl.html")
.execute("document.getElementById(\"" + comboId + "\").focus();")
.pressKeys(keys.ARROW_DOWN)
.sleep(500)
.execute(executeExpr)
.then(function (navigatedDescendant) {
// note: Germany is preselected.
assert(/^Germany/.test(navigatedDescendant),
"navigatedDescendant after first ARROW_DOWN: " + navigatedDescendant);
})
.pressKeys(keys.ARROW_DOWN)
.sleep(500)
.execute(executeExpr)
.then(function (navigatedDescendant) {
assert(/^UK/.test(navigatedDescendant),
"navigatedDescendant after second ARROW_DOWN: " + navigatedDescendant);
})
.pressKeys(keys.ESCAPE)
.sleep(500) // wait for async closing
.pressKeys(keys.ARROW_DOWN)
.sleep(500) // wait for async opening
.execute(executeExpr)
.then(function (navigatedDescendant) {
// note: on opening, the widget does focus on the navigated descendant
assert(/^UK/.test(navigatedDescendant),
"navigatedDescendant after opening the popup: " + navigatedDescendant);
})
};

var checkPopupPosition = function (remote, comboId, position) {
return loadFile(remote, "./Combobox-decl.html")
.execute("return moveToBottom(\"" + comboId + "\");")
Expand Down Expand Up @@ -1191,6 +1336,30 @@ define([
return checkKeyboardNavigationAutoscroll(remote, "combo3");
},

"check navigatedDescendant (no selection)": function () {
var remote = this.remote;
if (remote.environmentType.brokenSendKeys || !remote.environmentType.nativeEvents) {
return this.skip("no keyboard support");
}
return checkNavigatedDescendantNoSelection(remote, "combo3");
},

"check navigatedDescendant (with selection)": function () {
var remote = this.remote;
if (remote.environmentType.brokenSendKeys || !remote.environmentType.nativeEvents) {
return this.skip("no keyboard support");
}
return checkNavigatedDescendantWithSelection(remote, "combo3");
},

"check navigatedDescendant (with pre-selection)": function () {
var remote = this.remote;
if (remote.environmentType.brokenSendKeys || !remote.environmentType.nativeEvents) {
return this.skip("no keyboard support");
}
return checkNavigatedDescendantWithPreSelection(remote, "combo2-custom-sel-multiple");
},

"mouse navigation selectionMode=single, autoFilter=false": function () {
var remote = this.remote;
if (remote.environmentType.browserName === "internet explorer") {
Expand Down