chore(syntheticEvent): remove IE8 code #11178

Haroenv · 2017-10-10T17:25:27Z

Since IE8 has been deprecated for a while, I thought it might be useful to remove some IE8-only code

If this is not something you want to focus on yet, or is too much work to test, feel free to close this PR

Before submitting a pull request, please make sure the following is done:

Fork the repository and create your branch from master.
If you've added code that should be tested, add tests!
If you've changed APIs, update the documentation.
Ensure the test suite passes (npm test).
Format your code with prettier (npm run prettier).
Make sure your code lints (npm run lint).
Run the Flow typechecks (npm run flow).
If you haven't already, complete the CLA.

Since IE8 has been deprecated for a while, I thought it might be useful to remove some IE8-only code If this is not something you want to focus on yet, or is too much work to test, feel free to close this PR

blling · 2017-10-10T17:30:35Z

src/renderers/dom/shared/syntheticEvents/SyntheticUIEvent.js

-    } else {
-      return window;
-    }
+    return doc.defaultView;


Should return target.ownerDocument.parentWindow immediately.

gaearon · 2017-10-10T17:41:01Z

We’ll need someone to verify this is only the case in IE8, and not IE9+.

Haroenv · 2017-10-10T17:59:21Z

If I read MDN correctly, it's added in IE9, but should definitely be tested thoroughly

gaearon · 2017-10-10T18:03:26Z

MDN says ownerDocument has been around since IE6: https://developer.mozilla.org/en-US/docs/Web/API/Node/ownerDocument

MSDN also doesn’t mention any version support differences: https://msdn.microsoft.com/library/ms534315(v=vs.85).aspx

So there’s clearly something else happening here.
It would help to dig into the git history to see when this was added.

jquense · 2017-10-10T18:12:37Z

src/renderers/dom/shared/syntheticEvents/SyntheticUIEvent.js

-    } else {
-      return window;
-    }
+    return target.ownerDocument.defaultView;


I believe you could still have cases where ownerDocument is undefined, e.g. if the node is detached (for some reason)

This whole block might be able to go away, it looks like IE9 supports event.view:

I'll do some testing. MDN isn't comprehensive on support here:

Maybe we can fix that :)

Yep! UIEvent.view is supported in:

IE9-11
Safari Desktop 4, 5.1, 6.2, 7.1, 10.1, 11.0
Safari iOS 3, 4
FF 47
Windows Phone 8.1

Here's my test case:
https://codepen.io/nhunzaker/pen/BwVwqo

And I've submitted a revision to this page on MDN:
https://developer.mozilla.org/en-US/docs/Web/API/UIEvent/view

Haroenv · 2017-10-10T18:49:34Z

defaultView was added in IE9 preactjs/preact-compat#423, maybe I misread the comment, and there's no code to remove here 🤔

syranide · 2017-10-11T08:46:14Z

parentWindow is fallback for IE8 I believe and should be redundant. The rest is not IE8-specific though I'm sure.

nhunzaker

According to my research (https://github.com/facebook/react/pull/11178/files#r143985409), we can totally remove the event.view polyfill.

Could you remove the event.view entry?

nhunzaker · 2017-10-11T12:10:40Z

I did some testing of detail while we're in here. Here's what I found:

IE9-11
* BUG: IE11 always reports 0

IE Edge 14, 15

Safari Desktop 4, 5.1, 6.2, 7.1, 8, 9.1, 10.1, 11.0

Safari iOS 3, 4, 9, 10.3
* Doubleclick is hard on iOS

FF 47

Windows Phone 8.1
* BUG: Always reports 0

Chrome 27-61

Firefox 11-56

Opera 12.12-48

So basically everywhere except below IE9. IE 11/Windows Phone always reports 0 for detail, but we don't attempt to fix this in React.

We should remove the patch for event.detail

Haroenv · 2017-10-11T16:18:50Z

@nhunzaker, so I can remove all the code after if (event.view) return event.view?

nhunzaker · 2017-10-11T17:39:13Z

src/renderers/dom/shared/syntheticEvents/SyntheticUIEvent.js

-    } else {
-      return window;
-    }
+    return event.view;
  },
  detail: function(event) {
    return event.detail || 0;


I believe, unless I'm mistaken, this can be:

var UIEventInterface = { view: null, detail: null }

Maybe the var can be completely removed then? and not augmenting the class?

everything breaks otherwise

Haroenv · 2017-10-11T18:20:56Z

I did the augment with twice null @nhunzaker, I tried removing it completely, but that made the whole test suite cry, so I guess it depended on it being augmented.

nhunzaker · 2017-10-11T22:58:06Z

Yeah, I think it's essential. Thank you for your contribution!

@jquense, @aweary, or @syranide. I've confirmed basic support for these two properties everywhere, can you think of anything else we should check before I merge this?

blling · 2017-10-11T23:17:31Z

Why should not removing it completely ?

nhunzaker · 2017-10-11T23:27:50Z

@blling synthetic events maintain an interface so that they can copy over properties. It does that here:
https://github.com/facebook/react/blob/master/src/renderers/shared/shared/event/SyntheticEvent.js#L90-L107

The keys can't be completely removed because they tell SyntheticEvent to copy over those attributes from the original DOM event.

blling · 2017-10-11T23:36:51Z

Get it, thanks!

syranide · 2017-10-12T07:28:59Z

@nhunzaker 👍

nhunzaker · 2017-10-12T21:30:54Z

Thanks, @Haroenv!

Haroenv · 2017-10-12T21:38:25Z

Thanks for your guidance @nhunzaker

chore(syntheticEvent): remove IE8 code

af0eee2

Since IE8 has been deprecated for a while, I thought it might be useful to remove some IE8-only code If this is not something you want to focus on yet, or is too much work to test, feel free to close this PR

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

blling reviewed Oct 10, 2017

View reviewed changes

Update SyntheticUIEvent.js

fbdc408

jquense reviewed Oct 10, 2017

View reviewed changes

nhunzaker suggested changes Oct 11, 2017

View reviewed changes

Update SyntheticUIEvent.js

beb1ddb

nhunzaker reviewed Oct 11, 2017

View reviewed changes

Haroenv added 5 commits October 11, 2017 10:47

remove unused require

1977426

completely remove UIEvent

8689bb2

augment with noop

79642ec

everything breaks otherwise

comment back

a16b4d4

spacing

dd9760f

nhunzaker approved these changes Oct 11, 2017

View reviewed changes

nhunzaker merged commit 0c5a455 into facebook:master Oct 12, 2017

Haroenv deleted the patch-1 branch October 12, 2017 21:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(syntheticEvent): remove IE8 code #11178

chore(syntheticEvent): remove IE8 code #11178

Haroenv commented Oct 10, 2017

blling Oct 10, 2017

gaearon commented Oct 10, 2017

Haroenv commented Oct 10, 2017

gaearon commented Oct 10, 2017 •

edited

Loading

jquense Oct 10, 2017

nhunzaker Oct 11, 2017

nhunzaker Oct 11, 2017

Haroenv commented Oct 10, 2017

syranide commented Oct 11, 2017

nhunzaker left a comment

nhunzaker commented Oct 11, 2017 •

edited

Loading

Haroenv commented Oct 11, 2017

nhunzaker Oct 11, 2017

Haroenv Oct 11, 2017

Haroenv commented Oct 11, 2017

nhunzaker commented Oct 11, 2017

blling commented Oct 11, 2017

nhunzaker commented Oct 11, 2017

blling commented Oct 11, 2017

syranide commented Oct 12, 2017 •

edited

Loading

nhunzaker commented Oct 12, 2017

Haroenv commented Oct 12, 2017

chore(syntheticEvent): remove IE8 code #11178

chore(syntheticEvent): remove IE8 code #11178

Conversation

Haroenv commented Oct 10, 2017

blling Oct 10, 2017

Choose a reason for hiding this comment

gaearon commented Oct 10, 2017

Haroenv commented Oct 10, 2017

gaearon commented Oct 10, 2017 • edited Loading

jquense Oct 10, 2017

Choose a reason for hiding this comment

nhunzaker Oct 11, 2017

Choose a reason for hiding this comment

nhunzaker Oct 11, 2017

Choose a reason for hiding this comment

Haroenv commented Oct 10, 2017

syranide commented Oct 11, 2017

nhunzaker left a comment

Choose a reason for hiding this comment

nhunzaker commented Oct 11, 2017 • edited Loading

Haroenv commented Oct 11, 2017

nhunzaker Oct 11, 2017

Choose a reason for hiding this comment

Haroenv Oct 11, 2017

Choose a reason for hiding this comment

Haroenv commented Oct 11, 2017

nhunzaker commented Oct 11, 2017

blling commented Oct 11, 2017

nhunzaker commented Oct 11, 2017

blling commented Oct 11, 2017

syranide commented Oct 12, 2017 • edited Loading

nhunzaker commented Oct 12, 2017

Haroenv commented Oct 12, 2017

gaearon commented Oct 10, 2017 •

edited

Loading

nhunzaker commented Oct 11, 2017 •

edited

Loading

syranide commented Oct 12, 2017 •

edited

Loading