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

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

Merged
merged 1 commit into from Sep 25, 2018

Conversation

Projects
None yet
7 participants
@sergei-startsev
Contributor

sergei-startsev commented Sep 19, 2018

@sizebot

This comment has been minimized.

sizebot commented Sep 20, 2018

Details of bundled changes.

Comparing: e1a067d...d68bbcb

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.1% +0.1% 652.01 KB 652.36 KB 152.82 KB 152.9 KB UMD_DEV
react-dom.development.js +0.1% +0.1% 647.37 KB 647.71 KB 151.44 KB 151.52 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.1% 441.87 KB 442.21 KB 99.22 KB 99.3 KB UMD_DEV
react-art.development.js +0.1% +0.1% 373.63 KB 373.97 KB 82.02 KB 82.1 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.1% +0.1% 385.6 KB 385.94 KB 84.56 KB 84.64 KB UMD_DEV
react-test-renderer.development.js +0.1% +0.1% 381.19 KB 381.53 KB 83.43 KB 83.52 KB NODE_DEV

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.1% +0.1% 369.52 KB 369.86 KB 80.14 KB 80.23 KB NODE_DEV
react-reconciler-persistent.development.js +0.1% +0.1% 367.96 KB 368.3 KB 79.51 KB 79.59 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.1% 500.86 KB 501.22 KB 110.99 KB 111.08 KB RN_FB_DEV
ReactNativeRenderer-dev.js +0.1% +0.1% 500.52 KB 500.88 KB 110.91 KB 110.99 KB RN_OSS_DEV
ReactFabric-dev.js +0.1% +0.1% 491.05 KB 491.42 KB 108.58 KB 108.67 KB RN_FB_DEV
ReactFabric-dev.js +0.1% +0.1% 491.09 KB 491.45 KB 108.6 KB 108.68 KB RN_OSS_DEV

scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
scheduler.development.js n/a n/a 0 B 19.17 KB 0 B 5.74 KB UMD_DEV
scheduler.production.min.js n/a n/a 0 B 3.16 KB 0 B 1.53 KB UMD_PROD

Generated by 🚫 dangerJS

@gaearon

This comment has been minimized.

Member

gaearon commented Sep 20, 2018

Can we remove the window.event assignment now? Or replace that code with this one?

@viniciusll

This comment has been minimized.

viniciusll commented Sep 20, 2018

@sergei-startsev

This comment has been minimized.

Contributor

sergei-startsev commented Sep 20, 2018

@gaearon Could you clarify the idea of removing window.event assignment? It was initially added in #11696 to fix #11687, I'm not quite sure we can just remove it...

@gaearon

This comment has been minimized.

Member

gaearon commented Sep 20, 2018

I mean, if we're restoring the descriptor, wouldn't that be sufficient to cover that use case too? I thought that would capture its current value too.

@asapach

This comment has been minimized.

asapach commented Sep 20, 2018

@gaearon, the descriptor doesn't have a value, see #13688 (comment). The getter will instead return the current event emitted by dispatchEvent(). Which is what #11696 was trying to address.

@gaearon

This comment has been minimized.

Member

gaearon commented Sep 20, 2018

I just don't see how it makes sense that we both attempt to defineProperty and write to it.

Can we change the place where we write to replace the descriptor getter, and later restore it?

@sergei-startsev

This comment has been minimized.

Contributor

sergei-startsev commented Sep 20, 2018

@gaearon callCallback is called when a fake react-invokeguardedcallback event is dispatched, window.event is overridden by original event in callCallback, so you cannot replace it by using the descriptor, it will restore a fake event, that's what you would like to avoid. The following code doesn't work properly:

- window.event = windowEvent;
+ Object.defineProperty(window, 'event', windowEventDescriptor);

After all required manipulations with fake events we have to restore the default behavior for window.event.

Object.defineProperty(window, 'event', windowEventDescriptor);

Was this helpful?

@gaearon

This comment has been minimized.

Member

gaearon commented Sep 20, 2018

Can we do

Object.defineProperty(window, 'event', {
  ...windowEventDescriptor,
  get() { return windowEvent }
});

?

and then just

Object.defineProperty(window, 'event', windowEventDescriptor)
@sergei-startsev

This comment has been minimized.

Contributor

sergei-startsev commented Sep 20, 2018

Will it be actually beneficial? We still have to keep both windowEvent and windowEventDescriptor...

@gaearon

This comment has been minimized.

Member

gaearon commented Sep 20, 2018

It's strange that we're changing the characteristics of a descriptor. E.g. I didn't realize it was a getter, or non-enumerable.

If it's consistently a getter across browsers that support it, we should just keep it as a getter and not mess with it.

@sergei-startsev

This comment has been minimized.

Contributor

sergei-startsev commented Sep 20, 2018

I'm not quite sure regarding browser consistency, e.g. the issue isn't reproduced in FF 62 (the latest release version) -- window.event isn't supported here, but it looks like they implemented it in FF 63. The issue isn't also reproduced in IE 11.

@sergei-startsev

This comment has been minimized.

Contributor

sergei-startsev commented Sep 20, 2018

E.g. Object.getOwnPropertyDescriptor(window, 'event') returns undefined in IE 11. It also returns false if you try to get own property window.hasOwnProperty('event')...

@gaearon

This comment has been minimized.

Member

gaearon commented Sep 20, 2018

What a mess.

@gaearon

This comment has been minimized.

Member

gaearon commented Sep 20, 2018

OK. I can merge this if you can help test in all environments: IE9+ (would require some polyfills), including IE11, then different versions of FF and Chrome. And some mobile (e.g. iOS and Chrome on Android).

@sergei-startsev

This comment has been minimized.

Contributor

sergei-startsev commented Sep 20, 2018

OK, I'll try to cover all of them except iOS/Safari -- it can be a bit problematic.

@sergei-startsev

This comment has been minimized.

Contributor

sergei-startsev commented Sep 21, 2018

@gaearon I've used the following code to check that the fix works properly:

console.log('Global `window.event` descriptor before/after dispatching a fake event:');

console.log(Object.getOwnPropertyDescriptor(window, 'event'));
  ReactDOM.render(
    <h1>React Component</h1>,
    document.getElementById('container')
  );
console.log(Object.getOwnPropertyDescriptor(window, 'event'));

There're different results of window.event descriptor before and after dispatching react-invokeguardedcallback fake event without applying the fix, e.g. in Chrome 69:

{"enumerable":false,"configurable":true}
{"value":{"isTrusted":true},"writable":true,"enumerable":true,"configurable":true

Here's the proof.

With the applied fix descriptors are the same:

{"enumerable":false,"configurable":true}
{"enumerable":false,"configurable":true}

Note: Since FF 62 and IE9-11 returns false for window.hasOwnProperty('event') check, window.event will never be overridden:

if (
typeof window.event !== 'undefined' &&
window.hasOwnProperty('event')
) {
window.event = windowEvent;
}

And the issue isn't reproduced here.


I've checked the fix for the following environments:

  • Windows 10

    • Chrome 69 +
    • Chrome 71 canary +
    • FF 62 (window.event isn't supported) +
    • FF 63 Beta (window.event is supported) +
    • Edge 42 +
    • IE 11 +
    • IE 10 +
    • IE 9 +
  • Android 8

    • Chrome 69 +

I would appreciate if somebody could check it in iOS/Safari. Anyway I don't see why it shouldn't work here.

@philipp-spiess

This comment has been minimized.

Collaborator

philipp-spiess commented Sep 21, 2018

@sergei-startsev Looking good on iOS 12, 11, and 10 here. Unfortunately JSFiddle doesn't work on older iOS versions - but running the following script in the console works on iOS 8 and 9 as well:

var descriptor = Object.getOwnPropertyDescriptor(window, 'event');
Object.defineProperty(window, 'event', descriptor);
@sergei-startsev

This comment has been minimized.

Contributor

sergei-startsev commented Sep 21, 2018

@philipp-spiess thanks for checking!

@sergei-startsev

This comment has been minimized.

Contributor

sergei-startsev commented Sep 24, 2018

@gaearon Is there anything else we would like to check here?

@gaearon gaearon merged commit 17e703c into facebook:master Sep 25, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@gaearon

This comment has been minimized.

Member

gaearon commented Sep 25, 2018

LGTM, thanks for testing

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