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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Click first cell child on space keyup #1739

Merged
merged 3 commits into from
May 28, 2020
Merged

Click first cell child on space keyup #1739

merged 3 commits into from
May 28, 2020

Conversation

yuriy-fix
Copy link
Contributor

Fixes vaadin/vaadin-confirm-dialog-flow#120

Important note: Changes the grid behavior by clicking the cell on Space keyup. However cell is still "activated" on Space keydown.

Copy link
Contributor

@pekam pekam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes a lot of sense, considering that buttons normally get triggered on key up.

I just wonder if active-item changes should follow the same pattern...

EDIT:
One weird behavior caused by using keydown for changing active-item: When you keep holding the space key down, the active-item keeps toggling between null and the focused item (because keydown keeps on firing when the key is being held down).

test/keyboard-navigation.html Outdated Show resolved Hide resolved
@yuriy-fix
Copy link
Contributor Author

I was thinking about that, but explicitly preserved the activeItem behavior in order to avoid breaking changes to existing applications.

Active-item behavior is working this way on master and probably somebody relies on it.
I guess we can discuss changing it and create another issue if needed.

@pekam
Copy link
Contributor

pekam commented May 28, 2020

Yeah, I'm also a bit afraid to make this kind of changes...

But I do think that the case is pretty much the same: To trigger an action, whether that is through a button's click listener or active-item-changed, it should happen consistently on key up.

Thinking about the bug ticket, it might be also a valid use case that you want to open a confirm dialog for some action when "activating" the item anywhere in the row, instead of only via the button in one column. But if you'd open the confirm dialog on active-item-changed event, you would stumble upon this bug again.

I also think that if changing the active-item-changed to trigger on keydown is considered a breaking change, then the same is true for this click behavior. It's just maybe not as risky.

But I'm not sure about my personal opinion, whether this should be changed now or not...

@tomivirkki, I would like to hear your opinion on this, or even more preferably the absolute truth. ;)

Copy link
Contributor

@pekam pekam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

A separate issue will be created for considering the case of activeItem.

@yuriy-fix yuriy-fix merged commit e20bccf into master May 28, 2020
@yuriy-fix yuriy-fix deleted the fix-space-keyup branch May 28, 2020 11:14
yuriy-fix added a commit that referenced this pull request May 28, 2020
* Adjust test for space keyup

* Click cell on space keyup

* Enhance the test checking click is not fired on spaceDown
yuriy-fix added a commit that referenced this pull request May 28, 2020
* Adjust test for space keyup

* Click cell on space keyup

* Enhance the test checking click is not fired on spaceDown
yuriy-fix added a commit that referenced this pull request May 28, 2020
* Adjust test for space keyup

* Click cell on space keyup

* Enhance the test checking click is not fired on spaceDown
yuriy-fix added a commit that referenced this pull request May 28, 2020
* Adjust test for space keyup

* Click cell on space keyup

* Enhance the test checking click is not fired on spaceDown
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confirmdialog triggers confirmation without "real" user interaction
3 participants