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

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

Closed
sergei-startsev opened this issue Sep 19, 2018 · 8 comments · Fixed by #13697
Closed

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

sergei-startsev opened this issue Sep 19, 2018 · 8 comments · Fixed by #13697

Comments

@sergei-startsev
Copy link
Contributor

sergei-startsev commented Sep 19, 2018

Do you want to request a feature or report a bug?

Report a bug.

What is the current behavior?

Global window.event is overwritten in React 16.5+ in development mode. Here're minimal repro steps:

If you click the button, you see DOMContentLoaded event type.

What is the expected behavior?

The current behavior contradicts with specified behavior window property event returns the Event which is currently being handled by the site's code. Outside the context of an event handler, the value is always undefined. Moreover it works properly in production mode:

It returns expected click event type.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

It works properly in React 16.4.2 and prod mode:

The issue is reproduced in Chrome 69. It works properly in FF 62 (window.event isn't support in 62, however it should be reproduced in FF 63 - it was recently added, see details).

It seems that the issue was introduced by @ConradIrwin in #11696.

@gaearon
Copy link
Collaborator

gaearon commented Sep 19, 2018

Seems like a bug. Can you figure out what's wrong with #11696? Or we could revert it altogether although you'd still get a different event in some cases (which is what #11696 tried to fix).

@asapach
Copy link

asapach commented Sep 19, 2018

Can you figure out what's wrong with #11696?

According to the docs:

Outside the context of an event handler, the value is always undefined.

#11696 sets the value, but never resets it back to undefined:

window.event = windowEvent;

So it always stores some previous event.

@gaearon
Copy link
Collaborator

gaearon commented Sep 19, 2018

Want to send a PR to reset it? As long as we handle nesting correctly.

@asapach
Copy link

asapach commented Sep 19, 2018

@gaearon, I've tried a couple of things, and I don't think it's possible to restore the property correctly.

Initially Object.getOwnPropertyDescriptor(window, 'event') looks like this:

{
  configurable: true,
  enumerable: false,
  get: ...,
  set: ...
}

After it has been assigned (e.g. window.event = undefined or window.event = windowEvent) it looks like this:

{
  configurable: true,
  enumerable: true,
  value: undefined,
  writable: true
}

After delete window.event:

undefined

I would suggest using something other than window.event to fix #11687 (e.g. window.lastEvent) or roll this back entirely.

@ConradIrwin
Copy link
Contributor

I'm sorry about this. In testing I observed that changes to window.event made within event handlers are restored at the end of the event handler. That said I can't reproduce this any more, so I must have been mistaken.

I am happy with either exposing this somewhere other than window.event, or by using the property descriptor to restore itself:

let property = Object.getOwnPropertyDescriptor(window, 'event')

if (property) {
  let windowEvent = window.event
}

function call() {
  window.event = windowEvent
}

dispatchEvent()

if (property) {
  Object.defineProperty(window, 'event', property)
}

@sergei-startsev
Copy link
Contributor Author

@ConradIrwin I've applied the suggested fix for a project where the issue was initially detected -- it works fine for me.

I've also verified a restored descriptor:

image

Let me know if you would like to make PR yourself or delegate it.

@ConradIrwin
Copy link
Contributor

ConradIrwin commented Sep 19, 2018 via email

@sergei-startsev
Copy link
Contributor Author

@ConradIrwin np, the changes look pretty straightforward 😉

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

Successfully merging a pull request may close this issue.

4 participants