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 for issue 6887 #6893

Closed
wants to merge 2 commits into from
Closed

fix for issue 6887 #6893

wants to merge 2 commits into from

Conversation

f0rk
Copy link

@f0rk f0rk commented May 26, 2016

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

  1. Fork the repo and create your branch from master.
  2. If you've added code that should be tested, add tests!
  3. If you've changed APIs, update the documentation.
  4. Ensure the test suite passes (grunt test).
  5. Make sure your code lints (grunt lint) - we've done our best to make sure these rules match our internal linting guidelines.
  6. If you haven't already, complete the CLA.

@gaearon
Copy link
Collaborator

gaearon commented Jun 26, 2016

So far this doesn’t look like a browser inconsistency—more like a bug caused by another script or a third party extension. We can’t possibly fix every such error so we’d only merge this if this is due to browser differences. If this is the case, please help us verify this: #6887 (comment). Otherwise your best bet might be to override createEvent yourself again to protect against the third party code that hijacks it.

/**
* @param {boolean} hasEventPageXY
*/
injectHasEventPageXY: function(val) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of adding this method (which will end up in prod builds for no gain), let’s extract the if block contents into something like:

testEventPageXY: function() {
  if (document.createEvent) {
    hasEventPageXY = evt !== null && 'pageX' in evt;
  } else {
    hasEventPageXY = false;
  }
}

Then call this method both from ensureScrollValueMonitoring when if (hasEventPageXY === undefined) and from tests after stubbing out document.createElement.

@gaearon
Copy link
Collaborator

gaearon commented Aug 26, 2016

Now that we’ve learned more about the underlying causes, I’m happy to merge this if you address the above comments. Thank you so much!

@ghost ghost added the CLA Signed label Aug 26, 2016
@gaearon
Copy link
Collaborator

gaearon commented Aug 31, 2016

Closing as superseded by #7621.

@gaearon gaearon closed this Aug 31, 2016
This pull request was closed.
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.

2 participants