-
Notifications
You must be signed in to change notification settings - Fork 273
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.
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
feat(ui5-multiinput, ui5-multi-combobox): implement keyboard handling #2166
feat(ui5-multiinput, ui5-multi-combobox): implement keyboard handling #2166
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is getting closer to what it is expected to be, I found few issues, described below:
Both components:
- if I click on a token, the item navigation currentIndex is not being updated, and as a result the arrow keys no longer navigates between the tokens.
For example, open http://localhost:8080/test-resources/pages/MultiComboBox.html and use the 1st MultiComboBox, click on the second token directly and try using the arrows.
MultiInput
- We have double focus on entering the component with TAB
MultiComboBox
- First TAB directly moves the focus to the 1st token, perhaps should go to the entire input first?
- Pressing ArrowRight from the last token goes to the input (so you can start typing) as expected, but the focus outline is missing.
@@ -114,6 +135,14 @@ class Token extends UI5Element { | |||
this.i18nBundle = getI18nBundle("@ui5/webcomponents"); | |||
} | |||
|
|||
_handleSelect() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should fire an event both cases - when the user selects and unselects a token. So, _handleEvent and _select can be combined in one:
this.selected = !this.selected;
this.fireEvent("select");
I saw that we have such event name in the RadioButton already. In the CheckBox we have "change", let's keep it "select" for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting better, still something to look at:
Both components
Now when the user clicks on a token, the arrow navigation is working as expected. But, if the user does not click the token and just focuses the token by performing mousedown and then drags the mouse outside the token and releases the mouse (mouseup) outside the token, we have the same issue. Perhaps we should update on focusin/mousedown to handle that case.
Both components
When I select a token and then use the arrows to navigate between the tokens, the selection is lost
MultiComboBox
The MultiCombobox lacks of focus outline, both upon entering with TAB and by clicking in the input field.
Now I saw that this is a bug in the master and it is not exactly in the scope of the BLI, so I will accept the change without it.
return this._handleLeft(event); | ||
} | ||
|
||
this._skipOpenSuggestions = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initialize the flag in the constructor and explain what is needed for in a comment
…SAP#2166) Navigation between the tokens is now possible with the Arrow keys both in MultiComboBox and MultiInput components.
…#2166) Navigation between the tokens is now possible with the Arrow keys both in MultiComboBox and MultiInput components.
…#2166) Navigation between the tokens is now possible with the Arrow keys both in MultiComboBox and MultiInput components.
FIXES: #1534
Imporoved keyboard handling for both components according to the specifications, making it consistent with the UI5 Controls.