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

WebUI: fix incorrect behavior of ESC button on combobox #368

Closed
wants to merge 2 commits into from

Conversation

pvomacka
Copy link

@pvomacka pvomacka commented Jan 4, 2017

When combobox is opened then ESC key should close it. There was a bug
that ESC key closed also the dialog. It was caused by bad keyboard event
handling. The CB was closed by keydown event and the dialog by keyup.

Therefore the propagating of keyup and keydown event is stopped when CB
is opened (when the event is fired on CB element).

https://fedorahosted.org/freeipa/ticket/6388

Pavel Vomacka added 2 commits January 4, 2017 12:21
Adder dialog is mixed with confirmation_mixin. That mixin calls on_cancel method
when closing dialog using ESC key. Previously the on_cancel method
was not defined, therefore dialog was not correctly closed. This was the root
cause of the bug, that adder dialog cannot be opened after closing it using ESC.

Now the default function for on_cancel is dialog.close. So dialog
is correctly closed.

https://fedorahosted.org/freeipa/ticket/6388
When combobox is opened then ESC key should close it. There was a bug
that ESC key closed also the dialog. It was caused by bad keyboard event
handling. The CB was closed by keydown event and the dialog by keyup.

Therefore the propagating of keyup and keydown event is stopped when CB
is opened (when the event is fired on CB element).

https://fedorahosted.org/freeipa/ticket/6388
@pvoborni
Copy link
Member

Code LGTM, but I did not tests the behavior, so cannot give ACK now.

@MartinBasti
Copy link
Contributor

Works for me

@pvoborni pvoborni added the ack Pull Request approved, can be merged label Feb 17, 2017
@pvoborni
Copy link
Member

ACK given that Martin did functional testing

@martbab martbab added the pushed Pull Request has already been pushed label Feb 17, 2017
@martbab martbab closed this Feb 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
4 participants