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

Bug fix: Prevent event propagation on invalid key presses #512

Merged
merged 1 commit into from
Jul 13, 2023

Conversation

nicolasgasco
Copy link

@nicolasgasco nicolasgasco commented Jul 13, 2023

Hello! I noticed a behaviour that looks a bit weird to me and led to a bug ticket in Production (here for Qonto employees).

When @numberOfDecimal is set to 0 (so @value should always be an integer), following filtering logic is used for e, ,, and . key presses:

// ember-amount-input/src/components/amount-input.js
  @action
  onKeyDown(event) {
    if (event.keyCode === KEY_CODE_E) {
      return false;
    } else if (
      this.numberOfDecimal === 0 &&
      [KEY_CODE_FULLSTOP, KEY_CODE_COMMA].includes(event.keyCode)
    ) {
      return false;
    }
  }

But, since the events are not prevented from bubbling, you can actually type those characters in the input. The value is then converted to an integer on focus out. This leads to some weird states because inputs like 10.5 are actually NaN, while 10,5 would be correctly rounded to 10.

You can see here how the input behaves before and after preventing events from bubbling.
With propagation:

TestApp.Tests.-.13.July.2023.mp4

Without propagation:

TestApp.Tests.-.13.July.2023.1.mp4

I think preventing the event from propagating would make the component behave in a more rational way: if the input value is an integer, you shouldn't even be allowed to type e or a comma in the same way you can't type a letter or any other symbol.

Note on testing: I tried modifying the existing tests, but it seems that typeIn testing helper doesn't actually work like manually typing, so I don't think there's really a way to test this behaviour. The changes added in the PR don't make e, . and , key should be masked when numberOfDecimal={{0}} test fail.

@nicolasgasco nicolasgasco changed the title Bug fix: Prevent event propagation on certain key presses Bug fix: Prevent event propagation on forbidden key presses Jul 13, 2023
@nicolasgasco nicolasgasco changed the title Bug fix: Prevent event propagation on forbidden key presses Bug fix: Prevent event propagation on invalid key presses Jul 13, 2023
@vscav
Copy link
Member

vscav commented Jul 13, 2023

Hi @nicolasgasco! I followed the discussion you had with Marcin - did you try this solution on multiple browsers? Other than that, LGTM 👍

@nicolasgasco
Copy link
Author

Hi @nicolasgasco! I followed the discussion you had with Marcin - did you try this solution on multiple browsers? Other than that, LGTM 👍

As far as the scope of this PR is concerned, behaviour is consistent in Chrome, Firefox, and Safari: e, ,, and . presses are ignored.

The only inconsistent behaviour is that Firefox and Safari actually allow all kind of key presses, so you could input a letter for example. But this is already the case.

@vscav vscav merged commit 073f310 into qonto:master Jul 13, 2023
10 checks passed
@vscav vscav requested review from vscav and removed request for vscav July 13, 2023 14:11
@vscav vscav added the enhancement New feature or request label Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants