Pass modifier flags when triggering Buttons via keypress #561

cmslewis · 2017-01-27T08:28:58Z

Checklist

Include tests

Changes proposed in this pull request:

When we introduced AbstractButton, we unintentionally eliminated support for onKeyDown and onKeyUp callbacks on our buttons. This PR adds back that support, and it also fixes or adds tests to make sure all of this works as expected. In particular, this PR fixes an issue where a couple asserts wrapped in setTimeouts were causing button tests to pass without actually reaching those checks.

Reviewers should focus on:

@giladgray - I forked this off of your DRY Buttons components #544 PR and am currently setting that as the destination branch to show just the changes I made. We should change the destination to master before merging.
I had to cast to any in a few spots to get around TS compiler errors. LMK if any of these castings look unacceptable.

…lick prop

… use setTimeout

adidahiya · 2017-01-27T18:25:04Z

Do we really have to futz with an event's fields? Something feels off about this whole thing... AbstractButton seems to be more trouble than it's worth.

leebyp · 2017-01-27T19:15:19Z

because we're using buttonRef.click() and not proxying the onClick handler, we might be limited by enzyme:
enzymejs/enzyme#566
https://github.com/airbnb/enzyme/blob/master/docs/future.md

…keys

…appropriately

giladgray · 2017-01-30T23:48:50Z

packages/core/src/components/button/abstractButton.tsx

@@ -46,6 +46,7 @@ export abstract class AbstractButton<T> extends React.Component<React.HTMLProps<
        },
    };

+    // define these as private members to avoid polluting component state with sneaky fields from this scope


do we still need this comment?

Nope, deleted.

giladgray · 2017-01-30T23:49:55Z

packages/core/test/buttons/buttonTests.tsx

@@ -59,17 +59,31 @@ function buttonTestSuite(component: React.ComponentClass<any>, tagName: string)
            assert.equal(onClick.callCount, 0);
        });

-        it("calls onClick when enter key pressed", () => {
+        // clewis: After adding explicit done()s that wee missing before, these


were missing 😭

… SPACE

blueprint-bot · 2017-01-31T00:27:10Z

Add tests to verify onKeyDown and onKeyUp triggered with modifier flags

Preview: docs | table
Coverage: core | datetime

blueprint-bot · 2017-01-31T00:31:25Z

Delete unneeded comment

Preview: docs | table
Coverage: core | datetime

cmslewis · 2017-01-31T00:32:55Z

@leebyp FYI - fixed the unit-test issues with some any cleverness courtesy of @giladgray.

cmslewis · 2017-01-31T00:38:39Z

@adidahiya @giladgray @leebyp - Check out the new approach (I've updated the PR description above).

blueprint-bot · 2017-01-31T00:44:43Z

Merge branch 'master' into cl/refactor-buttons-modifier-keys

Preview: docs
Coverage: core | datetime

leebyp · 2017-01-31T00:57:02Z

packages/core/test/buttons/buttonTests.tsx

+            // wait for the whole lifecycle to run
+            setTimeout(() => {
+                assert.equal(onClick.callCount, 1);
+                done();


im pretty sure you do, but did you check if this is still needed with the change in the logic?

leebyp · 2017-01-31T00:57:40Z

packages/core/test/buttons/buttonTests.tsx

+            checkKeyEventCallbackInvoked("onKeyUp", "keyup", Keys.SPACE);
+        });
+
+        it("calls onClick when enter key pressed", (done) => {


"... when enter key released"

leebyp · 2017-01-31T01:01:23Z

packages/core/test/buttons/buttonTests.tsx

+            // IButtonProps doesn't include onKeyDown or onKeyUp in its
+            // definition, even though Buttons support those props. Casting as
+            // `any` gets around that for the purpose of these tests.
+            const wrapper = button({ [callbackPropName]: callback } as any);


you can edit the button helper function to use IButtonProps & ButtonHTMLProps (I cant remember the exact nomenclature)

Trying it - it's leading to a soupy syntactic mess that'll percolate throughout the file. Okay if I leave as is?

adidahiya · 2017-01-31T17:44:47Z

packages/core/src/components/button/abstractButton.tsx

@@ -75,22 +75,24 @@ export abstract class AbstractButton<T> extends React.Component<React.HTMLProps<
        };
    }

-    protected handleKeyDown = (e: React.KeyboardEvent<HTMLElement>) => {
+    protected handleKeyDown = (e: React.KeyboardEvent<any>) => {


why does this need to be any?

// CC @giladgray, who looked at this with me before committing

I kept seeing compiler errors to the effect of KeyboardEvent<T> is not a supertype of KeyboardEvent<HTMLElement> for the safeInvoke line below. If there's a better way to address that issue, let me know. 👍

adidahiya · 2017-01-31T17:45:14Z

packages/core/test/buttons/buttonTests.tsx

+            // doesn't properly propagate events on non-string refs
+            // (https://github.com/airbnb/enzyme/issues/566), we need to dig in
+            // and attach a spy to the ref's .click function ourselves.
+            const buttonRef = (wrapper.instance() as any).buttonRef;


as AbstractButton?

Looks like a no-go. This change:

const buttonRef = (wrapper.instance() as AbstractButton<HTMLButtonElement>).buttonRef;

yields the following error:

[ts] Property 'buttonRef' is protected and only accessible within class 'AbstractButton<T>' and its subclasses. (property) AbstractButton<HTMLButtonElement>.buttonRef: HTMLElement

blueprint-bot · 2017-01-31T20:50:45Z

Add clearer comments and fix wording in test case description

Preview: docs
Coverage: core | datetime

giladgray · 2017-01-31T22:54:15Z

packages/core/src/components/button/abstractButton.tsx

+    // we're casting as `any` to get around a somewhat opaque safeInvoke error
+    // that "Type argument candidate 'KeyboardEvent<T>' is not a valid type
+    // argument because it is not a supertype of candidate
+    // 'KeyboardEvent<HTMLElement>'."


shouldn't need to duplicate this comment

giladgray · 2017-01-31T22:56:58Z

packages/core/test/buttons/buttonTests.tsx

-            const onClick = sinon.spy();
-            button({ onClick }, true).simulate("keyup", { which: Keys.SPACE });
-            setTimeout(() => assert.equal(onClick.callCount, 1), 0);
+        it("pressing enter triggers onKeyUp props with any modifier flags", () => {


truthfully, these tests are not particularly useful. you're really just testing the safeInvoke line as it reuses the exact same event so of course the modifier flags come through. there's nothing special here, it's just vanilla React.

i'd be fine with removing these tests, or simplifying to one for ENTER and one for SPACE

If these tests had been in place, the NumericInput functionality wouldn't have broken without warning. I want some kind of safeguard.

Okay if we leave them? I guess I'm okay with deleting the tests for onKeyUp, but I definitely want a test in place for onKeyDown, since that's what broke before.

K, deleted tests for onKeyUp.

giladgray · 2017-01-31T22:58:00Z

packages/core/test/buttons/buttonTests.tsx

+            // that it is being called. casting as `any` eliminates the button's
+            // knowledge of which members are public, private, or protected.
+            // this allows us to access the private buttonRef for the purposes
+            // of this test.


this comment feels too verbose. should be enough to say "mock the DOM click() function because enzyme only handles simulated React events"

does not seem fixed in the diff

giladgray · 2017-01-31T22:58:25Z

packages/core/test/buttons/buttonTests.tsx

+            // knowledge of which members are public, private, or protected.
+            // this allows us to access the private buttonRef for the purposes
+            // of this test.
+            const buttonRef = (wrapper.instance() as any).buttonRef;


adi suggested casting to AbstractButton instead of any. does that work?

Not when I tried, no. Still complained that buttonRef was protected and therefore inaccessible.

oh of course

blueprint-bot · 2017-01-31T23:43:38Z

Delete comments / make them more concise

Preview: docs
Coverage: core | datetime

blueprint-bot · 2017-01-31T23:47:21Z

Delete tests for onKeyUp

Preview: docs
Coverage: core | datetime

giladgray · 2017-02-01T00:08:03Z

packages/core/test/buttons/buttonTests.tsx

+            // that it is being called. casting as `any` eliminates the button's
+            // knowledge of which members are public, private, or protected.
+            // this allows us to access the private buttonRef for the purposes
+            // of this test.


does not seem fixed in the diff

Gilad Gray and others added 11 commits January 25, 2017 11:32

protected renderChildren() reduces duplicated render logic

26772ae

replace key handle switches with simple if

85eb479

rename key handlers to handle*

091cffb

getProps() for shared button props

a7a6c39

copyright, fix awkward import

2db82e8

fix lint

b97f07e

rename to getCommonButtonProps, use disabled

89c3e5e

Fix AbstractButton to pass modifier keys from key-press events to onC…

353b7d2

…lick prop

[Reveals breaking tests] Invoke done() explicitly in buttonTests that…

8327046

… use setTimeout

Skip the two offending tests for now

216f553

Use safeInvoke for this.props.onClick

51bf10a

cmslewis changed the title ~~Pass modifier flags when triggering Button/AnchorButton via keypress~~ Pass modifier flags when triggering Buttons via keypress Jan 27, 2017

Fix lint error by explicitly re-specifying parameter type

c326af3

Gilad Gray and others added 3 commits January 30, 2017 11:59

one maybeRender to rule them all

0e53de0

Merge branch 'gg/refactor-buttons' into cl/refactor-buttons-modifier-…

848c9a9

…keys

Revert previous changes; just invoke this.props.{onKeyDown, onKeyUp} …

f2ef1ce

…appropriately

giladgray reviewed Jan 30, 2017

View reviewed changes

cmslewis added 4 commits January 30, 2017 16:05

Fix tests to ensure buttonRef.click triggered after pressing ENTER or…

808d7f2

… SPACE

Add tests to verify onKeyDown and onKeyUp triggered with modifier flags

7d4d88f

Fix tests

733d25a

Delete unneeded comment

b107e94

cmslewis changed the base branch from gg/refactor-buttons to master January 31, 2017 00:33

Merge branch 'master' into cl/refactor-buttons-modifier-keys

e38ce8d

leebyp suggested changes Jan 31, 2017

View reviewed changes

adidahiya reviewed Jan 31, 2017

View reviewed changes

Add clearer comments and fix wording in test case description

d4d120c

leebyp approved these changes Jan 31, 2017

View reviewed changes

giladgray suggested changes Jan 31, 2017

View reviewed changes

cmslewis added 2 commits January 31, 2017 15:39

Delete comments / make them more concise

e6d8727

Delete tests for onKeyUp

c4014e7

giladgray suggested changes Feb 1, 2017

View reviewed changes

giladgray approved these changes Feb 1, 2017

View reviewed changes

cmslewis merged commit 7415fc9 into master Feb 1, 2017

cmslewis deleted the cl/refactor-buttons-modifier-keys branch February 1, 2017 00:11

giladgray mentioned this pull request Feb 3, 2017

Prepare Release v1.8.0 #616

Merged

Pass modifier flags when triggering Buttons via keypress #561

Pass modifier flags when triggering Buttons via keypress #561

Conversation

cmslewis commented Jan 27, 2017 • edited

Checklist

Changes proposed in this pull request:

Reviewers should focus on:

adidahiya commented Jan 27, 2017

leebyp commented Jan 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueprint-bot commented Jan 31, 2017

Add tests to verify onKeyDown and onKeyUp triggered with modifier flags

blueprint-bot commented Jan 31, 2017

Delete unneeded comment

cmslewis commented Jan 31, 2017

cmslewis commented Jan 31, 2017

blueprint-bot commented Jan 31, 2017

Merge branch 'master' into cl/refactor-buttons-modifier-keys

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 Jan 31, 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

blueprint-bot commented Jan 31, 2017

Add clearer comments and fix wording in test case description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmslewis Jan 31, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueprint-bot commented Jan 31, 2017

Delete comments / make them more concise

blueprint-bot commented Jan 31, 2017

Delete tests for onKeyUp

Choose a reason for hiding this comment

cmslewis commented Jan 27, 2017 •

edited

cmslewis Jan 31, 2017 •

edited

cmslewis Jan 31, 2017 •

edited