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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tab skipping on multiselect field #2137

Merged
merged 2 commits into from Oct 7, 2019
Merged

Fix tab skipping on multiselect field #2137

merged 2 commits into from Oct 7, 2019

Conversation

afbora
Copy link
Member

@afbora afbora commented Sep 25, 2019

Describe the PR

This issue occurred because the multiselect field did not contain an element that could be focused.
I've assigned the element tabindex: 0* on mounted and the enter key to open it when focused.

Since I assigned the enter key to open it, it conflict (refocusing) with the enter key when selecting an item (MultiselectInput.vue). I've found the source and solution of the issue, but I'm waiting for your reviews about this conflict. We can either change the key to open it while focus or remove the enter key when selecting an item 馃

Tested on 3.2.4 and 3.2.5

Only suggestion as draft PR. If it is an insufficient solution, we can close it.

Reference solution: shentao/vue-multiselect#401

*means that the element should be focusable in sequential keyboard navigation, but its order is defined by the document's source order.

Related issues

Ready?

  • Added unit tests for fixed bug/feature
  • Passing all unit tests
  • Fixed code style issues with CS fixer and composer fix
  • Added in-code documentation (if neeed)

@afbora afbora added needs: discussion 馃棧 Requires further discussion to proceed priority: low 馃悓 type: bug 馃悰 Is a bug; fixes a bug labels Sep 30, 2019
@bastianallgeier bastianallgeier marked this pull request as ready for review October 7, 2019 11:15
@bastianallgeier bastianallgeier added this to the 3.3.0 milestone Oct 7, 2019
@bastianallgeier
Copy link
Member

It works like a charm!

@bastianallgeier bastianallgeier merged commit 7027e33 into getkirby:develop Oct 7, 2019
@afbora
Copy link
Member Author

afbora commented Oct 7, 2019

Since I assigned the enter key to open it, it conflict (refocusing) with the enter key when selecting an item (MultiselectInput.vue). I've found the source and solution of the issue, but I'm waiting for your reviews about this conflict. We can either change the key to open it while focus or remove the enter key when selecting an item

@bastianallgeier Actually, there was a conflict I was talking about. Don't you think that's a problem?

@afbora
Copy link
Member Author

afbora commented Oct 7, 2019

tab

Actions: TAB + TAB + TAB + ENTER(open multiselect) + ARROW DOWN ... + ENTER (select *)

*conflicting with same action key as enter. After the selecting item, reopening and focus the field instead of keep focused on selected item. Am i wrong?

@bastianallgeier
Copy link
Member

What do you think should happen instead? Should the dropdown stay closed? Maybe @distantnative has more thoughts on this as the creator of the field.

@afbora
Copy link
Member Author

afbora commented Oct 7, 2019

Here original field screencast:

multiselect

As you can see, when an item is selected with enter, you can continue from the same item.

I think we should decide that:

  1. Is the right key to open the field with Enter?
  2. Is the right key to select the item with Enter?
  3. Can both work together without conflicts if both are yes?
  4. If first or second option is No, we can change one of the keys.

@bastianallgeier
Copy link
Member

Thinking some more about it, we should switch to space to open the dropdown. That's the same behaviour as for native select boxes and we avoid the conflict. What do you think?

@afbora
Copy link
Member Author

afbora commented Oct 7, 2019

Thinking some more about it, we should switch to space to open the dropdown.

We have already @keydown.native.space on MultiSelectField. I will conflict again.

https://github.com/getkirby/kirby/blob/develop/panel/src/components/Forms/Input/MultiselectInput.vue#L55

That's the same behaviour as for native select boxes and we avoid the conflict.

We have @keydown.native.enter on MultiSelectField but the default selection key for the native select box is already the enter key, and it is natural that it does not cause any conflicts.

https://github.com/getkirby/kirby/blob/develop/panel/src/components/Forms/Input/MultiselectInput.vue#L54

Let me work on whether the both can work together without conflict.
Of course, @distantnative reviews are important. I think he has a solution.

@afbora
Copy link
Member Author

afbora commented Oct 10, 2019

@distantnative Can you handle this?

@distantnative
Copy link
Member

Can we create an issue or something that I will remember to look at it on the weekend? I am afraid in this merged and closed PR I will forget it easily.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: discussion 馃棧 Requires further discussion to proceed type: bug 馃悰 Is a bug; fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants