Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Aug 14, 2018
Merged

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

merged 1 commit into from
Aug 14, 2018

Conversation

ConradIrwin
Copy link
Contributor

@ConradIrwin ConradIrwin commented Nov 28, 2017

Fixes #11687

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.

@@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

if (!__DEV__ && window.hasOwnProperty('event')) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

aweary commented Nov 29, 2017

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
Copy link
Contributor Author

@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
Copy link
Collaborator

gaearon commented Jan 5, 2018

Could you please fix the CI and merge conflicts?

@ConradIrwin
Copy link
Contributor Author

Will do.

@ConradIrwin
Copy link
Contributor Author

done!

@ibash
Copy link
Contributor

ibash commented May 7, 2018

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

@ConradIrwin
Copy link
Contributor Author

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gaearon Thanks for looking into this.

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

  1. 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.
  2. 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
Copy link

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
Copy link
Collaborator

gaearon commented Aug 14, 2018

Which browsers have you tested this on?

@ConradIrwin
Copy link
Contributor Author

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

@gaearon
Copy link
Collaborator

gaearon commented Aug 14, 2018

Can you please also test iOS Safari and Android?

@ConradIrwin
Copy link
Contributor Author

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

@gaearon gaearon merged commit 69e2a0d into facebook:master Aug 14, 2018
@gaearon
Copy link
Collaborator

gaearon commented Aug 14, 2018

Cool. Thanks!

@ConradIrwin
Copy link
Contributor Author

👍 Thank you for all your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

react-dom: Ability to access window.event in development
6 participants