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
5 changes: 3 additions & 2 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>
<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>
5 changes: 5 additions & 0 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 Down