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

KeyBoardEvents cause error on serverside with IE Edge #4112

Closed
mikotin opened this issue May 14, 2018 · 7 comments
Closed

KeyBoardEvents cause error on serverside with IE Edge #4112

mikotin opened this issue May 14, 2018 · 7 comments
Assignees
Milestone

Comments

@mikotin
Copy link
Contributor

mikotin commented May 14, 2018

KeyBoardEvent requires a "isComposing" property and Edge doesn't give such so using standard keyboard events (KeyDownEvent, KeyPressEvent, KeyUpEvent) will cause exception on creation of given event:

java.lang.IllegalArgumentException: Unable to create an event object of type com.vaadin.flow.component.KeyUpEvent
	at com.vaadin.flow.component.ComponentEventBus.createEventForDomEvent(ComponentEventBus.java:362)

Test:

Input input = new Input();
input.addListener(KeyUpEvent.class, keyUpEvent -> {
	// throws java.lang.IllegalArgumentException: 
	// with Edge
	if (keyUpEvent.getKey().equals(Key.ENTER)) {
		System.out.println("We got enter!");
	}
});

The data that actually comes from client to server:

image

In the end this is (isComposing) only a boolean value so probably it could just fall back to false in this (or other similar) cases.

@mikotin mikotin added the bug label May 14, 2018
@pleku pleku added this to the Before 1.0 RC milestone May 14, 2018
@pleku
Copy link
Contributor

pleku commented May 14, 2018

I think for missing boolean properties we could just set the default to false if it is missing from the json received. I would find this much better DX than the error.

@Legioth
Copy link
Member

Legioth commented May 15, 2018

Another alternative for this particular case would be to change the definition from event.isComposing to !!event.isComposing so that null and undefined would be coalesced to false.

@pleku
Copy link
Contributor

pleku commented May 15, 2018

After a discussion @mikotin we decided to add a way to handle the special cases of missing event data with an optional second parameter for the annotation that states the default value as a String. Thus when facing a missing parameter from client side, Flow will try to convert the value given in the string (if any) to the corresponding type in the event constructor. Eg. for this case, it would be
@EventData("event.isComposing", "false").
It lets you use eg. experimental API event data with all the browsers without being afraid that things will explode. The default usage of the @EventData stays unaffected.

@Legioth
Copy link
Member

Legioth commented May 15, 2018

@EventData("event.isComposing", "false") won't compile, but @EventData(value = "event.isComposing", default = "false") would.

Would support for default values then also be implemented for DomListenerRegistration.addEventData which the @EventData annotation is based on?

@pleku
Copy link
Contributor

pleku commented May 15, 2018

Oops for the first one, I don't see why not (adding overrides) for the second one.

Also we should update the related tutorials https://github.com/vaadin/flow-and-components-documentation/search?l=AsciiDoc&q=EventData&type=

@Legioth
Copy link
Member

Legioth commented May 17, 2018

What would happen if there are multiple addEventData calls or @EventData annotations that define different default values for the same expression?

@Legioth
Copy link
Member

Legioth commented May 17, 2018

Concluded that instead of allowing to explicitly configure default values, we should simplify this by explicitly checking for primitive @EventData parameters and in that case convert a missing or null value to the corresponding default value (i.e. false, 0 or 0.0 for the three types that are currently supported). These are also the values that you get with asBoolean() or asNumber() from a JsonNull value.

If the developer wants to distinguish between e.g. false and a missing value, then they can change the parameter type from boolean to Boolean and handle a null value in their own code instead.

mikotin added a commit that referenced this issue May 18, 2018
Added primitive checking on event data value decoding, so that primitive values get default values instead of null.

Fixes issue #4112
denis-anisimov pushed a commit that referenced this issue May 24, 2018
* Add primitive default data support on evendata

Added primitive checking on event data value decoding, so that primitive values get default values instead of null.

Fixes issue #4112

* Remove Pair usage

Removed Pair -usage and moved primitive casting and checking
prior to decode -call

* Removed unused import

* Added primitives support to JsonCodec.decodeAS

- Added primitives support toJsonCodec.decodeAS
- Added tests for primitives
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants