Use only public API for ChangeEventPlugin-test.js #11333

Ethan-Arrowood · 2017-10-23T04:38:47Z

Do not merge this! I need help understanding the ChangeEventPlugin._isInputEventSupported method on line 172.

jquense · 2017-10-24T00:32:53Z

It exists to toggle the plugin into the polyfill mode in order to test certain paths, since the jsdom environment always supports the input event. The "public api" way to address this would be to defeat the feature test in the plugin by stunning out document.mode or something

jquense · 2017-10-24T00:34:22Z

packages/react-dom/src/events/__tests__/ChangeEventPlugin-test.js

+var setUntrackedValue = Object.getOwnPropertyDescriptor(
+  HTMLInputElement.prototype,
+  'value',
+).set;


I don't know if this will work since we stub this in the input tracking code...or is that only the instance property that's stubbed?

This is the only prototype method I could find that would grant me access to this kind of info. If the value is untracked it won't show up in the Object.getOwnPropertyDescriptor() call. (Hence why I am using the .set method).

jquense · 2017-10-24T00:34:58Z

packages/react-dom/src/events/__tests__/ChangeEventPlugin-test.js

+    // Do not understand this method. Please advise
+    // if (!ChangeEventPlugin._isInputEventSupported) {
+    //   return;
+    // }


Essentially if the input event isn't supported this test won't test anything

how do you check if an input is valid?

I think this test as mutated a bit over time. This guard should be fine to just remove at this point. onInput will always be supported in the unit test environment so this wouldn't ever get hit.

Okay then I'll clean up my code and push. Should be good to go at this point.

gaearon · 2017-10-26T20:26:46Z

Is this ready for review, or is there more work planned?

Ethan-Arrowood · 2017-10-27T01:07:11Z

@gaearon I think its ready to be reviewed

gaearon · 2017-10-27T11:42:36Z

Can we also try to get rid of ReactTestUtils.SimulateNative calls? Like in #11309.

Ethan-Arrowood · 2017-10-30T15:21:47Z

@gaearon quick git question. I noticed that you merged master into this PR. How do I make sure my local branch is up to date now? Should I use git pull --rebase upstream master from my open-test-to-api local branch?

gaearon · 2017-10-31T14:21:45Z

I think you can just git pull on it. I pushed to your branch, so you can pull from your branch.

gaearon

Let's avoid SimulateNative too.

Ethan-Arrowood · 2017-10-31T18:42:46Z

Could use some help. I can't for the life of me get the onChange callback function to run:
Any tips? @gaearon

jquense · 2017-10-31T18:48:48Z

@Ethan-Arrowood checkboxes don't listen to change or input events oddly enough. They actually respond to 'click' events. check out the original code a bit closer, you'll see it (it's easy to miss)

Ethan-Arrowood · 2017-10-31T22:37:58Z

@jquense okay changing 'change' to 'click' works. The checked value is now changing as expected; however, no onChange or onClick is being executed. Any idea?

gaearon · 2017-11-03T20:45:04Z

ping @jquense :-)

jquense · 2017-11-06T13:20:21Z

Sorry, just pulled the branch and the tests all seem to pass for checkboxes. Is there some specific spot they aren't working or something i missed? Otherwise this seems gtg

Ethan-Arrowood · 2017-11-06T23:48:49Z

@jquense my orig edits work but @gaearon wants me to remove ReactTestUtils.SimulateNative

jquense · 2017-11-07T00:28:06Z

Ah yes I am hunk I know why. So all "artificial" events are going to be inadvertently silenced because to he dedupe logic. SimulateNative bypasses by tagging the event logic with a "simulated" flag. Check out the test utils code to see what I mean.

The east way to deal with it is to mimic SimulateNative by dispatching an event with the flag. Maybe let's see if that fixes the issue? Folks may want something a little less dependent on the implementation details but maybe let's confirm that's the issue

gaearon · 2017-11-08T22:12:46Z

The east way to deal with it is to mimic SimulateNative by dispatching an event with the flag.

This doesn't actually test the same code path as the browser goes through, does it? In that case I'd prefer we don't use the flag, and do something that resembles the browser code path.

gaearon · 2017-11-08T22:16:13Z

TODO: before merging, verify cases in #11337 are also covered.

gaearon

Let's try to avoid the simulated flag. Any ideas?

jquense · 2017-11-08T23:16:16Z

I'm taking a closer look...I'm not sure the simulated thing is actually the cause here.

jquense · 2017-11-08T23:22:27Z

so from what I can tell using:

function dispatchEventOnNode(node, type) {
  node.dispatchEvent(new Event(type, {bubbles: true, cancelable: true}));
}

to dispatch events just doesn't fire anything in the React event stack. It's not the ChangeEvent, ReactDOMEventListener.dispatch event isn't seeing anything either

Ethan-Arrowood · 2017-11-09T04:58:19Z

Sorry for not updating @jquense and @gaearon I've had a busy week full of internship interviews. I'll have some time to revisit this on Saturday.

Ethan-Arrowood · 2017-11-12T17:54:29Z

I've figured out how to get click callback to work. Seems to be a potential bug with the change event. I found a unanswered Stackoverflow question that actually links to one of your comments @jquense https://stackoverflow.com/questions/47169131/how-to-use-js-or-jquery-to-trigger-simulated-change-and-input-events-for-react-1

Ethan-Arrowood · 2017-11-12T18:08:59Z

Check out this CodeSandbox I made demonstrating this potential bug:

Open the console and watch as you enter characters into the input. A change event is dispatched as expected. But when we call dispatchChange no change is logged to the screen. Does this have to do with the fact that the input value is actually not changing?

Ethan-Arrowood · 2017-11-12T18:15:04Z

Experimental but could be useful in the future once completed: https://w3c.github.io/input-events/index.html#interface-InputEvent

Ethan-Arrowood · 2017-11-20T21:34:10Z

I can give it another shot, but I'm a little confused about the behavior of the onChange handler

gaearon · 2017-11-24T01:43:04Z

I wouldn't expect triggering change event to work.

If you open ChangeEventPlugin and read its source, it says: should use change? not for inputs (unless they're file inputs). The purpose of ChangeEventPlugin is to mostly turn other events into React onChange events. That's what @jquense also said in #11333 (comment).

Also, if I search for "checkbox" in that file, I find an explanation for why this is the case and how to trigger the event:

react/packages/react-dom/src/events/ChangeEventPlugin.js

Lines 200 to 203 in 46dd197

    
           function shouldUseClickEvent(elem) { 
        
             // Use the `click` event to detect changes to checkbox and radio inputs. 
        
             // This approach works across all browsers, whereas `change` does not fire 
        
             // until `blur` in IE8.

So trying to get dispatching change to work was the original problem. It wouldn't.

gaearon · 2017-11-24T02:05:09Z

Interestingly it seems that dispatching click in jsdom actually toggles the checked value.

gaearon · 2017-11-24T02:15:18Z

packages/react-dom/src/events/__tests__/ChangeEventPlugin-test.js


-    expect(called).toBe(1);
+    var node = ReactDOM.findDOMNode(stub);


Tip: there is no "stub" :-) Old tests are bad, don't copy them.
ReactDOM.render() already returns a DOM node.

gaearon · 2017-11-24T02:18:08Z

packages/react-dom/src/events/__tests__/ChangeEventPlugin-test.js

    );
+    var input = ReactDOM.findDOMNode(ref);
+    setUntrackedValue.call(input, 'bar');


Why are we setting untracked value here? I thought this test verifies that it catches setting value programmatically (and thus through the tracked value property like in the original test).

gaearon · 2017-11-24T02:40:23Z

packages/react-dom/src/events/__tests__/ChangeEventPlugin-test.js

@@ -128,9 +145,9 @@ describe('ChangeEventPlugin', () => {
    var input = ReactTestUtils.renderIntoDocument(


This won't work because it's not actually rendering into document (yes, the naming of that method is pretty bad). That's why reviews in other similar tests added setting up a container and adding it to the body before every test.

gaearon · 2017-11-24T02:55:59Z

Here's how I tried to write some of these tests based on your code:

describe('ChangeEventPlugin', () => {
  var container;

  beforeEach(() => {
    React = require('react');
    ReactDOM = require('react-dom');

    // The container has to be attached for events to fire.
    container = document.createElement('div');
    document.body.appendChild(container);
  });

  afterEach(() => {
    document.body.removeChild(container);
    container = null;
  });

  fit('should fire change for checkbox input', () => {
    var called = 0;

    function cb(e) {
      called++;
      expect(e.type).toBe('change');
    }

    var node = ReactDOM.render(
      <input type="checkbox" onChange={cb} />,
      container,
    );

    // We want `setUntrackedChecked.call(node, true)` but jsdom incorrectly
    // changes `checked` on `click` event dispatch so we set it the opposite value.
    setUntrackedChecked.call(node, false);
    node.dispatchEvent(new Event('click', {bubbles: true, cancelable: true}));
    expect(called).toBe(1);
    expect(node.checked).toBe(true);

    // We want `setUntrackedChecked.call(node, false)` but jsdom incorrectly
    // changes `checked` on `click` event dispatch so we set it the opposite value.
    setUntrackedChecked.call(node, true);
    node.dispatchEvent(new Event('click', {bubbles: true, cancelable: true}));
    expect(called).toBe(2);
    expect(node.checked).toBe(false);
  });

  fit('should not fire change setting the value programmatically', () => {
    var called = 0;

    function cb(e) {
      called++;
      expect(e.type).toBe('change');
    }

    var input = ReactDOM.render(
      <input type="text" defaultValue="foo" onChange={cb} />,
      container,
    );

    // Set to a value programmatically...
    input.value = 'bar';
    input.dispatchEvent(new Event('input', {bubbles: true, cancelable: true}));
    expect(called).toBe(0);

    // ...but then change it back as if the user did it.
    setUntrackedValue.call(input, 'foo');
    input.dispatchEvent(new Event('input', {bubbles: true, cancelable: true}));

    // Deduplication logic should understand it's not a 'foo' -> 'foo' noop, but
    // 'bar' -> 'foo' change, and therefore the onChange event should be fired.
    expect(called).toBe(1);
    expect(input.value).toBe('foo');

    // For good measure, check that firing an input without an actual change
    // is deduplicated.
    input.dispatchEvent(new Event('input', {bubbles: true, cancelable: true}));
    expect(called).toBe(1);
  });

  xit('should not fire change setting checked programmatically', () => {
    var called = 0;

    function cb(e) {
      called++;
      expect(e.type).toBe('change');
    }

    var input = ReactDOM.render(
      <input type="checkbox" onChange={cb} defaultChecked={true} />,
      container,
    );

    // Set to a value programmatically...
    input.checked = false;
    input.dispatchEvent(new Event('click'));
    expect(called).toBe(0);

    // ...but then change it back as if the user did it.
    setUntrackedValue.call(input, 'foo');
    input.dispatchEvent(new Event('input', {bubbles: true, cancelable: true}));

    // Deduplication logic should understand it's not a 'foo' -> 'foo' noop, but
    // 'bar' -> 'foo' change, and therefore the onChange event should be fired.
    expect(called).toBe(1);
    expect(input.value).toBe('foo');

    // For good measure, check that firing an input without an actual change
    // is deduplicated.
    input.dispatchEvent(new Event('input', {bubbles: true, cancelable: true}));
    expect(called).toBe(1);
  });

  fit('should not fire change when setting checked programmatically', () => {
    var called = 0;

    function cb(e) {
      called++;
      expect(e.type).toBe('change');
    }

    var input = ReactDOM.render(
      <input type="checkbox" onChange={cb} defaultChecked={true} />,
      container
    );

    // Set to a value programmatically...
    input.checked = true;
    // We want the real checked value to stay `true` but jsdom incorrectly
    // changes `checked` on `click` event dispatch so we set it to the opposite.
    setUntrackedChecked.call(input, false);
    input.dispatchEvent(new Event('click', {bubbles: true, cancelable: true}));
    expect(called).toBe(0);

    // ...but then change it back as if the user did it.
    // We want `setUntrackedChecked.call(input, false)` but jsdom incorrectly
    // changes `checked` on `click` event dispatch so we set it to the opposite.
    setUntrackedChecked.call(input, true);
    input.dispatchEvent(new Event('click', {bubbles: true, cancelable: true}));

    expect(called).toBe(1);
  });

Ethan-Arrowood · 2017-11-24T03:46:38Z

Thanks for the help @gaearon! Let me see what I can come up with now.

Ethan-Arrowood · 2017-11-24T06:55:58Z

@gaearon alright I've update everything. I tried my best to understand everything and I hope my tests are covering what they are supposed to. Let me know how I did and if there are any changes to be made.

gaearon · 2017-11-24T11:27:57Z

packages/react-dom/src/events/__tests__/ChangeEventPlugin-test.internal.js

+    container = null;
+  });
+
+  fit('should fire change for checkbox input', () => {


I wrote "fit" in my example to force only a few tests to execute. It should not be used in committed code.

Should it just be it?

gaearon · 2017-11-24T11:29:55Z

packages/react-dom/src/events/__tests__/ChangeEventPlugin-test.internal.js

-    ReactTestUtils.SimulateNative.click(input);
-
+    setUntrackedChecked.call(node, false);
+    node.dispatchEvent(new Event('click', {bubbles: true, cancelable: true}));


We should leave a comment about jsdom click misbehaving. It's not obvious and doesn't happen in the browser (it's also possible jsdom has fixed this in new versions so we need to make it clear in case somebody later tries to update it, and the test will fail).

Can you please elaborate on what a 'common' is?

node.dispatchEvent(new Event('click')); expect(called).toBe(0);

Is this what you mean?

Sorry, autocorrect. Meant "we should leave a comment". Like in the sample I wrote above.

gaearon · 2017-11-24T11:30:52Z

packages/react-dom/src/events/__tests__/ChangeEventPlugin-test.internal.js


    expect(called).toBe(1);
  });

-  it('should unmount', () => {
-    var container = document.createElement('div');
+  fit('should catch setting the value programmatically', () => {


I don't understand what this test is trying to test. What is the thinking behind it?

I think it should test that the tracked value updates when you set it from js. The test should normally set the value then trigger and event and assert that the event doesn't fire

Sorry, I meant that I don't understand what the new (rewritten) test is doing (since it's not testing setting the field at all).

Ethan-Arrowood · 2017-11-24T19:01:53Z

Forgot to prettify. Pushing now

gaearon · 2017-11-24T19:02:46Z

Thanks. I think I can take it from here as I want to make some more changes.

Ethan-Arrowood · 2017-11-24T19:04:01Z

Okay! Glad I could contribute 😃

gaearon · 2017-11-24T21:51:41Z

packages/react-dom/src/events/__tests__/ChangeEventPlugin-test.internal.js

-    [
-      <input type="text" onChange={cb} />,
-      <input type="number" onChange={cb} />,
-      <input type="range" onChange={cb} />,


Seems like it was important to do this for all types of inputs, and not just 'text'?

gaearon · 2017-11-24T22:06:09Z

I rewrote some of the more complex tests but really appreciate you starting this! That was a tough one 😄

Ethan-Arrowood · 2017-11-25T01:25:02Z

Thank you very much! Love the challenge :) Thanks for all your help.

gaearon · 2017-11-25T01:36:46Z

This was a great discussion. I feel like I actually understand this now. Here's a follow up #11654

* Use only public API for ChangeEventPlugin-test.js * precommit commands complete * Removed comments * Improving event dispatchers * Updated tests * Fixed for revisions * Prettified * Add more details and fixes to tests * Not internal anymore * Remove unused code

Ethan-Arrowood added 2 commits October 23, 2017 00:33

Use only public API for ChangeEventPlugin-test.js

29253ec

precommit commands complete

df01d2b

facebook-github-bot added the CLA Signed label Oct 23, 2017

jquense reviewed Oct 24, 2017

View reviewed changes

Removed comments

c7777f9

Merge branch 'master' into open-test-to-api

b6283f0

gaearon requested changes Oct 31, 2017

View reviewed changes

Merge branch 'master' into open-test-to-api

27ec33f

gaearon requested changes Nov 8, 2017

View reviewed changes

gaearon reviewed Nov 24, 2017

View reviewed changes

Ethan-Arrowood added 2 commits November 23, 2017 22:52

Merge remote-tracking branch 'upstream/master' into open-test-to-api

8be2a24

Updated tests

054c955

gaearon requested changes Nov 24, 2017

View reviewed changes

Fixed for revisions

6238df3

Prettified

852e663

gaearon reviewed Nov 24, 2017

View reviewed changes

gaearon added 2 commits November 24, 2017 22:00

Add more details and fixes to tests

7bb2124

Not internal anymore

0570084

gaearon approved these changes Nov 24, 2017

View reviewed changes

Remove unused code

d34a205

gaearon merged commit a67757e into facebook:master Nov 24, 2017

gaearon mentioned this pull request Nov 25, 2017

Replace inputValueTracking-test with public API tests #11654

Merged


		expect(called).toBe(1);
		var node = ReactDOM.findDOMNode(stub);

		@@ -128,9 +145,9 @@ describe('ChangeEventPlugin', () => {
		var input = ReactTestUtils.renderIntoDocument(

Use only public API for ChangeEventPlugin-test.js #11333

Use only public API for ChangeEventPlugin-test.js #11333

Conversation

Ethan-Arrowood commented Oct 23, 2017

jquense commented Oct 24, 2017

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

gaearon commented Oct 26, 2017

Ethan-Arrowood commented Oct 27, 2017

gaearon commented Oct 27, 2017

Ethan-Arrowood commented Oct 30, 2017

gaearon commented Oct 31, 2017

gaearon left a comment

Choose a reason for hiding this comment

Ethan-Arrowood commented Oct 31, 2017

jquense commented Oct 31, 2017

Ethan-Arrowood commented Oct 31, 2017

gaearon commented Nov 3, 2017

jquense commented Nov 6, 2017

Ethan-Arrowood commented Nov 6, 2017

jquense commented Nov 7, 2017

gaearon commented Nov 8, 2017

gaearon commented Nov 8, 2017

gaearon left a comment

Choose a reason for hiding this comment

jquense commented Nov 8, 2017

jquense commented Nov 8, 2017 • edited by gaearon Loading

Ethan-Arrowood commented Nov 9, 2017

Ethan-Arrowood commented Nov 12, 2017

Ethan-Arrowood commented Nov 12, 2017

Ethan-Arrowood commented Nov 12, 2017

Ethan-Arrowood commented Nov 20, 2017

gaearon commented Nov 24, 2017 • edited Loading

gaearon commented Nov 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Nov 24, 2017

Ethan-Arrowood commented Nov 24, 2017

Ethan-Arrowood commented Nov 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon Nov 24, 2017 • edited Loading

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

gaearon Nov 24, 2017 • edited Loading

Choose a reason for hiding this comment

Ethan-Arrowood commented Nov 24, 2017

gaearon commented Nov 24, 2017

Ethan-Arrowood commented Nov 24, 2017

Choose a reason for hiding this comment

gaearon commented Nov 24, 2017

Ethan-Arrowood commented Nov 25, 2017

gaearon commented Nov 25, 2017

jquense commented Nov 8, 2017 •

edited by gaearon

Loading

gaearon commented Nov 24, 2017 •

edited

Loading

gaearon commented Nov 24, 2017 •

edited

Loading

gaearon Nov 24, 2017 •

edited

Loading

gaearon Nov 24, 2017 •

edited

Loading