Combo Box is a keyboard trap #845
Conversation
|
@snide Can you please add some focus styling to the |
|
@chandlerprall Do want to take a second look? I changed the implementation a lot. The original implementation did not work when the combo box was tabbed into from the reverse order. Now tabbing into the combo box from either direction always has the same behavior. |
| const tabbableItems = tabbable(document); | ||
| const comboBoxIndex = tabbableItems.indexOf(this.searchInput); | ||
| tabbableItems[comboBoxIndex].focus(); | ||
| this.tabOffset = 0; |
There was a problem hiding this comment.
Shouldn't this default to the index of the currently selected item?
| if (this.tabOffset === UNSET) { | ||
| const tabbableItems = tabbable(document); | ||
| const comboBoxIndex = tabbableItems.indexOf(this.searchInput); | ||
| tabbableItems[comboBoxIndex].focus(); |
There was a problem hiding this comment.
this.searchInput.focus() instead of the lookup calculation?
| @@ -293,6 +304,13 @@ export class EuiComboBox extends Component { | |||
| document.addEventListener('click', this.onDocumentFocusChange); | |||
| document.addEventListener('focusin', this.onDocumentFocusChange); | |||
| this.openList(); | |||
|
|
|||
| if (this.tabOffset === UNSET) { | |||
There was a problem hiding this comment.
this.tabOffset is set back to UNSET only if tabbing away from the combo box, if I click away or in any other way focus shifts to another element, I don't think we could trust tabOffset?
|
@nreese @chandlerprall Maybe we can just remove these buttons from the tab order entirely? This would be another way to solve the keyboard trap problem. People using the keyboard don't need to access the "down caret" button to open the combo box since it opens automatically when you tab to it. @aphelionz Can we still consider this component accessible if keyboard users have to hit backspace to delete all content in the combo box instead of using the "Clear all" button? This would simplify things greatly. |
|
If we do want to allow tabbing to the "Clear all" button, then it seems like we could add a |
|
closing in favor of #866 |
fixes #788 and #787