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

Class component.Key is practically unsable with KeyDownEvent #4626

Closed
Bloodust opened this issue Sep 19, 2018 · 11 comments
Closed

Class component.Key is practically unsable with KeyDownEvent #4626

Bloodust opened this issue Sep 19, 2018 · 11 comments

Comments

@Bloodust
Copy link

Goal: Submit form when Enter key is pressed Or Call a method when Enter key is pressed

Code with comments that explain the insanity:

password.addKeyDownListener(new ComponentEventListener<KeyDownEvent>() {

	@Override
	public void onComponentEvent(KeyDownEvent event) {
		if(event.getKey().equals(Key.ENTER)) //equals not implemented, doesnt work
			pressed();
		if(event.getKey().matches(Key.ENTER.toString())) //toString not implemented, doesnt work
			pressed();
		if(event.getKey().getKeys().containsAll(Key.ENTER.getKeys())) //works but come on this is stupid
			pressed();
	
	}
});

Key class is not actually a Key class, its Keys class because it contains multiple Keys inside of itself. I can understand the need for this when pressing down multiple keys eg. "ctrl+enter" but I dont think this is the way to do it.
Key need implement equals and toString. Key.Enter.equals(Key.Enter) makes sense.

@heruan
Copy link
Member

heruan commented Sep 19, 2018

I don't fully understand what you are describing, but you can use addKeyDownListener(Key key, ComponentEventListener<KeyDownEvent> listener) to specify the key without checking it yourself, e.g.

password.addKeyDownListener(Key.ENTER, event -> pressed());

Methods equals and toString are not implemented because Key is a functional interface, backed by a list of strings which are all the possibile denominations of that key in different browsers. If you look at the JavaDocs and source code, you'll find also a matches(String key) method you can use as a shortcut to check if the Key instance matches a certain key denomination.

@Bloodust
Copy link
Author

My bad, I didn't see the modifiers were an optional parameter. This solves my main issue.

This does not however solve the issue that matches(String key) method is cumbersome to use since you cannot ask Key for its String representation. If one wishes to use matches(String key) one needs to write key.matches("Enter") instead of key.matches(Key.Enter) or key.matches(Key.Enter.toString()) and hope that Enter key definition never changes from "Enter" to for example "ENTER"

@heruan
Copy link
Member

heruan commented Sep 20, 2018

The matches method is intended to match strings coming from the browser, that's why it accepts a string. An overload of the method accepting another Key could be a shortcut, but do you feel there are real use cases where to use that?

@Bloodust
Copy link
Author

I see. I just feel like the interface is unintuitive. Being unable to compare one key to another easily.
Enter key is the only key I've needed to listen so far and I don't see myself needing to capture a broader set of keys in the future.

I think the use case is when a user wants to manually check what key was pressed. I guess the issue then comes down to "When would someone want to do this?" but I don't know. Other than the inability to use this password.addKeyDownListener(Key.ENTER, event -> pressed());

@mvysny
Copy link
Member

mvysny commented Jun 2, 2020

I agree with @Bloodust :

  1. the API is counter-intuitive and feels obfuscated (I'd expect an Enum constant since a set of keys is usually a finite set), definitely not a closure.
  2. The API is hard to use (it's really to write a code for "the key I received, is it an Enter key?"). I should be able to use event.code == Key.Enter instead of event.code.matches("Enter"). The API basically leaks the uncertainty of JavaScript of having multiple key codes for the same key.
  3. The performance suffers because of that: addKeyDownListener() should take advantage of DomListenerRegistration.filter() so that the keys are filtered client-side; instead all key strokes are sent to the server and are filtered server-side.

This API is completely broken by design. There's even ShortcutRegistration.HashableKey class to work around the broken equals()/hashCode()....... wat

@PAX523
Copy link

PAX523 commented Jul 30, 2020

It would be helpful if you at least provide convenient checker methods in Key and KeyModifier that allow me to verify that a specific Key and a specific KeyModifier were pressed as indicated by the key event listener methods.

For instance, something like:

  private boolean isKeyPressed(final KeyDownEvent event, final Key triggerKey, final KeyModifier triggerKeyModifier) {
    return triggerKey.getKeys().equals(event.getKey().getKeys()) && event.getModifiers().stream()
      .allMatch(kms -> kms.getKeys().stream().allMatch(km -> triggerKeyModifier.getKeys().contains(km)));
  }

@pleku
Copy link
Contributor

pleku commented May 19, 2021

One thing I don't really understand here is that why to use KeyDownListener instead of registering a Shortcut with for example https://vaadin.com/api/platform/14.6.1/com/vaadin/flow/component/Shortcuts.html#addShortcutListener-com.vaadin.flow.component.Component-com.vaadin.flow.component.ShortcutEventListener-com.vaadin.flow.component.Key-com.vaadin.flow.component.KeyModifier...-

Please elaborate @Bloodust @PAX523 @mvysny ?
(the API did not exist when this issue was made but the comments from 2020 are after they do exist)

@denis-anisimov denis-anisimov self-assigned this Sep 14, 2021
@denis-anisimov denis-anisimov added this to Needs triage in OLD Vaadin Flow bugs & maintenance (Vaadin 10+) via automation Sep 14, 2021
vaadin-bot added a commit that referenced this issue Sep 16, 2021
… in "of" (#11837) (#11875)

fixes #8474 and #4626

Co-authored-by: Denis <denis@vaadin.com>
vaadin-bot added a commit that referenced this issue Sep 16, 2021
… in "of" (#11837) (#11876)

fixes #8474 and #4626

Co-authored-by: Denis <denis@vaadin.com>
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with platform 22.0.0.alpha4 and is also targeting the upcoming stable 22.0.0 version.

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with platform 21.0.2.

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with platform 20.0.8.

@caalador
Copy link
Contributor

caalador commented Dec 9, 2021

Closed by 11837

@caalador caalador closed this as completed Dec 9, 2021
OLD Vaadin Flow bugs & maintenance (Vaadin 10+) automation moved this from Needs triage to Closed Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment