[Numeric Input] Add allowNumericCharactersOnly prop #676

cmslewis · 2017-02-13T08:58:16Z

Fixes aspects of #618

Checklist

Include tests
Update documentation
- Automatically added new prop to component props table
- Added new Numeric characters only toggle to NumericInput basic example

Changes proposed in this pull request:

Motivation

Now that we have both a basic example and an extended example for NumericInput (PR #670), the basic/out-of-the-box example doesn't feel restrictive enough with its character validation (e.g. it allows entry of alpha characters that have no place in a numeric format). The component was designed to mimic the powerful numeric inputs in the Sketch app, but it also makes sense for the component to optionally behave exactly like native input[type="number"] elements.

Change

Thus, the allowNumericCharactersOnly prop was born in this PR. It defaults to true, in which case the component mimics native input[type="number"]s; when this prop is false, the component behaves as it currently behaves, allowing any character to be typed.

Why not avoid the prop and always require numeric characters only?

Because then it would be impossible to add arbitrary additional functionality to the component, such as mathematical expressions, which would need support for characters like * and /, or number abbreviations, which would need support for characters like k, m, b, and so on.

Earlier implementations

I initially hoped I could just use a real input[type="number"] element and defer all validation logic to it (only if allowNumericCharactersOnly was true). I tried this in two ways, both of which proved unworkable:

Replace the input[type="text"] with an input[type="number"], and let the user type into it directly.
Keep using input[type="text"] for the visible text field, but pass events down to a hidden input[type="number"] to get sanitization behavior for free whenever the input text changes.

Option 1 was bad for two reasons: (1) setSelectionRange doesn't work in input[type="number"] (link), which makes impossible our current selectAllOnFocus and "select all on increment" behavior; and (2) styling the native stepper, while mostly doable, is a huge pain that requires different approaches for each browser (link).

Option 2 was bad because you can't "forward" keypresses to an input field programmatically (as if a user had typed them) and expect appropriate events to fire.

Final implementation

So, the next-best thing was to read the W3 spec for input[type="number"] (link) and implement our own validation logic to replicate that. Fortunately, this wasn't too difficult. Native input[type="number"]s simply accept any ordering of floating-point number characters as described elsewhere in the W3 spec (link).

There were two input cases to consider:

Typing (disallow keystrokes of anything except 0-9, e, E, +, -, or ..
Pasting (follow through with any paste, but omit characters not present in the list above).

There are a plethora of unit-tests to verify correctness of this behavior, so I'm confident it works.

Remaining issues

The only behavioral difference I'm aware of between this implementation and the native input[type="number"] is that after pasting (possibly in the middle of existing text), this implementation always moves your cursor to the end of the text. Would be nice to fix, but it seems uncommon and therefore low-priority.
PR Prepare Release v1.9.0 #671 hasn't merged yet; when it does, there will be merge conflicts in the example file. Let's make sure to merge that before this.

Reviewers should focus on:

This logic feels duplicative, and parts of it seem a little inelegant (particularly the didPasteEventJustHappen flag). If you have other (or just better) implementation ideas, let me know.
~~The prop name is long but precise. Happy to consider allowNumericCharactersOnly or something shorter though.~~ So changed

- Mimics input[type="number"]'s limited char set now

- When true and when false - Add e.key null check in numericInput.tsx (for unit tests)

- Also fire this.props.onPaste via safeInvoke - Add unit test validating on-paste sanitization

- Mimics input[type="number"]'s limited char set now

- When true and when false - Add e.key null check in numericInput.tsx (for unit tests)

- Also fire this.props.onPaste via safeInvoke - Add unit test validating on-paste sanitization

blueprint-bot · 2017-02-13T09:02:24Z

Implement sanitization after "paste" event - Also fire this.props.onPaste via safeInvoke - Add unit test validating on-paste sanitization

Preview: docs | table
Coverage: core | datetime

giladgray · 2017-02-13T16:22:55Z

How about simply allowNumericCharactersOnly? all numbers in JS are floating point.

adidahiya · 2017-02-13T19:48:24Z

+1 for reducing the character length of this prop name 😅

cmslewis · 2017-02-13T20:31:17Z

@giladgray that's good with me (and that's what @llorca suggested originally). Just want to make sure it's clear that the input will support more than just 0-9 (and maybe . and -).

…antir/blueprint into cl/numeric-input-numeric-digits-only

cmslewis · 2017-02-13T20:59:13Z

Changed the prop name to allowNumericCharactersOnly. Also, the commits got in some weird state. The net changes are the same though.

blueprint-bot · 2017-02-13T21:01:33Z

Rename prop to allowNumericCharactersOnly

Preview: docs | table
Coverage: core | datetime

leebyp · 2017-02-14T04:47:39Z

packages/core/examples/numericInputBasicExample.tsx

@@ -141,6 +141,7 @@ export class NumericInputBasicExample extends BaseExample<INumericInputBasicExam
        return (
            <div>
                <NumericInput
+                    allowFloatingPointNumberCharactersOnly={false}


missed prop name change

leebyp · 2017-02-14T04:50:21Z

packages/core/test/controls/numericInputTests.tsx

+        it("sets allowNumericCharactersOnly to true by default", () => {
+            const component = mount(<NumericInput />);
+            const value = component.props().allowNumericCharactersOnly;
+            expect(value).to.be.true;


you can probably remove this test =p

The test isn't here to test that React's defaultProps functionality works as expected; it's here to enforce the contract the component claims to uphold. During a refactor, if I accidentally delete or change a default value, then that would affect consumers, and I'd want to know about it from a failing test.

can the same not be said for all defaultProps?

In my perfect world, yes. :) But even my coverage isn't complete here; you're right. Still, I'd prefer to strive toward a world of testing (and therefore ensuring) defaults, even though setting a default is a trivial operation.

i agree with @leebyp here--this is not an important feature to test. it's a trivial test that can't catch bugs in our code because there's no logic being exercised here. it also doesn't increase code coverage.

I've come to appreciate unit tests as a way to both verify and communicate every reasonable assumption a consumer can make about a component. Catching bugs and increasing code coverage both play into that, and those are of course the most tangible benefits.

Expanding on the additional piece of my philosophy a bit, if I approach work on an unfamiliar component (e.g.) and see unit tests for Big Behavior X and Big Behavior Y but not Little Behavior Z, then (1) I can't tell if Little Behavior Z is an important part of the contract, (2) I can't always determine how to get the component in that state to verify how Little Behavior Z is supposed to work, and (3) I don't have a safeguard in place to tell me if I break Little Behavior Z.

For me, testing defaults fits into (3) and also fits into (1) if the default is impactful—but definitely does not fit into (2) (2 is rarer, but I still encounter it occasionally with subtle error-checking code).

Anyway, it's the communication piece that leads me to opt for testing defaults. Happy to go with the majority in this case though; will remove.

In the case of <NumbericInput />, to me, it seems more logical to have a test where you test the functionality of the component when you don't pass anything in - that's actually the output you care about, regardless of what the internals do.

In the examples, I think the problem is actually that behaviour z should be tested in the first place. No one checks the tests to see what the component contracts are (or at least I don't). If I break something and the tests don't catch it - we need more tests.

test the functionality of the component when you don't pass anything in

This sounds like a good approach to me.

If I break something and the tests don't catch it - we need more tests.

Agree.

leebyp · 2017-02-14T04:53:31Z

packages/core/test/controls/numericInputTests.tsx

@@ -212,6 +218,410 @@ describe("<NumericInput>", () => {
        });
    });

+    describe("Keyboard text entry in input field", () => {
+


ALL THE TESTS!!!!

i think you can pull a bunch of the repeating constants to the top, especially the keys

It's a little tricky, since certain keys hop from "invalid" lists into "valid" lists depending on the prop flag. I'll see what I can do though.

Done 👍 This feels way better; thanks!

blueprint-bot · 2017-02-14T20:26:37Z

Pull common constants to the top-most scope of the new test block

Preview: docs | table
Coverage: core | datetime

cmslewis · 2017-02-15T21:45:17Z

@giladgray @leebyp - ready for re-review!

blueprint-bot · 2017-02-15T21:45:30Z

Delete default allowNumericCharactersOnly value test

Preview: docs | table
Coverage: core | datetime

llorca · 2017-02-17T00:38:42Z

LGTM 👍 would be sweet if we could add that prop to the modifier switches in the example, so we can play with it

cmslewis · 2017-02-17T02:02:54Z

@llorca done!

blueprint-bot · 2017-02-17T02:13:27Z

Fix lint errors

Preview: docs | table
Coverage: core | datetime

llorca · 2017-02-17T02:49:20Z

Awesome! Hey I just realized this feature is broken on Safari... any idea why?

leebyp · 2017-02-17T16:08:35Z

packages/core/src/components/forms/numericInput.tsx

+        let nextValue: string;
+
+        if (this.props.allowNumericCharactersOnly && this.didPasteEventJustOccur) {
+            this.didPasteEventJustOccur = false;


what happens if we run through this block everytime regardless of whether paste event happened or not - is this a perf thing or are there other effects?

It's a little extra work, yes—with my interviewer hat on, O(n) for each keystroke—but that won't be an issue in real-world scenarios where numbers are only going to be a few characters long. I'd still prefer not to do that extra work, if only because it feels unclean and less clear that it's helpful specifically for the paste scenario. Maybe a comment would suffice though.

Yeah it's pretty obvious what's going on, i was only curious because you mentioned it's one of the inelegant place. A comment sounds good.

Comment + leave as is, or comment + removing this.didPasteEventJustOccur?

comment + leave as is

leebyp · 2017-02-17T16:09:22Z

rest looks good except the safari issue - curious about the onPaste logic too

cmslewis · 2017-02-17T21:41:00Z

@llorca @leebyp RE: Safari issue: looks like Safari doesn't support e.key. Also, did you know that a keyCode emitted in onKeyDown doesn't necessarily match a keyCode emitted in onKeyPress, making String.fromCharCode(e.keyCode) wholly unreliable? What a mess. I think the solution is to move the character-sanitization logic into onKeyPress though; that appears to have a more predictable output from String.fromCharCode. Lemme play and get back to you.

adidahiya · 2017-02-17T21:42:59Z

If the Safari fix is nontrivial, would prefer addressing that in a follow-up; it's low priority

blueprint-bot · 2017-02-17T22:34:41Z

Move key sanitization from onKeyDown to onKeyPress, update unit tests

Preview: docs | table
Coverage: core | datetime

cmslewis · 2017-02-17T22:37:58Z

@llorca @leebyp @adidahiya - Moving the logic to onKeyPress worked fine. Looks like it works in Chrome/Firefox/Safari now! But running unit tests in Firefox did surface a couple unrelated test failures in that browser. Tracking in #703.

EDIT: Also verified that it works on IE 11.

cmslewis added 8 commits February 12, 2017 22:27

Add allowFloatingPointNumberCharactersOnly prop

4afc324

- Mimics input[type="number"]'s limited char set now

Add unit tests for allowFloatingPointNumberCharactersOnly

b405725

- When true and when false - Add e.key null check in numericInput.tsx (for unit tests)

Tweak prop description

af8ff14

Implement sanitization after "paste" event

303ee71

- Also fire this.props.onPaste via safeInvoke - Add unit test validating on-paste sanitization

Add allowFloatingPointNumberCharactersOnly prop

e63ffae

- Mimics input[type="number"]'s limited char set now

Add unit tests for allowFloatingPointNumberCharactersOnly

e6d03d4

- When true and when false - Add e.key null check in numericInput.tsx (for unit tests)

Tweak prop description

bb795cb

Implement sanitization after "paste" event

98145c6

- Also fire this.props.onPaste via safeInvoke - Add unit test validating on-paste sanitization

cmslewis added 2 commits February 13, 2017 12:56

Merge branch 'cl/numeric-input-numeric-digits-only' of github.com:pal…

21639f1

…antir/blueprint into cl/numeric-input-numeric-digits-only

Rename prop to allowNumericCharactersOnly

f6b05e5

leebyp reviewed Feb 14, 2017

View reviewed changes

cmslewis changed the title ~~[Numeric Input] Add allowFloatingPointNumericCharactersOnly prop~~ [Numeric Input] Add allowNumericCharactersOnly prop Feb 14, 2017

cmslewis added 3 commits February 14, 2017 11:44

Fix allowNumericCharactersOnly props in each example

574cd96

Delete repeated block of unit tests from funky commit situation earlier

a37ecd8

Pull common constants to the top-most scope of the new test block

de274ad

Delete default allowNumericCharactersOnly value test

066dddf

cmslewis added 2 commits February 16, 2017 17:50

Add 'Numeric characters only' toggle to Basic Example

fd6bc30

Add unit-test to check incrementing from scientific-notation input

222c878

Fix lint errors

7a0bb2f

leebyp reviewed Feb 17, 2017

View reviewed changes

Move key sanitization from onKeyDown to onKeyPress, update unit tests

c6b4a48

leebyp approved these changes Feb 17, 2017

View reviewed changes

cmslewis merged commit c4eea14 into master Feb 17, 2017

cmslewis deleted the cl/numeric-input-numeric-digits-only branch February 17, 2017 23:16

giladgray mentioned this pull request Feb 24, 2017

Prepare Release v1.10.0 #730

Merged

[Numeric Input] Add allowNumericCharactersOnly prop #676

[Numeric Input] Add allowNumericCharactersOnly prop #676

Conversation

cmslewis commented Feb 13, 2017 • edited

Fixes aspects of #618

Checklist

Changes proposed in this pull request:

Reviewers should focus on:

blueprint-bot commented Feb 13, 2017

Implement sanitization after "paste" event - Also fire this.props.onPaste via safeInvoke - Add unit test validating on-paste sanitization

giladgray commented Feb 13, 2017

adidahiya commented Feb 13, 2017 • edited

cmslewis commented Feb 13, 2017

cmslewis commented Feb 13, 2017

blueprint-bot commented Feb 13, 2017

Rename prop to allowNumericCharactersOnly

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmslewis Feb 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmslewis Feb 14, 2017 • edited

Choose a reason for hiding this comment

giladgray Feb 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmslewis Feb 14, 2017 • edited

Choose a reason for hiding this comment

blueprint-bot commented Feb 14, 2017

Pull common constants to the top-most scope of the new test block

cmslewis commented Feb 15, 2017

blueprint-bot commented Feb 15, 2017

Delete default allowNumericCharactersOnly value test

llorca commented Feb 17, 2017

cmslewis commented Feb 17, 2017

blueprint-bot commented Feb 17, 2017

Fix lint errors

llorca commented Feb 17, 2017

Choose a reason for hiding this comment

cmslewis Feb 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leebyp commented Feb 17, 2017 • edited

cmslewis commented Feb 17, 2017

adidahiya commented Feb 17, 2017

blueprint-bot commented Feb 17, 2017

Move key sanitization from onKeyDown to onKeyPress, update unit tests

cmslewis commented Feb 17, 2017 • edited

cmslewis commented Feb 13, 2017 •

edited

adidahiya commented Feb 13, 2017 •

edited

cmslewis Feb 14, 2017 •

edited

cmslewis Feb 14, 2017 •

edited

giladgray Feb 15, 2017 •

edited

cmslewis Feb 14, 2017 •

edited

cmslewis Feb 17, 2017 •

edited

leebyp commented Feb 17, 2017 •

edited

cmslewis commented Feb 17, 2017 •

edited