Combobox keyboard navigation. Fixes #362.

[AdrianVasiliu]

Implements #362.
Includes code, doc, and test.
I plan to do further testing and bug fixing tomorrow - I create the PR now to allow review/feedback ASAP.

[AdrianVasiliu]

Remarks:

• Includes a workaround for delite/HasDropDown: opt-out from focusing the dropdown open by keyboard delite#373. Might evolve depending on how this issue will evolve.
• Also includes a change in List.js that I found necessary for the combo navigation while also making sense outside the Combobox use-case.
• Requires the latest delite release (0.6.0-beta3).

[wkeese]

Overall, I think most of the code in this PR should end up being changed/moved from Combobox to List: specifically the handling for scrolling and space key selection..

I know that your goal is just to get Combobox keyboard support for the Thursday deadline. You've achieved that. I just don't see much point to checking in code that we know will be redone later.

[AdrianVasiliu]

Overall, I think most of the code in this PR should end up being changed/moved from Combobox to List: specifically the handling for scrolling and space key selection.

As said in another comment, I believe this would be far less than most of the code.

I know that your goal is just to get Combobox keyboard support for the Thursday deadline. You've achieved that. I just don't see much point to checking in code that we know will be redone later.

Again, I think this isn't about redoing Combobox later, but about making adjustments when (and if) we decide to rework parts of List. Progressing incrementally looks as a reasonable option to me.

Let's take the example of your suggestion to move the "autoscroll" code from Combobox to List. Note that Combobox was already doing auto-scroll before this PR - I refactored the code, but it was basically already there. This is because Combobox wants to autoscroll the List such that the selected element is visible when the dropdown opens - which doesn't seem something List could handle by itself. Of course, Combobox could handle it by calling an autoScroll method on List - but it would still need to handle it in some cases, as it does it before the PR. And this also means the public API of List would need to be increased with a method for autoScroll, which is of a relatively low interest for the standalone List use-case.

By that I mean some of the changes you suggest deserve closer examination, which can be done in a later step, I would think.

[wkeese]

... when (and if) we decide to rework parts of List.

We already agreed months ago that List would support focus-less operation. And it does support it, except for scrolling and handling the SPACE key. These are just bugs that should be fixed, even if no one besides Combobox uses the focus-less operation feature.

Frankly, these seem like trivial bugs to fix. The "auto scrolling" is probably just adding a few lines to either navigateTo() or _descendantNavigateHandler() to call scrollIntoView() for keyboard events. And the problem handling the SPACE key is just a case of using this.navigatedDescendant instead of event.target, at least for keyboard events (might still need to use event.target for mouse).

Let's take the example of your suggestion to move the "autoscroll" code from Combobox to List.

Yes, I'm suggesting that you leave the existing Combobox code (before your PR) to handle the initial scroll, and just add a few lines to List to handle scroll on navigateTo().

As said in another comment, I believe this would be far less than most of the code.

Well, your PR has 117 new/changed lines in Combobox.js (which show up in green on github). Out of those, 29 lines are about the changes to auto-scroll. They would be rolled back.

In addition, in the code for "keynav-child-navigated", I don't follow why you are closing the popup on a navigation event. It should presumably be closed based on a click event or space event. But anyway, the current code is:

                if (this.selectionMode === "single") {
                    if (rend) {
                        if (!this.list.isSelected(rend.item)) {
                            if (evt.triggerEvent &&
                                (evt.triggerEvent.type === "keydown" || evt.triggerEvent.type === "keypress")) {
                                this.list.setSelected(rend.item, true);
                                this._updateScroll(rend.item, true);
                            } else {
                                this.defer(function () {
                                    // deferred such that the user can see the selection feedback
                                    // before the dropdown closes.
                                    this.closeDropDown(true/*refocus*/);
                                }.bind(this), 100); // worth exposing a property for the delay?
                            }
                        }
                        input.setAttribute("aria-activedescendant", evt.newValue.id);
                    } else {
                        input.removeAttribute("aria-activedescendant");
                    }
                } else if (this.selectionMode === "multiple") {
                    if (rend) {
                        if (rend.item && evt.triggerEvent &&
                            (evt.triggerEvent.type === "keydown" || evt.triggerEvent.type === "keypress")) {
                            this._updateScroll(rend.item);
                        }
                        input.setAttribute("aria-activedescendant", evt.newValue.id);
                    } else {
                        input.removeAttribute("aria-activedescendant");
                    }
                }

and at a minimum it would be changed to something like:

                if (rend) {
                    input.setAttribute("aria-activedescendant", evt.newValue.id);
                    if (this.selectionMode === "single") {
                        if (!this.list.isSelected(rend.item)) {
                            if (evt.triggerEvent &&
                                (evt.triggerEvent.type === "keydown" || evt.triggerEvent.type === "keypress")) {
                                this.list.setSelected(rend.item, true);
                            } else {
                                this.defer(function () {
                                    // deferred such that the user can see the selection feedback
                                    // before the dropdown closes.
                                    this.closeDropDown(true/*refocus*/);
                                }.bind(this), 100); // worth exposing a property for the delay?
                            }
                        }
                    }
                } else {
                    input.removeAttribute("aria-activedescendant");
                }

So that's at least 12 more lines changed.

The last thing is the code for handling SPACE, but I don't understand it well enough to know how it would be changed.

In any case, that's around 41 / 117 lines that would be rolled back or rewritten. I agree it's less than "most of the code", but it's still a substantial amount.

Really though, my point is that you shouldn't be introducing workarounds in Combobox due to bugs in List. Just fix the bugs in List.

[AdrianVasiliu]

I don't follow why you are closing the popup on a navigation event. It should presumably be closed based on a click event or space event.

It's not only about when it should be closed, it's also about when it should select the item. Because currently done on navigation events, which are fired by KeyNav upon "pointerdown" and "focusin" (shouldn't KeyNav listen to "click" rather than "pointerdown"? - https://github.com/ibm-js/delite/blob/0.6.0-beta.3/KeyNav.js#L160 - entered ibm-js/delite#374 and ibm-js/delite#375), items get selected on mouse down instead of mouse up. I'm working on that as first priority.

[AdrianVasiliu]

Following the discussion with @wkeese on ibm-js/delite/issues/374, I went with using a "click" handler for mouse interaction while using navigation events only for keyboard interaction (instead of relying on navigation events in both cases).

This reworking is included in c70321a. Together with 485ba54, I think I addressed all open issues raised by @wkeese, except those involving List changes - for which a closer examination is needed, hence I entered #498, #499 and #500 for them.

Note that the updated PR assumes that ibm-js/delite/issues/375 gets fixed (in my testing I used a locally fixed delite/KeyNav).

[wkeese]

I guess this should be working now, but I see many issues on Chrome/mac, in tests/functional/Combobox-decl.html:

• For first Combobox, click the down-arrow icon, then press down arrow key. Instead of looping through options, the page scrolls.
• For first Combobox, click the <input>, then press down arrow key until you get to Canada. Then press SPACE. The entire page scrolls.
• For first Combobox, click the <input>, then press down arrow key until you get to Canada. Then press ENTER. The form submits.
• For first Combobox, click the input area, then press down arrow key repeatedly. Rather than changing which option is inverted (white text on blue background), it just draws a fuzzy focus border. This is inconsistent with how native <select>'s work.

Also, I don't see any testcases for scrolling. Which test are you using to check that?

[AdrianVasiliu]

Thanks for the feedback. Here's how it goes:

• For first Combobox, click the down-arrow icon, then press down arrow key. Instead of looping through options, the page scrolls.

Right, this happens when clicking the arrow, while it doesn't happen when clicking elsewhere in the element, nor when opening the popup using keyboard interaction. Fixed in c93c2cd.

• For first Combobox, click the <input>, then press down arrow key until you get to Canada. Then press SPACE. The entire page scrolls.
• For first Combobox, click the <input>, then press down arrow key until you get to Canada. Then press ENTER. The form submits.

For some reason, I do not reproduce these issues with my Chrome 40 on Mac OS X 10.9.5 (nor with Safari on Mac OS X, nor with Chrome 40 on Win7). This holds before and after c93c2cd.

• For first Combobox, click the input area, then press down arrow key repeatedly. Rather than changing which option is inverted (white text on blue background), it just draws a fuzzy focus border. This is inconsistent with how native <select>'s work.

Are you sure this is for the first Combobox? The first combo is in single selection mode, and I do not reproduce. What you describe corresponds to the third one (starting from the top of the page), which is in multiple selection mode. In multiple mode, the native <select> behaves quite differently, already because it shows always expanded in the page (no popup showing up when clicking). Also, if you try the keyboard interaction with a native <select multiple>', you'll see that each browser goes its own way in terms of shortcuts. Combobox goes with a cross-browser interaction, which is basically List's one: the up/down arrows highlight the currently navigated option, while SPACE toggles the selection state. All in one, it was not a purpose to be similar to the native <select>' in multiple mode, in particular for keyboard interaction that browsers treat inconsistently for native selects. The mouse interaction does go the same way as for native select, in the sense the clicks toggle the selection state of items (no focus border).

Also, I don't see any testcases for scrolling. Which test are you using to check that?

samples/Combobox.html contains a higher number of items thus it's easier to get scrolling by reducing the height of the browser window. I also typically take profit of the debugger console, as I can adjust the height of the page thanks to it.

It's weird that you're calling a function called "_navigationHandler" on click.

Right. Renamed this private method into _clickOrKeyNavHandler (in c93c2cd). Hope it fits better.

Why not put the closeDropDown() call inside the click handler?

If I put it directly in the click handler instead of this function shared by both click and keynav interaction I'd need to repeat (duplicate) the check of selection mode, renderer target etc.

I don't see where the code is for closing the drop down based on enter key, but presumably that should follow the same code path as the click handler.

It is in the keydown handler (this.on("keydown", ...).

[wkeese]

About the problems I saw on the test page that you couldn't reproduce, I guess it my fault because I was testing with scrollIntoView() (in delite/KeyNav), and it was causing unnecessary scrolling

[AdrianVasiliu]

Okay, understood.

[AdrianVasiliu]

But as far as I can tell all you need to duplicate is the line

I'll revisit this point (and others) in the next milestone.

[AdrianVasiliu]

Merged via 179f96d .