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 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
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
137 changes: 136 additions & 1 deletion tests/functional/ComboPopup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,127 @@ define([
valueNodeValueAtLatestInputEvent: "China,Germany",
widgetValueAtLatestChangeEvent: ["China", "Germany"],
valueNodeValueAtLatestChangeEvent: "China,Germany"
}, "after clicking again the root node (close)");
}, "after clicking on the 'Ok' button (close)");
})
.end();
};

var checkMultiSelectionCancel = function (remote, comboId) {
var executeExpr = "return getComboState(\"" + comboId + "\");";
return loadFile(remote, "./ComboPopup.html")
.findById(comboId)
.execute(executeExpr)
.then(function (comboState) {
// The first option should be the one selected,
// the popup is closed initially
checkComboState(comboId, comboState,
{ // expected combo state
inputNodeValue: comboState.multipleChoiceNoSelectionMsg,
popupInputNodeValue: "", // invisible!
widgetValue: [],
valueNodeValue: "",
opened: false,
selectedItemsCount: 0,
itemRenderersCount: 37,
inputEventCounter: 0, // no event so far
changeEventCounter: 0,
widgetValueAtLatestInputEvent: undefined, // never received
valueNodeValueAtLatestInputEvent: undefined,
widgetValueAtLatestChangeEvent: undefined,
valueNodeValueAtLatestChangeEvent: undefined
}, "right after load");
})
.click()
.sleep(500) // wait for List's loading panel to go away
.execute(executeExpr)
.then(function (comboState) {
// The first option should be the one selected,
// the first click just opens the dropdown.
checkComboState(comboId, comboState,
{ // expected combo state
inputNodeValue: comboState.multipleChoiceNoSelectionMsg,
popupInputNodeValue: "", // invisible
widgetValue: [],
valueNodeValue: "",
opened: true,
selectedItemsCount: 0,
itemRenderersCount: 37,
inputEventCounter: 0, // no event so far
changeEventCounter: 0,
widgetValueAtLatestInputEvent: undefined, // never received
valueNodeValueAtLatestInputEvent: undefined,
widgetValueAtLatestChangeEvent: undefined,
valueNodeValueAtLatestChangeEvent: undefined
}, "after click on root node");
})
.end()
.findById(comboId + "_item1") // "Germany"
.click()
.sleep(500) // wait for popup to close
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 comment seems wrong, the ComboPopup doesn't close until you click the OK or cancel buttons.
Likewise on line 485.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, now I see that checkMultiSelection() and checkMultiSelectionCancel() are basically the same code, except for the last section where checkMultiSelection() clicks OK and checkMultiSelectionCancel() clicks Cancel. So instead of having all that copy-and-pasted code can you just make a single method?

You could make checkMultiSelection() take a parameter (or parameters) to control whether it clicks the OK button or the cancel button. Or alternately you could move the code that's specific to each test out of the shared function and into the tests themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

About your first comment: yes, it was a copy-paste mistake.
About your second comment: agree with you. A passed parameter could be enough to split the code and to keep the function still clear.

.execute(executeExpr)
.then(function (comboState) {
checkComboState(comboId, comboState,
{ // expected combo state
inputNodeValue: "Germany",
popupInputNodeValue: "", // invisible
widgetValue: ["Germany"],
valueNodeValue: "Germany",
opened: true,
selectedItemsCount: 1,
itemRenderersCount: 37,
inputEventCounter: 1,
changeEventCounter: 0,
widgetValueAtLatestInputEvent: ["Germany"],
valueNodeValueAtLatestInputEvent: "Germany",
widgetValueAtLatestChangeEvent: undefined,
valueNodeValueAtLatestChangeEvent: undefined
}, "after clicking option (Germany))");
})
.end()
.findById(comboId + "_item6") // "China"
.click()
.sleep(500) // wait for popup to close
.execute(executeExpr)
.then(function (comboState) {
checkComboState(comboId, comboState,
{ // expected combo state
inputNodeValue: string.substitute(comboState.multipleChoiceMsg, {items: 2}),
popupInputNodeValue: "", // invisible
widgetValue: ["China", "Germany"],
valueNodeValue: "China,Germany",
opened: true,
selectedItemsCount: 2,
itemRenderersCount: 37,
inputEventCounter: 1,
changeEventCounter: 0,
widgetValueAtLatestInputEvent: ["China", "Germany"],
valueNodeValueAtLatestInputEvent: "China,Germany",
widgetValueAtLatestChangeEvent: undefined,
valueNodeValueAtLatestChangeEvent: undefined
}, "after clicking option (China))");
})
.end()
.findByCssSelector(".d-combo-cancel-button")
.click()
.sleep(500) // wait for the async closing of the popup
.execute(executeExpr)
.then(function (comboState) {
checkComboState(comboId, comboState,
{ // expected combo state
inputNodeValue: comboState.multipleChoiceNoSelectionMsg,
popupInputNodeValue: "", // invisible
widgetValue: [],
valueNodeValue: "",
opened: false,
selectedItemsCount: 0,
itemRenderersCount: 37,
inputEventCounter: 1,
changeEventCounter: 1, // incremented
widgetValueAtLatestInputEvent: [],
valueNodeValueAtLatestInputEvent: "",
widgetValueAtLatestChangeEvent: [],
valueNodeValueAtLatestChangeEvent: ""
}, "after clicking on the 'Cancel' button (close)");
})
.end();
};
Expand Down Expand Up @@ -570,6 +690,21 @@ define([
return checkMultiSelection(remote, "combo3");
},

"multi selection selection (combo3)": function () {
var remote = this.remote;

if (remote.environmentType.browserName === "internet explorer") {
// https://github.com/theintern/leadfoot/issues/17
return this.skip("click() doesn't generate mousedown/mouseup, so popup won't open");
}
if (remote.environmentType.platformName === "iOS") {
// https://github.com/theintern/leadfoot/issues/61
return this.skip("click() doesn't generate touchstart/touchend, so popup won't open");
}

return checkMultiSelectionCancel(remote, "combo3");
},

"tab navigation (combo3)": function () {
var remote = this.remote;

Expand Down