parttio / textfieldformatter

TextField Formatter Vaadin add-on
Apache License 2.0
13 stars 9 forks source link

fix: update TextField component value after Cleave changes value on internal Input element #68

Closed bwajtr closed 1 year ago

bwajtr commented 1 year ago

Hi, I'm Bretislav from the Vaadin team. Vaadin is interested in fixing this issue and therefore we looked into it.

It seems that there is truly a regression from https://github.com/johanneshayry/textfieldformatter/commit/073f3eb7b8d13370c008c5ba112cbc3cc17891fb which attached Cleave.js to the underlying <input> instead of the vaadin-text-field element.

The issue here is then the order of input event processing. There are two input event handlers attached to the underlying <input> element:

  1. the first one comes from the Vaadin and watches for value changes - if the value is changed in the <input>, the value is copied to the value property of the vaadin-text-field. It is then this value of vaadin-text-field which is sent to the server as a new value.
  2. Second input event handler is the Cleave one -> it checks the new value and modifies it according to the chosen format. It then sets the new value to the underlying <input> element. But new value is not automatically copied also to the vaadin-text-field value.

So Cleave does its job, but the new value is not copied to the vaadin-text-field component. Incorrect value is then sent to the server, which is causing validation issues and client-server value synchronization issues.

The fix is to update the vaadin-text-field value after Cleave finished processing. I've added onValueChanged handler to Cleave which seems to be a good place where to do this.

I've tested with existing tests and all seems to be ok. I've also tested if binder validation works (including testing with various Value Change Modes) and all seems to be fine.

This PR fixed these two issues:

Fixes https://github.com/johanneshayry/textfieldformatter/issues/66 Fixes https://github.com/johanneshayry/textfieldformatter/issues/56

bwajtr commented 1 year ago

@johanneshayry I humbly ask for review, and if OK for merge and new release of the AddOn :)

johanneshayry commented 1 year ago

Thank you! I’ll try to have time for this as soon as possible.

ke 31.5.2023 klo 14.50 Břetislav Wajtr @.***> kirjoitti:

@johanneshayry https://github.com/johanneshayry I humbly ask for review, and if OK for merge and new release of the AddOn :)

— Reply to this email directly, view it on GitHub https://github.com/johanneshayry/textfieldformatter/pull/68#issuecomment-1570044752, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJIX6WU3ZFXFTUQVF637ELXI4V7DANCNFSM6AAAAAAYVJLWTQ . You are receiving this because you were mentioned.Message ID: @.***>

bwajtr commented 1 year ago

Hi @johanneshayry, any news on this? We need to release the fix, but we would like to avoid forking your Addon and creating another copy of it to the Vaadin Directory...

If you do not have time for maintaining this Addon anymore, then Vaadin (namely Team Parttio) can take over this repository and the entry in the Vaadin directory.

Thanks, and let me know if you are interested in transferring the ownership...

johanneshayry commented 1 year ago

Seems to be that my priorities have shifted too much to keep maintaining this addon. I gladly move the ownership to Team Parttio. Please let me know how to proceed with it.

ti 11.7.2023 klo 13.15 Břetislav Wajtr @.***> kirjoitti:

Hi @johanneshayry https://github.com/johanneshayry, any news on this? We need to release the fix, but we would like to avoid forking your Addon and creating another copy of it to the Vaadin Directory...

If you do not have time for maintaining this Addon anymore, then Vaadin (namely Team Parttio) can take over this repository and the entry in the Vaadin directory.

Thanks, and let me know if you are interested in transferring the ownership...

— Reply to this email directly, view it on GitHub https://github.com/johanneshayry/textfieldformatter/pull/68#issuecomment-1630555044, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJIX6SCFN7FOMECI2HKAADXPURVVANCNFSM6AAAAAAYVJLWTQ . You are receiving this because you were mentioned.Message ID: @.***>

samie commented 1 year ago

Hi! I added johanneshayry as member, so you should be able to transfer the repo.

mrgreywater commented 1 year ago

Regarding this PR. I actually depend on the fact that the value does not change to the formatted value, but instead holds the raw unformatted value. If this is merged, I'd prefer if either there is a way to get the raw value (wrapper of cleave getRawValue) or if there was an option so one can choose if the input element uses the formatted or unformatted value (wrapper of cleave swapHiddenInput https://github.com/nosir/cleave.js/issues/397 )

bwajtr commented 1 year ago

@mrgreywater Thanks for the input. I'm not sure if I understand your usecase correctly. However, this MR primarily focuses on a bug in value synchronization -> to make sure that what the user sees in the browser is also available as value on the server - e.g. when the formatter restricted the length of the text field input to 5 characters and you pasted in a text with 10 characters using the clipboard, then the client eventually showed 5 characters - however the server received 10 characters.

Anyway, I've added a Java API method getRawValue to get the raw value from the client anyway. Hopefully, you'll find it useful.