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

Fix Safari turning decimal number inputs to 0 #1254

Merged

Conversation

rjackson
Copy link
Contributor

Chrome has some slightly unique input[type="number"] handling behaviour, where it restricts the characters that can be typed to numeric characters. Under-the-hood, Chrome withholds certain invalid input states from being emitted as change events – so we only receive change events for valid numbers.

Safari behaves slightly differently. It allows any characters to be entered and emits a change event on each character press, but its internal "value" is only set if the typed input is a valid number. The change events then either come through with a numeric value represented, or as an empty string.

For example, when typing "12.34" we receive onChange events with "1", "12", "", "12.3", and "12.34".

On that third onChange event when we pass "" up to React, React happily ignores the change (I think bceause the incoming value "" already matches the element's value of ""; nonetheless, React have solved this issue for us).

When we parse the input via Number(v) we encounter problems. Number("") resolves to 0, React sets the input's value to 0, and the user is therefore unable to type decimal values successfully.

The solution for this is not to cast to a number at all. We'll rely on default browser behaviour to manage the input for us (allowing users to enter invalid characters), but we're still safe because the element's internal state will always be numeric.

In the scenario that a customer does try enter an invalid input "abc123", the input field simply blanks itself when they leave it.

/claim #1181

Chrome has some slightly unique input[type="number"] handling behaviour, where it restricts the characters that can be typed to numeric characters. Under-the-hood, Chrome withholds certain invalid input states from being emitted as change events – so we only receive change events for valid numbers.

Safari behaves slightly differently. It allows any characters to be entered and emits a change event on each character press, but its internal "value" is only set if the typed input is a valid number. The change events then either come through with a numeric value represented, or as an empty string.

For example, when typing "12.34" we receive onChange events with "1", "12", "", "12.3", and "12.34".

On that third onChange event when we pass "" up to React, React happily ignores the change (I _think_ bceause the incoming value "" already matches the element's value of ""; nonetheless, React have solved this issue for us).

When we parse the input via `Number(v)` we encounter problems. `Number("")` resolves to `0`, React sets the input's value to `0`, and the user is therefore unable to type decimal values successfully.

The solution for this is not to cast to a number at all. We'll rely on default browser behaviour to manage the input for us (allowing users to enter invalid characters), but we're still safe because the element's internal state will always be numeric.

In the scenario that a customer does try enter an invalid input "abc123", the input field simply blanks itself when they leave it.
@vercel
Copy link

vercel bot commented May 10, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
rowy-os ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 15, 2023 7:18pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
rowy-typedoc ⬜️ Ignored (Inspect) May 15, 2023 7:18pm

@vercel
Copy link

vercel bot commented May 10, 2023

@rjackson is attempting to deploy a commit to the Rowy Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Contributor

@bofeiw bofeiw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @rjackson, nice fix. It worked in the table editor cell.

  1. Can you apply the change to the side panel editor as well? (the right side panel when you click the expand button on the right)
  2. Noticed that invalid input in Safari (e.g. "12,3") will be saved as empty string in Firebase instead of 0. Can this be fixed?
  3. Noticed that input "e" will reset the number to 0. It might be a conflict with scientific number (e.g. 1e9). Out of scope for this PR and there may not be a simple solution. Do not need fix in this PR, however if you have an idea welcome to contribute.

@rjackson
Copy link
Contributor Author

@bofeiw Apologies, I hadn't spotted the side panel editor!

I've got the PR updated with those changes now. The SideDrawerField works a little differently to EditorCell, so there's a slight difference between the two pieces of code, but the overall pattern is the same.

Whilst a user is typing (onChange), we're acting upon any non-invalid inputs (skipping over empty strings). I've re-added the onChange code blocks even though they're technically unnecessary here, but it lets me add a comment explaining what's going on.

Once a user has finished typing and leaves the field (onBlur), we're then casting the values to Number – in case they weren't already. This ensures the value we store in Firebase is number – either 0, or a valid number.

The EditorCell also supports completing the change via the Enter key, which was still saving empty strings to Firebase. To solve that I've adjusted the Enter-key code to invoke onBlur if we've given it one. As far as I can tell, that now guarantees all inputs will be cast to Number before being saved.

Finally, I've applied the same fixes to the Percentage field type. There are a few other field types that are represented by numbers under-the-hood, but Number and Percentage were the two allowing input via input[type=number] and therefore susceptible to this issue. The other number fields (Slider, Rating) have custom inputs that aren't affected by this bug.

@bofeiw bofeiw merged commit 9999066 into rowyio:develop May 15, 2023
@harinij
Copy link
Member

harinij commented May 16, 2023

Thanks @rjackson for the contribution 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants