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

feat: prevent minus sign typing #794

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

AntoineMoues
Copy link
Contributor

@AntoineMoues AntoineMoues commented Apr 11, 2024

Description

This PR is a proposal to solve the issue #793.
The issue concerns an issue with input with type number that allows minus sign to be typed but can lead to empty values. To avoid this behavior, this PR proposes to prevent default behavior when user presses any - sign.

💡 As the issue was not reviewed or discussed yet, this PR just act as a proposal around imagined solution. Feel free to share any thoughts in the issue regarding this idea.

As - sign has multiple keycodes (depending on keyboard layout, numpad, etc.) and keyCode is deprecated, this PR uses key property instead.

Also, we can imagine scenarii where prevent the usage of this symbol is not convenient, the removal is only available behind a argument integerOnly which is set to false by default.

@AntoineMoues AntoineMoues added the enhancement New feature or request label Apr 11, 2024
@AntoineMoues AntoineMoues self-assigned this Apr 11, 2024
@nicolasgasco
Copy link

I'm wondering how effective it is to hide this fix behind a new argument that is false by default 🤔 This means that whoever uses this library would need to go back to their code and add this new argument.

Can we confidently say that we NEVER want to have a - sign that is not in first position? In that case, I don't think we need the new argument at all.

@m-jovan
Copy link

m-jovan commented Apr 15, 2024

Do we want to handle copy pasting - sign? Do you think an input mask would be useful instead of strictly checking for key?

@AntoineMoues
Copy link
Contributor Author

Do we want to handle copy pasting - sign?

I think that's also something we would like too. I will add it to the issue and add it to the onPaste method.

Do you think an input mask would be useful instead of strictly checking for key?

I think this might be the best solution here indeed. It would solve the issue raised by @nicolasgasco while also fixing some cross-browser issues (i. e. Firefox does not do any prevention on characters)

@m-jovan
Copy link

m-jovan commented Apr 15, 2024

Do you think an input mask would be useful instead of strictly checking for key?

I think this might be the best solution here indeed. It would solve the issue raised by @nicolasgasco while also fixing some cross-browser issues (i. e. Firefox does not do any prevention on characters)

I agree. In that case do you want to explore it in this PR?

@mrloop
Copy link
Member

mrloop commented Apr 15, 2024

The code would be simpler and less error prone if we didn't mix both KeyboardEvent.code and KeyboardEvent.keyCode. I'd suggest another MR first moving current implementation to KeyboardEvent.code, then this MR adding additional functionality.

@mrloop
Copy link
Member

mrloop commented Apr 15, 2024

The code would be simpler and less error prone if we didn't mix both KeyboardEvent.code and KeyboardEvent.keyCode. I'd suggest another MR first moving current implementation to KeyboardEvent.code, then this MR adding additional functionality.

Also we should make this a major version release, as we'd be dropping browser older browser support. Is KeyboardEvent.code supported by all our target browser version where we use ember-amount-input?

@AntoineMoues AntoineMoues force-pushed the prevent-minus-sign branch 2 times, most recently from 822fd99 to de8a93d Compare April 16, 2024 09:12
@AntoineMoues
Copy link
Contributor Author

Hi! Thanks a lot for all your inputs. I took every of them to improve this PR but the results of the exploration has been pretty disappointing for now.

Input mask

I tried to implement an input mask on it, but doing so on an <input type="number"> has been painful as some options aren't available, for example the pattern attribute.

I also thought about having a validation in the onInput method. However, when trying to access to event.target.value when the value is invalid (for instance "-1"), the returned value is null. This method cannot be used then.

Do not use an arg and only prevent minus if not on first place

This is a great idea but once again that did not succeed. Without possibility of using patterns, either on input or JavaScript, we could rely on selectionStart property from HTMLInputElement. But this property does not return any value if the input is of type number.

An other approach could be to prevent minus sign to be typed in if target.value is not null. The downside of this is the fact that it won't be possible to navigate with arrow keys and add a minus at the start.

Using KeyboardEvent.code

As I did not want to drop support of older browsers only for this feature, I added check that mixes usage of KeyboardEvent.code and KeyboardEvent.keyCode which provides stability and reliability for newest browsers, while supporting older browsers.

What's next

After explorations, the solutions imagined afters comments would require a bigger rework, like switching to <input type="text"> with masking. This is a huge rework, at risk.

Then, I would suggest a smaller and enclosed solution like this one.

WDYT?

cc @nicolasgasco @m-jovan @mrloop

@nicolasgasco
Copy link

Do not use an arg and only prevent minus if not on first place

This is a great idea but once again that did not succeed. Without possibility of using patterns, either on input or JavaScript, we could rely on selectionStart property from HTMLInputElement. But this property does not return any value if the input is of type number.

An other approach could be to prevent minus sign to be typed in if target.value is not null. The downside of this is the fact that it won't be possible to navigate with arrow keys and add a minus at the start.

WDYT?

cc @nicolasgasco @m-jovan @mrloop

Aren't we keeping track of the input value anywhere? If not, how hard would it be to do it? If I understand correctly, the issue here is that we don't have a way to know the current value of the input.

@AntoineMoues
Copy link
Contributor Author

Aren't we keeping track of the input value anywhere?

We are actually doing it in onInput method. We can also access it anywhere event is accessible

If not, how hard would it be to do it?

It's here that there's the issue. The event.target.value and the input.value is set to a blank string whenever an invalid value is entered (for instance "100-"), meaning that you can't evaluate, parse or compute it for instance

@nicolasgasco
Copy link

Aren't we keeping track of the input value anywhere?

We are actually doing it in onInput method. We can also access it anywhere event is accessible

If not, how hard would it be to do it?

It's here that there's the issue. The event.target.value and the input.value is set to a blank string whenever an invalid value is entered (for instance "100-"), meaning that you can't evaluate, parse or compute it for instance

And can we have a reference of the value before the - is inserted? If we're preventing - from being inputted in position other than 0, we would never have 100-.

@AntoineMoues
Copy link
Contributor Author

And can we have a reference of the value before the - is inserted? If we're preventing - from being inputted in position other than 0, we would never have 100-.

Yes it's possible ! However, it's not possible to know which position the character is typed in as selectionStart does not exists for input of type number.

However I also suggested this :

An other approach could be to prevent minus sign to be typed in if target.value is not null. The downside of this is the fact that it won't be possible to navigate with arrow keys and add a minus at the start.

If the downside is considered as reasonable then we can move with this approach

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.

4 participants