ComboBox has problems with Enter key

[zepumph]

Like we have seen many times in a11y, when using the enter key we need to guard against letting the enter key click a listener, and then again click again on release once that listener focuses something else.

In comboBox this is complicated further because of #446.

[zepumph]

We can't put the guard in like it was before because we don't have access to the listener on the button side anymore, it is buried in the Rectangular push button.

[pixelzoom]

In ComboBox, this currently means that when the listbox is popped up, pressing the Return key changes the selection but does not dismiss the list.

[pixelzoom]

Here's an idea... Make the items in the list (ComboBoxListItemNode) have role (tagName?) 'button', then addInputListener( { click: ... } ) on each item.

[zepumph]

For now @pixelzoom and @zepumph just converted the keydown listener in the list box to use keyup. This way we don't get the event tranfered back to the button. @jessegreenberg this felt like a workaround to me because I thought we wanted to try to make the UX as similar to the browser click as possible, in which a click from enter occurs on keydown. A few things to do from here:

• explore problems with a11y and time-based buttons scenery#931 to see about a general solution for this type of problem (especially as it relates to "fire on hold"
• Discuss with designers like TS if it is ok for us to make "click" functionality come from keyup, and if so when do we allow that?
• Perhaps decide that even if we don't want to use this generally, that it is ok to use this keyup pattern for ComboBox because why would we want the enter key to fire multiple times on the list box? Especially since there is a discrepancy between how "enter" behaves and "space".

@pixelzoom's main question is "why does there need to be a difference between how "space" and "enter" keys are handled in general."

@jessegreenberg what do you think about the conversion to keyup in combo box, and how do you think we should proceed?

[pixelzoom]

Using keyup for the items didn't solve this problem.

We still have a significant problem here, which I described over in phetsims/scenery#939 (comment):

Here's something else that makes no sense to me, observed in the current implementation of ComboBox. If the combo box button has focus, pressing Enter pops up the listbox, and releasing Enter immediately pops down the listbox.

Demonstrated in Molarity:

1. Tab to the combo box, so that its button has focus.
2. Press and hold the Enter key. The button's click listener fires. The listbox pops up and the item that is currently selected is given focus.
3. Release the Enter key. The selected item's keyup listener fires, and the listbox pops down.

So by clicking Enter (which supposedly means "accept") the related events get sent to whatever UI component happens to have focus at the time. That Enter was meant for the button, and only the button. The listbox and its items shouldn't be receiving anything related to that user action.

[pixelzoom]

Deferred until the more general issue of how to handle Enter and Space is resolved, see phetsims/scenery#939.

[pixelzoom]

In the above commit, I added logging of a11y keyboard events for ComboBox and its components. Run Molarity with ?a11y&log and you'll see these events logged to the browser console. This demonstrates the problem that is specific to ComboBox, as well as the more general issues with Enter and Space keys.

It would be great if everyone played around with this before our Wednesday 2/6 conference call, so that we're all up to speed on what the current behavior is.

As noted in the TODO comments in the code, this logging should be removed when this issue is resolved.

[zepumph]

There are a few pieces to this issue, and there are multiple issues around scenery for tackling these problems. In phetsims/scenery@f1ce6d9 (above commit) @jessegreenberg and I fixed the most prominent bug so that the "keyup workaround" when clicking on the button and moving focus to the listbox. is working as expected. Whereas before it was still broken when used with the enter key. So the problem summed up in #447 (comment) is fixed now.

Outline for work to be done (both in scenery generally and as it pertains to ComboBox directly):

• Discuss the ideal design of using both space and enter for "clicking" in How to handle Space vs Enter for buttons scenery#939 (comment)
• Work out a way to support options like fireOnHold in scenery, see problems with a11y and time-based buttons scenery#931

[pixelzoom]

@jessegreenberg @zepumph Nice solution in phetsims/scenery@f1ce6d9, ComboBox seems to be working as desired. The "work to be done" in #447 (comment) is covered by other issues, so...

Do you think it would be OK to close this issue?

And do you think it's OK to remove the phet.log output that I added in #447 (comment) ?

[pixelzoom]

@jessegreenberg @zepumph ping.

[jessegreenberg]

Thanks @pixelzoom, yes this can be closed since it is being tracked in more general issues. It was helpful in discussion, but the phet.log code from 855ec6d can be removed now.

[pixelzoom]

phet.log removed, tested, closing.

[zepumph]

There is one more TODO that I see for this issue:

sun/js/ComboBoxListBox.ts

Line 130 in 1f28f82

//TODO sun#447, scenery#931 we're using keyup because keydown fires continuously

I would like to see if it makes sense to change back to a keydown. I kinda don't think so, but I would like to investigate for a second.

[pixelzoom]

It doesn't sound like I need to be involved in this, so unassigning.

[zepumph]

I do not think that we would want keydown here. All of our buttons fire on keyup. And then we don't have the potential issues with keydown then triggering the button again after listbox selection. Ready to close for me!