Fix window.event in development (#11687) #11696

ConradIrwin · 2017-11-28T23:07:03Z

The code change is relatively simple, but I had to add a fixture to test
this as window.event is not supported by JSDom.

I included a commented-out test copying the pattern of another test in
that file, but I'm not sure how much value that adds so would happily
remove it.

aweary · 2017-11-28T23:17:55Z

packages/shared/ReactErrorUtils.js

@@ -185,6 +190,9 @@ if (__DEV__) {
        // nested call would trigger the fake event handlers of any call higher
        // in the stack.
        fakeNode.removeEventListener(evtType, callCallback, false);
+        if (windowEvent && window.event !== windowEvent) {
+          window.event = windowEvent;


Have you verified that the browsers that support window.event define it as a writable property?

I've verified that the fixture works in Chrome, Safari and IE 11. Firefox does not support this. Is there a complete list of browsers I should try and support, or any support for running the tests/fixtures in a large set of real browser environments?

It might be good to test in IE9 and IE10 as well if they support it.

Does this get properly reset after the event dispatch is complete? We need to be sure that window.event doesn't start persisting, since it should only be defined while an event is in progress.

Good call on checking. It does not work in IE 9 or 10 or 11 (I'm not sure how I persuaded myself that it did before), as it fails with "Assignment to read-only properties is not allowed in strict mode".

The event does get reset correctly in Chrome and Safari, and I've added a check for that to the fixture.

There are two other approaches I considered, but neither seem to work:

Instead of window.event = , defining a property on window that returns the correct event. This works to return the correct event object (as far as I can tell), but the event object is unusable in IE 9 or 10 (it does work in IE 11) because any attempt to access the event's properties throws "Member not found".

Instead of creating a new fake event to dispatch, just re-dispatching window.event. (this fails with an error that "event is already being dispatched")

For now I've updated this pull request to not preserve window.event in IE at all. If it would be preferred, I can make it work in IE 11 using window.defineProperty — it still will not work in IE9 or 10, but it can be made to not crash.

gaearon · 2017-11-29T17:44:13Z

packages/shared/ReactErrorUtils.js

@@ -185,6 +190,9 @@ if (__DEV__) {
        // nested call would trigger the fake event handlers of any call higher
        // in the stack.
        fakeNode.removeEventListener(evtType, callCallback, false);
+        if (windowEvent && window.event !== windowEvent) {


I would prefer a strict check here (e.g. typeof windowEvent !== 'undefined' || windowEvent !== null, depending on what actually happens).

gaearon · 2017-11-29T17:45:04Z

packages/shared/__tests__/ReactErrorUtils-test.internal.js

@@ -55,6 +55,34 @@ describe('ReactErrorUtils', () => {
    expect(context.didCall).toBe(true);
  });

+  if (!__DEV__ && window.hasOwnProperty('event')) {


So is this always false? Then this shouldn't be a test at all if it never executes.

You could, however, "polyfill" window.event in jsdom yourself somehow.

aweary · 2017-11-29T17:46:06Z

Re: #11696 (comment)

If we can't find a solution that works consistently it may be better for us to document this as a limitation of the synthetic event system. Even though window.event has decent browser support, it's non-standard, which is why we have these inconsistencies.

ConradIrwin · 2017-11-30T02:56:26Z

@gaearon I've updated the checks to be more precise, and removed the test that cannot be run :).

@aweary I'd very much like to have this work in development — in production it works just fine already. For more of the backstory see #11687. If it would be better to make it work in IE 11 too I can do that, it's just more code.

gaearon · 2018-01-05T17:31:36Z

Could you please fix the CI and merge conflicts?

ConradIrwin · 2018-01-05T17:36:20Z

Will do.

ConradIrwin · 2018-01-18T06:12:11Z

done!

ibash · 2018-05-07T22:07:49Z

@gaearon It looks like setHandleTopLevel has been removed. Would love to have this PR merged so we can upgrade!

ConradIrwin · 2018-05-25T17:08:13Z

@aweary @gaearon This is blocking our ability to upgrade to React 16.3. Is there any chance of merging this?

gaearon · 2018-08-09T17:40:05Z

packages/shared/invokeGuardedCallback.js

+        // in IE as it crashes in strict mode.
+        if (
+          typeof window.event !== 'undefined' &&
+          window.hasOwnProperty('event')


What's the story behind this?
Why does window.hasOwnProperty('event') tell us whether it's writable?

@gaearon Thanks for looking into this.

It is useful for two reasons, only one of which I added to the comment:

Running window.event = event on IE <= 10 throws an error "SCRIPT3: Member Not Found". As far as I can tell, this has nothing to do with anything other than "running in IE <= 10". So I needed at least a feature check for these browsers.

Firefox does not support window.event at all. So I wanted a check for browsers that usually have an event property on window.

By happy coincidence this check does both. I've updated the comment, and rebased over your latest changes.

Please let me know if I can improve this more, as this is currently the main thing holding us back on 16.2.

Before this change in development window.event was overridden in invokeGuardedCallback. After this change window.event is preserved in the browsers that support it.

pull-bot · 2018-08-14T05:42:54Z

Details of bundled changes.

Comparing: 5550ed4...1a710d3

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom.development.js	+0.1%	+0.1%	643.46 KB	644.05 KB	151.44 KB	151.6 KB	UMD_DEV
react-dom.development.js	+0.1%	+0.1%	639.59 KB	640.19 KB	150.29 KB	150.45 KB	NODE_DEV

react-art

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-art.development.js	+0.1%	+0.2%	434.94 KB	435.54 KB	98.73 KB	98.9 KB	UMD_DEV
react-art.development.js	+0.2%	+0.2%	367.46 KB	368.06 KB	81.67 KB	81.84 KB	NODE_DEV

react-test-renderer

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-test-renderer.development.js	+0.2%	+0.2%	363.36 KB	363.95 KB	80.07 KB	80.24 KB	UMD_DEV
react-test-renderer.development.js	+0.2%	+0.2%	359.49 KB	360.08 KB	79.11 KB	79.28 KB	NODE_DEV

react-reconciler

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-reconciler.development.js	+0.2%	+0.2%	348.48 KB	349.07 KB	75.74 KB	75.9 KB	NODE_DEV
react-reconciler-persistent.development.js	+0.2%	+0.2%	347.09 KB	347.69 KB	75.18 KB	75.35 KB	NODE_DEV

react-native-renderer

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
ReactNativeRenderer-dev.js	+0.1%	+0.2%	482.65 KB	483.27 KB	106.88 KB	107.05 KB	RN_FB_DEV
ReactNativeRenderer-dev.js	+0.1%	+0.2%	482.38 KB	483 KB	106.83 KB	107 KB	RN_OSS_DEV
ReactFabric-dev.js	+0.1%	+0.2%	472.86 KB	473.48 KB	104.45 KB	104.62 KB	RN_FB_DEV
ReactFabric-dev.js	+0.1%	+0.2%	472.89 KB	473.52 KB	104.46 KB	104.63 KB	RN_OSS_DEV

Generated by 🚫 dangerJS

gaearon · 2018-08-14T17:36:42Z

Which browsers have you tested this on?

ConradIrwin · 2018-08-14T17:43:57Z

Lastest Chrome, Safari, Firefox, IE 11, 10, 9

gaearon · 2018-08-14T17:45:14Z

Can you please also test iOS Safari and Android?

ConradIrwin · 2018-08-14T20:05:47Z

Also checked on my iPhone's Safari, and a friend's Android Chrome at it works as expected.

gaearon · 2018-08-14T20:35:43Z

Cool. Thanks!

ConradIrwin · 2018-08-14T20:44:28Z

👍 Thank you for all your help!

facebook-github-bot added the CLA Signed label Nov 28, 2017

ConradIrwin mentioned this pull request Nov 28, 2017

react-dom: Ability to access window.event in development #11687

Closed

aweary reviewed Nov 28, 2017

View reviewed changes

gaearon reviewed Nov 29, 2017

View reviewed changes

gaearon reviewed Aug 9, 2018

View reviewed changes

Ability to access window.event in development (#11687)

1a710d3

Before this change in development window.event was overridden in invokeGuardedCallback. After this change window.event is preserved in the browsers that support it.

gaearon approved these changes Aug 14, 2018

View reviewed changes

gaearon merged commit 69e2a0d into facebook:master Aug 14, 2018

gaearon mentioned this pull request Sep 5, 2018

Changelog for 16.5.0 #13571

Merged

This was referenced Sep 19, 2018

Global window.event is overwritten in React 16.5+ in development mode. #13688

Closed

Restore global window.event after event dispatching (#13688) #13697

Merged

dependencies bot mentioned this pull request Oct 1, 2018

Update react-dom in /modules/frontend from 16.3.0 to 16.5.2 project-accelerate/accelerate#277

Open

This was referenced Nov 1, 2018

Update react-dom in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#253

Closed

Update react in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#254

Closed

This was referenced Dec 1, 2018

Update react-test-renderer in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#312

Open

Update react in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#313

Open

gaearon mentioned this pull request Jul 1, 2020

Fix development mode hang when iframe is removed #19220

Merged

This was referenced Dec 18, 2020

[Snyk] Security upgrade lighthouse from 2.0.0 to 7.0.0 cajonemsft/react#48

Open

[Snyk] Security upgrade lighthouse from 2.0.0 to 7.0.0 heshamelmasry77/react#38

Open

This was referenced Sep 16, 2021

[Snyk] Security upgrade lighthouse from 2.0.0 to 7.0.0 heshamelmasry77/react#48

Open

[Snyk] Security upgrade lighthouse from 2.0.0 to 7.0.0 cajonemsft/react#78

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix window.event in development (#11687) #11696

Fix window.event in development (#11687) #11696

ConradIrwin commented Nov 28, 2017 •

edited by aweary

aweary Nov 28, 2017

ConradIrwin Nov 28, 2017

aweary Nov 28, 2017

ConradIrwin Nov 29, 2017

gaearon Nov 29, 2017

gaearon Nov 29, 2017

aweary commented Nov 29, 2017

ConradIrwin commented Nov 30, 2017

gaearon commented Jan 5, 2018

ConradIrwin commented Jan 5, 2018

ConradIrwin commented Jan 18, 2018

ibash commented May 7, 2018

ConradIrwin commented May 25, 2018

gaearon Aug 9, 2018

ConradIrwin Aug 14, 2018

pull-bot commented Aug 14, 2018

react-dom

react-art

react-test-renderer

react-reconciler

react-native-renderer

gaearon commented Aug 14, 2018

ConradIrwin commented Aug 14, 2018

gaearon commented Aug 14, 2018

ConradIrwin commented Aug 14, 2018

gaearon commented Aug 14, 2018

ConradIrwin commented Aug 14, 2018

Fix window.event in development (#11687) #11696

Fix window.event in development (#11687) #11696

Conversation

ConradIrwin commented Nov 28, 2017 • edited by aweary

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 Nov 29, 2017

ConradIrwin commented Nov 30, 2017

gaearon commented Jan 5, 2018

ConradIrwin commented Jan 5, 2018

ConradIrwin commented Jan 18, 2018

ibash commented May 7, 2018

ConradIrwin commented May 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pull-bot commented Aug 14, 2018

react-dom

react-art

react-test-renderer

react-reconciler

react-native-renderer

gaearon commented Aug 14, 2018

ConradIrwin commented Aug 14, 2018

gaearon commented Aug 14, 2018

ConradIrwin commented Aug 14, 2018

gaearon commented Aug 14, 2018

ConradIrwin commented Aug 14, 2018

ConradIrwin commented Nov 28, 2017 •

edited by aweary