Combobox: multiple fixes. #674

brunano21 · 2016-11-28T16:20:06Z

This PR:

Fixes broken placeholder - _Actually I don't remember it was working before the Combobox refactor at _ 785de84.
displayedValue prop refers to the value to display at each user interaction. If it's not provided at instantiation time, it will be retrieved from the source, using the value as key. Only valid if source != null
Delay the widget initialization if the list has not been created yet.
Restore down-arrow key original behaviour, even if the inputNode is empty. This means that if you press down arrow key, the popup will open - (Fixes accessibility issues).
Fixes ComboPopup: cancelHandler does not clear selected items, if any. #677. This bug has been introduced by 785de84
Do not forward selection event to the list if dropdown is not opened. This bug has been there before the refactor at 785de84.
Issue explanation: In case of Combobox configured in "selectionMode=multiple", "Spacebar" events are sent to the list even though the dropdown is closed, resulting in a weird/ambiguous Combobox's selection behavior.
Fixes broken readOnly property. This bug has been there before the refactor at 785de84.
Issue explanation: even if the widget is set i.e. in autoFilter=false OR selectionMode=multiple, at the creation time the widget's inputNode does not have the readonly attribute set, despite the this.notifyCurrentValue("_inputReadOnly") is executed. From my debugging I see that the notification about the _inputReadOnly property is delivered only when the user starts to interact with the widgets, i.e. when the user opens the dropdown. However, this is too late. In fact, if the user gets the focus on the widget via tabbing over the page and then he starts typing, the input is still "no-readonly" mode, allowing so the actual typing.

wkeese · 2016-12-08T05:29:56Z

Combobox.js

+			}
+			if ("displayedValue" in oldValues) {
+				this.inputNode.value = this.displayedValue;
+			}


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

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

wkeese · 2016-12-08T05:30:44Z

Combobox.js

+				if (this._justValidated) {
+					this._justValidated = false;
+				}
+			}


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

this._justValidated = false;

all the time, without the if() statements?

wkeese · 2016-12-08T05:32:44Z

Combobox.js

+				}
+			}
+			return item;
+		},


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.

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

wkeese · 2016-12-08T05:40:29Z

Combobox.js

+					if (this.inputNode.value.length === 0) {
+						if (this.minFilterChars === 0) {
+							this.list.query = this._getDefaultQuery();
+						}


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?

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?

wkeese

Looks like this commit (along with some/all of the other commits) needs tests added.

wkeese

Looks like this needs (automated) tests to be added

wkeese

Hmm, the _updateInputReadOnly() method is strange. The standard way to do this would be to set this._inputReadOnly in computeProperties(), and then do the setAttribute("unselectable", "on") etc. stuff in refreshRendering().

I think that would also avoid the race-condition like issues listed in the comment saying that "FormValueWidget.refreshRendering() mirrors the value of widget.readOnly to focusNode.readOnly”. It shouldn't be an issue anymore because Combobox#refreshRendering() will run after FormValueWidget#refreshRendering().

Also, this needs an automated test case.

wkeese

Looks like this needs a test case too, specifically the part where you make the <input> readonly when the source is null or empty.

These changes also seem tangled up with the previous commit, probably they should be merged together?

wkeese · 2016-12-08T06:00:31Z

Combobox.js

@@ -295,6 +295,8 @@ define([
 		// Flag used for post initializing the widget value, if the list has not been created yet.
 		_processValueAfterListInit: false,

+		_isMobile: false,


I think you can just say:

_isMobile: !!ComboPopup,

wkeese

OK, but needs a test case.

wkeese

I'm not sure what bug this is fixing.

Also, it needs a test case. And probably you shouldn't be commenting out an existing test.

wkeese

Seems reasonable, but as usual it should have a test case. We must already have a test that clicks on a list item so it's just a question of updating that test to check that focus returned to the <input>.

wkeese

As usual, needs a test case (or extension to existing test).

wkeese

Seems like aria-expanded should be set to true/false rather than added/removed? That's how other code is doing it.

Also, as usual, this should be added to the aria tests.

brunano21 · 2016-12-08T16:41:31Z

Regarding the _updateInputReadOnly(), the problem seems to be serious. I already tried a solution similar the one you proposed, a while ago but it did not work. I now tried the exactly what you said, that is to update the _inputReadOnly property within computeProperties() and in refreshRendering() I do setAttribute("unselectable", "on") etc. stuff. Note: _inputReadOnly property is mapped into the Combobox.html template. However, even if _inputReadOnly is true (inspecting the widget) the inputNode does not have the readonly attribute set.

DisplayedValue prop refers to the value to display at each user interaction. Delay the widget initialization if the list has not been created yet. Restore down-arrow key original behaviour, even if the inputNode is empty. Add default placeholder check into unit tests.

…ctedItems. Add tests.

…e popup used to close by itself; it should stay open instead.

…de.value to the widget'sinputNode.value. Redefine logic for displaying/hiding the combo popup's list node.

Add 3 functional tests.

…s. Add 1 unit test.

Moved value initilization code from refreshRendering into computeProperties. Add more functional tests for filtering feature when widget is in auto-complete mode Add unit test for checking the value of readOnly attribute when source is empty. Add unit test for checking the value of readOnly attribute when list's source gets modified.

…(ComboPopup.html).

wkeese · 2016-12-30T05:49:36Z

Combobox/ComboPopup.js

 			this.combobox.closeDropDown();
+			// cont: then ask to validate, so widget's value and inputNode get updated as well.
+			this.combobox._validateMultiple(true);


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

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.

Oh OK I didn't realize that.

wkeese · 2016-12-30T05:54:41Z

tests/functional/ComboPopup.js

+			.end()
+			.findById(comboId + "_item1") // "Germany"
+			.click()
+			.sleep(500) // wait for popup to close


This comment seems wrong, the ComboPopup doesn't close until you click the OK or cancel buttons.
Likewise on line 485.

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.

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.

wkeese · 2016-12-30T06:20:41Z

Combobox.js

+						this._toggleComboPopupList();
+					} else {
+						this.closeDropDown();
+					}


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.

wkeese · 2016-12-30T06:27:10Z

Combobox.js

-				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);


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?

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.

wkeese · 2016-12-30T06:27:32Z

tests/functional/ComboPopup.js

+		// 	}
+
+		// 	return checkListInPopup(remote, "combo3", false, true);
+		// },


Huh? You just commented out a bunch of tests.

wkeese · 2016-12-30T06:27:48Z

Combobox.js

@@ -746,7 +746,7 @@ define([
 					if (this.opened) {
 						this.closeDropDown(true/*refocus*/);
 					}
-				} else if (evt.key === "Spacebar") {
+				} else if (evt.key === "Spacebar" && this.opened) {


Thanks, I pushed this as 7d08bcc.

wkeese · 2016-12-30T07:57:09Z

Combobox.js

@@ -539,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.


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:

The first Combobox in Combobox-decl.html where you can't focus the <input>. What happens then?

Multi-select mode. In that case don't you want to keep focus on the list item?

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?

wkeese · 2016-12-30T11:59:40Z

tests/functional/Combobox.js

+			.then(function (navigatedDescendant) {
+				assert(/^Canada/.test(navigatedDescendant),
+					"navigatedDescendant after other two ARROW_DOWN: " + navigatedDescendant);
+			})


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.

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.

wkeese

Checkin comment should also say:

Fixes #677, refs #668.

wkeese

Hmm, when I ran grunt test:remote I got a failure:

× internet explorer 11 on Windows 8.1 - unit tests - deliteful/Combobox: programatic - inputNode: aria-expanded attribute on open/close popup (0.046s)
AssertionError: aria-expanded - popup is open: HasDropDown_233: expected 'false' to equal 'true'
No stack or location

wkeese · 2016-12-30T12:16:00Z

Combobox.js

@@ -984,6 +984,7 @@ define([
 						}
 						this.dropDown.focus();
 					}
+


This shouldn't be there.

wkeese · 2016-12-30T12:18:52Z

Combobox/ComboPopup.js

+						if ("opened" in oldValues) {
+							this.inputNode.setAttribute("aria-expanded", this.combobox.opened);
+						}
+					}.bind(this));


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.

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.

Filed #687.

wkeese · 2016-12-30T12:19:37Z

tests/unit/Combobox.js

+
+			return d;
+		},
+


Got this error after running grunt test:remote (with this changeset only):

× internet explorer 11 on Windows 8.1 - unit tests - deliteful/Combobox: programatic - inputNode: aria-expanded attribute on open/close popup (0.046s)
AssertionError: aria-expanded - popup is open: HasDropDown_233: expected 'false' to equal 'true'
No stack or location

Does this happens only in IE11?
IIRC, there were some issues with events and IE, so some other tests were disabled for this environment.

wkeese · 2017-01-16T00:37:02Z

Combobox/ComboPopup.html

 		<input d-hidden="{{this.combobox._inputReadOnly}}"
+			aria-labelledby="{{_tag}}-{{widgetId}}-header"


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.

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.

wkeese · 2017-01-16T03:32:48Z

Combobox/ComboPopup.html

 		<input d-hidden="{{this.combobox._inputReadOnly}}"
+			aria-labelledby="{{_tag}}-{{widgetId}}-header"


Filed #686.

wkeese · 2017-01-16T11:22:23Z

Combobox.js

@@ -684,7 +706,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;


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

wkeese · 2017-01-27T00:52:27Z

I finally got this all pushed, except for the one change we decided not to do.

Commits are efc19c5, ad9a031, 5de89a3, 6f5417d, 3196136, 70f2142, 707d1ac, 11bc7ff, d9af56b, 2c3dbb2, 668271c, ada6275, 933f1f6, 87c92d6, 9cfe2a1.

brunano21 force-pushed the combobox_init branch from 595af40 to ce7dd6e Compare December 7, 2016 11:53

wkeese requested changes Dec 8, 2016

View reviewed changes

brunano21 added 10 commits December 13, 2016 11:25

Fix Cancel handler for ComboPopup, which wasnt clearing eventual sele…

b0199b3

…ctedItems. Add tests.

Do not forward selection event to the list if dropdown is not opened.

ff26def

Fix autocomplete mode bug: when inputNode was empty and in mobile, th…

23c5824

…e popup used to close by itself; it should stay open instead.

In not-autocomplete mode (hasDownArrow=true), set the popup's inputNo…

80e6270

…de.value to the widget'sinputNode.value. Redefine logic for displaying/hiding the combo popup's list node.

Refocus inputNode if user clicks on list's item.

3f36ca1

Focus any previously focus list's item when opening the dropdown.

50a0edf

Add 3 functional tests.

Set aria-expanded=true|false attribute when the popup opens and close…

95a0fb2

…s. Add 1 unit test.

jshint fixes.

49fabb6

brunano21 force-pushed the combobox_init branch from 138edf1 to 49fabb6 Compare December 13, 2016 11:43

Add aria-expanded and aria-describedby attributes for mobile version …

1ff8cb7

…(ComboPopup.html).

wkeese reviewed Dec 30, 2016

View reviewed changes

Combobox.js

@@ -984,6 +984,7 @@ define([

}

this.dropDown.focus();

}

Copy link

Member

wkeese Dec 30, 2016

Choose a reason for hiding this comment

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

This shouldn't be there.

wkeese reviewed Dec 30, 2016

View reviewed changes

wkeese reviewed Jan 16, 2017

View reviewed changes

Combobox/ComboPopup.html

<input d-hidden="{{this.combobox._inputReadOnly}}"

aria-labelledby="{{_tag}}-{{widgetId}}-header"

Copy link

Member

wkeese Jan 16, 2017

Choose a reason for hiding this comment

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

Filed #686.

wkeese reviewed Jan 16, 2017

View reviewed changes

wkeese closed this Jan 27, 2017

		<input d-hidden="{{this.combobox._inputReadOnly}}"
		aria-labelledby="{{_tag}}-{{widgetId}}-header"

Combobox: multiple fixes. #674

Combobox: multiple fixes. #674

Conversation

brunano21 commented Nov 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brunano21 Dec 8, 2016 • edited Loading

Choose a reason for hiding this comment

wkeese left a comment

Choose a reason for hiding this comment

wkeese left a comment

Choose a reason for hiding this comment

wkeese left a comment • edited Loading

Choose a reason for hiding this comment

wkeese left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wkeese left a comment

Choose a reason for hiding this comment

wkeese left a comment

Choose a reason for hiding this comment

wkeese left a comment

Choose a reason for hiding this comment

wkeese left a comment

Choose a reason for hiding this comment

wkeese left a comment

Choose a reason for hiding this comment

brunano21 commented Dec 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wkeese Dec 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wkeese Dec 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wkeese left a comment

Choose a reason for hiding this comment

wkeese left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wkeese Jan 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wkeese Dec 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wkeese commented Jan 27, 2017

brunano21 commented Nov 28, 2016 •

edited

Loading

brunano21 Dec 8, 2016 •

edited

Loading

wkeese left a comment •

edited

Loading

wkeese Dec 30, 2016 •

edited

Loading

wkeese Dec 30, 2016 •

edited

Loading

wkeese Jan 13, 2017 •

edited

Loading

wkeese Dec 30, 2016 •

edited

Loading