Remove old IE polyfill code #10238

jquense · 2017-07-20T17:11:51Z

This is a bit of an experiment in what can be pulled out here. I’m not yet convinced it’s a good idea. I need to fixup my branch at least.

Removes onClick logic for radio and checkboxes, believe that was ie8 only
removes a substantial chunk of of the input polypill. ie9 partially supports onInput already, and we only need to add in the selection change event to handle the cases it doesn’t work on, e.g. deleting text

For everything else we don’t distinguish between when to use change vs input (such as for select inputs) since we listen to both and dedupe.

I tested against the fixtures on IE9 with positive results!

jquense · 2017-07-20T17:15:12Z

src/renderers/dom/shared/eventPlugins/ChangeEventPlugin.js

-    topLevelType === 'topKeyDown'
+    topLevelType === 'topInput' ||
+    topLevelType === 'topChange' ||
+    topLevelType === 'topSelectionChange'


it might be worth keeping the top level keyUp and keyDown since it's easy

What would we lose?

the original comment:

99% of the time, keydown and keyup aren't necessary. IE8 fails to fire
propertychange on the first input event after setting value from a
script and fires only keydown, keypress, keyup. Catching keyup usually
gets it and catching keydown lets us fire an event for the first
keystroke if user does a key repeat (it'll be a little delayed: right
before the second keystroke). Other input methods (e.g., paste) seem to
fire selectionchange normally.

It says IE8 only but one never knows right? We could add them back for sense of security, downside is just a few more lines of code, and maybe cargo-culting in code.

I think having them here plus a todo for further investigation sounds good to me.

aweary · 2017-07-20T17:53:11Z

src/renderers/dom/shared/eventPlugins/ChangeEventPlugin.js

-
-// For IE8 and IE9.
+// In IE9 the input event does not fire for deleting text. Luckily
+// selectionchange does, so we also listen for that in those cases.


Is this true? I tested this build out in IE9 on Windows 7 and it wasn't dispatching change events for deletion. But I also tested out master and I saw the same problem. Are you seeing something different?

oops it is true, but we aren't hitting this path, because the targetInst is null...I wonder if that is related to selectionchange not having the right target? why would the instance be null in this case?

and this bug is definitely in master as well

ok yeah this bug is probably in all versions of react. The Event responder can't find the associated instance for this event because the target is the document. This would be true for any weird events like this...

aweary · 2017-07-20T18:01:32Z

I think it'd be great if we could land this. I published the DOM fixtures with this build here:

http://remove-ie9-change-polyfills.surge.sh/

jquense · 2017-07-20T18:53:09Z

Updated with fixed logic

nhunzaker · 2017-07-20T18:54:18Z

Very cool. @jquense I didn't want to let this grow cold. I should be able to get a solid chunk of time to dig into this later today.

jquense · 2017-07-20T18:54:43Z

src/renderers/dom/shared/eventPlugins/ChangeEventPlugin.js

+
+    // On the selectionchange event, the target is the document which isn't
+    // helpful becasue we need the input, so we use the activeElement instead.
+    if (!isTextInputEventSupported && topLevelType === 'topSelectionChange') {


so this was the problem (and is the problem on master/stable) there is no targetInst for this event so there is no way this event will get published to react-land

Does getActiveElement not return the correct node for getting the instance from getInstanceFromNode?

it does generally, The null check is out of caution, since this depends on on the input being focused, there may be edge cases where focus drops or some programatic driving of the browser doesn't set focus, etc. I don't think there is much we can do about such edge cases and wanted to limit the possible effect to only cases where the polyfill was being used.

This is a frustrating constraint, but it sounds we can't do anything about it. 👍.

We were already tracking the active element for this code so the constraint isn't new, i'm just deferring to activeElement instead of caching it locally in a variable.

nhunzaker · 2017-07-20T20:15:05Z

src/renderers/dom/shared/eventPlugins/__tests__/ChangeEventPlugin-test.js

@@ -54,7 +54,7 @@ describe('ChangeEventPlugin', () => {
    );

    setUntrackedValue(input, true);
-    ReactTestUtils.SimulateNative.click(input);
+    ReactTestUtils.SimulateNative.change(input);


Sorry if this was asked before: why the switch from click to change?

Radios and Checkboxes were listening for onClick, i removed that in favor of just listening for onChange/onInput like text inputs.

I'm not entirely sure why the original code used click, the comment suggested it was for IE8, it appears to work fine in IE9 though

Cool. I like this better too, but I wasn't sure on the legacy. 👍

jquense · 2017-07-20T20:19:46Z

It should be noted that this narrows the scope of the onInput polyfill to only IE9, originally it was falling back to the polyfill for any env that didn't have onInput, but at this point I think that check is exactly the same as "just IE9" given the support in other browsers.

nhunzaker · 2017-07-20T20:22:34Z

fixtures/dom/public/index.html

+                  clearTimeout(id);
+              };
+      }());
+    </script>


Where does requestAnimationFrame come in?

React apparently requires it, I got a warning for it being missing. Not sure what the official place to snag it is

Fiber currently requires it, I think this way is fine.

I think this is fine.

aweary · 2017-07-20T20:29:24Z

fixtures/dom/public/index.html

+              };
+      }());
+    </script>
+    <script src="https://unpkg.com/babel-polyfill@6.23.0/dist/polyfill.min.js"></script>


Do you think using babel-polyfill will be problematic at all? It might make it easy to miss changes that won't work in all our supported browsers by default.

that's a good question...we should probably use a very focused set of polyfills according to what React says it requires. Should also help keep React honest about that as well :P

I just didn't know what React requires now, for instance in DEV you need Set which didn't seem obvious

Yeah, I like that with requestAnimationFrame we have an invariant so it fails early. I feel like we should do the same with other stuff like Set or Map if we have a hard requirement on those without fallbacks.

Yes, please let’s keep this focused on Map and Set.

aweary · 2017-07-20T20:32:19Z

src/renderers/dom/shared/eventPlugins/ChangeEventPlugin.js

-    topLevelType === 'topSelectionChange' ||
-    topLevelType === 'topKeyUp' ||
-    topLevelType === 'topKeyDown'
+    topLevelType === 'topInput' ||


If getTargetInstForInputEventPolyfill is only used in a browser (IE9) that doesn't support the input event, does it make sense to check for topInput?

Its like a half-polyfill, we actually do rely on IE9's native onInput but patch around it so it's not unusably buggy. Specifically it works fine except when deleting text (lol), the selectionchange covers that case.

OK cool, got it. For some reason, I was thinking IE9 didn't fire input events 😄

nhunzaker · 2017-07-20T20:39:42Z

@jquense I did a local build and booted this up. I can not change the fixture select or react version dropdown. (neither can I on http://remove-ie9-change-polyfills.surge.sh/):

This is in Chrome 59. Any idea what's going on?

aweary · 2017-07-20T20:42:11Z

I'm seeing that same problem as well.

On another note, it looks like the Angular team ran into the backspace issue and used the change and keydown events to side-step it. Could we do the same?

Reference: angular/angular.js#936

jquense · 2017-07-20T21:14:32Z

O wow selects in Chrome do some very weird things when you listen for both Input and Change, the change event has an empty value. (wat?). Lets go back to just using change for that, the intent wasn't really to change that behavior anyway.

jquense · 2017-07-20T21:18:22Z

To recap:

select and file inputs: listen to change
ie9 text inputs: listen to change input, selectionchange, keydown, and keyup
everything else: change and input

multiple events are deduped

nhunzaker · 2017-07-20T21:29:55Z

Cool. I'll pick up heavy duty browser QA and report back.

aweary · 2017-07-20T22:45:09Z

src/renderers/dom/shared/eventPlugins/ChangeEventPlugin.js

    }

    if (getTargetInstFunc) {
-      var inst = getTargetInstFunc(topLevelType, targetInst);
+      var inst = getTargetInstFunc(topLevelType, targetInst, targetNode);
      if (inst) {
        var event = createAndAccumulateChangeEvent(


@jquense can we use targetNode as nativeEventTarget for selection events in IE9? When you start backspacing it registers a change event but it gets undefined for event.target.value since event.target is still the document.

If we use this same check to branch when calling createAndAccumulateChangeEvent it should fix input backspacing in IE9 (tested locally).

var event = createAndAccumulateChangeEvent( inst, nativeEvent, (!isTextInputEventSupported && topLevelType === 'topSelectionChange') ? targetNode : nativeEventTarget, );

We can dedupe that check with a constant, but yeah. I think that should be work?

I think so! I forgot about actual event target, good call

nhunzaker · 2017-07-21T11:20:15Z

Found an issue in IE9. EDIT: Sorry, looks like @aweary beat me to it.

Open the text input fixture (http://localhost:3000/text-inputs
Go go "Cursor when editing email inputs"
Enter "user@example.com"
Backspace
I receive the following error:

Warning: A component is changing a controlled input of type email to be uncontrolled...https://fb.me/react-controlled-components
    in input (at InputTestCase.js:46)
    in fieldset (at InputTestCase.js:44)
    in div (at InputTestCase.js:43)
    in div (at Fixture.js:13)
    in Fixture (at InputTestCase.js:40)
    in InputTestCase (at index.js:56)

This is because the value is getting lost, like e.target.value is undefined and that throws our controlled input validations.

This is not an issue in IE10, IE11, or Edge.

jquense · 2017-07-24T15:14:40Z

the eventTarget issue should be fixed btw

nhunzaker · 2017-07-24T23:01:14Z

Tested successfully in:

Chrome 59, 51
Opera 46
Safari 10.1*
Firefox 48
IE 11
Edge 15
Yandex 14.12
iOS 8
Chrome Android 5.1

*Safari does not accept key input on range inputs, unless I've missed something

gaearon

Seems like this is good to go, except for the fixtures polyfill part? I trust you folks to test this well :-)

jquense · 2017-08-02T16:40:58Z

ok updated the polyfill situation. I ended up getting a CRA upgrade in there and moving the loader stuff into webpack-land so I didn't have to get a browser build of the core-js polyfills i wanted

gaearon · 2017-08-02T16:46:28Z

It would be nice to separate fixture changes in another PR (we can take it sooner). So that if we end up reverting this, we don't risk merge conflicts in fixtures.

jquense · 2017-08-02T16:51:16Z

The up[grade was sort of triggered by how I needed to tweak the entry to allow the more focused polyfilling. I could split it out and then rebase just the onChange on top of it if that's easier. Or structure the commits a bit better so you can revert the one cleanly if needed

Upgrade to react-scripts v1 and include required polyfills for older browsers

gaearon · 2017-08-02T16:57:06Z

I'm just proposing to split any changes in src from other changes. Other changes could get in earlier separately (even if they cause fixture to fail).

jquense · 2017-08-02T17:00:58Z

I cleaned up the commits. I can split them into two PR's but it seems like we are pretty ready to go with this, so it may not save much. That is unless @nhunzaker or @aweary won't have a chance to test again for a while.

nhunzaker · 2017-08-02T17:10:05Z

@jquense I can do some thorough QA later today, if it can wait 3 hours.

jquense · 2017-08-02T17:13:50Z

of course, no rush :)

outdated

aweary · 2017-08-02T19:05:27Z

I'm a little busy this week, but I can possibly get some QA in tomorrow if there's still a need for it ❤️

nhunzaker · 2017-08-02T23:51:06Z

I did it! This works in the following browsers:

Chrome OSX Sierra 59
Safari OSX Sierra 10.1
Safari OSX Lion 6
IE 9 Windows 7
IE10 Windows 7
IE11 Windows 10
Edge 15 Windows 10
Firefox 46 Windows 10
Opera 38 Windows 10
Nexus 6 Android 5
Safari iPhone 4s IOS 6
Safari iPhone 6s IOS 10
Yandex 17.7 OSX Sierra

gaearon · 2017-08-03T12:45:54Z

Thanks!

jquense · 2017-08-03T14:01:14Z

pour one out for the super clever ie8 onInput polyfill 🍾

gaearon · 2017-08-03T15:00:46Z

Not too fast, maybe we’ll have to revert 😛 These things are tricky

jquense · 2017-08-03T15:41:34Z

indeed, i've learned that the hard way.

**what is the change?:** This reverts facebook@0b220d0 Which was part of facebook#10238 **why make this change?:** When trying to sync the latest React with FB's codebase, we had failing integration tests. It looks like they are running with an old version of Chrome and there is something related to file upload that fails when these polyfills are missing. For now we'd like to revert this to unblock syncing, but it's worth revisiting this change to try and add some polyfill for FB and remove it from React, or to fix whatever the specific issue was with our file upload error. **test plan:** `yarn test` and also built and played with the `dom` and `packaging` fixtures

**what is the change?:** This reverts 0b220d0 Which was part of #10238 **why make this change?:** When trying to sync the latest React with FB's codebase, we had failing integration tests. It looks like they are running with an old version of Chrome and there is something related to file upload that fails when these polyfills are missing. For now we'd like to revert this to unblock syncing, but it's worth revisiting this change to try and add some polyfill for FB and remove it from React, or to fix whatever the specific issue was with our file upload error. **test plan:** `yarn test` and also built and played with the `dom` and `packaging` fixtures

facebook-github-bot added the CLA Signed label Jul 20, 2017

jquense requested review from nhunzaker and sophiebits and removed request for sophiebits July 20, 2017 17:12

jquense commented Jul 20, 2017

View reviewed changes

aweary reviewed Jul 20, 2017

View reviewed changes

jquense force-pushed the trim-ie8-change-code branch from 26352fe to c921618 Compare July 20, 2017 18:52

jquense commented Jul 20, 2017

View reviewed changes

nhunzaker reviewed Jul 20, 2017

View reviewed changes

aweary reviewed Jul 20, 2017

View reviewed changes

gaearon previously requested changes Jul 31, 2017

View reviewed changes

jquense mentioned this pull request Aug 2, 2017

Docs: Add note about checkboxes/radios and preventDefault #10354

Closed

jquense force-pushed the trim-ie8-change-code branch from d0b05a0 to dd936f3 Compare August 2, 2017 16:39

jquense added 2 commits August 2, 2017 12:56

Upgrade DOM Fixtures

a00d3a0

Upgrade to react-scripts v1 and include required polyfills for older browsers

Remove ChangeEvent polyfills for unsupported browsers

7659334

jquense force-pushed the trim-ie8-change-code branch from dd936f3 to 7659334 Compare August 2, 2017 16:57

Merge branch 'master' into trim-ie8-change-code

de74960

gaearon merged commit 0b220d0 into facebook:master Aug 3, 2017

jquense deleted the trim-ie8-change-code branch August 3, 2017 14:01

jquense mentioned this pull request Aug 11, 2017

Use the virtual target for change events to avoid restoring controlled state on the real target #10444

Merged

flarnie mentioned this pull request Aug 17, 2017

Revert "Remove old IE polyfill code" #10483

Merged

jquense mentioned this pull request Sep 26, 2017

Use Change/Input events for checkable inputs #10830

Closed

Remove old IE polyfill code #10238

Remove old IE polyfill code #10238

Conversation

jquense commented Jul 20, 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

Choose a reason for hiding this comment

aweary commented Jul 20, 2017

jquense commented Jul 20, 2017

nhunzaker commented Jul 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhunzaker Jul 20, 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

jquense commented Jul 20, 2017

nhunzaker Jul 20, 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

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

nhunzaker commented Jul 20, 2017

aweary commented Jul 20, 2017

jquense commented Jul 20, 2017

jquense commented Jul 20, 2017

nhunzaker commented Jul 20, 2017

Choose a reason for hiding this comment

aweary Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhunzaker commented Jul 21, 2017 • edited Loading

jquense commented Jul 24, 2017

nhunzaker commented Jul 24, 2017

gaearon left a comment

Choose a reason for hiding this comment

jquense commented Aug 2, 2017

gaearon commented Aug 2, 2017

jquense commented Aug 2, 2017

gaearon commented Aug 2, 2017 • edited Loading

jquense commented Aug 2, 2017

nhunzaker commented Aug 2, 2017

jquense commented Aug 2, 2017

aweary commented Aug 2, 2017

nhunzaker commented Aug 2, 2017

gaearon commented Aug 3, 2017

jquense commented Aug 3, 2017

gaearon commented Aug 3, 2017

jquense commented Aug 3, 2017

nhunzaker Jul 20, 2017 •

edited

Loading

nhunzaker Jul 20, 2017 •

edited

Loading

aweary Jul 20, 2017 •

edited

Loading

nhunzaker commented Jul 21, 2017 •

edited

Loading

gaearon commented Aug 2, 2017 •

edited

Loading