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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closes issue #4287 #4356

Closed
wants to merge 1 commit into from
Closed

Closes issue #4287 #4356

wants to merge 1 commit into from

Conversation

asimkt
Copy link

@asimkt asimkt commented Jun 9, 2017

[BUG] Violation warnings in verbose level chrome dev tools logging.
I hope this closes #4287

I'm not sure whether this is the way to solve this issue. From https://developers.google.com/web/updates/2016/06/passive-event-listeners it may need to be in passive for a better scrolling in touch devices. Please comment and I'll change accordingly.

Note: This's my first PR to a public repo. 馃槃

[BUG] Violation warnings in verbose level chrome dev tools logging
@etimberg etimberg added this to the Version 2.7 milestone Jun 9, 2017
@@ -723,7 +723,7 @@ module.exports = function(Chart) {
};
helpers.addEvent = function(node, eventType, method) {
if (node.addEventListener) {
node.addEventListener(eventType, method);
node.addEventListener(eventType, method, {passive: true});
Copy link
Member

Choose a reason for hiding this comment

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

Will adding this parameter cause issues on browsers other than Chrome? We should confirm that this doesn't break IE11 compatibility.

Copy link
Member

Choose a reason for hiding this comment

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

It's not supported in IE and Edge and therefore options support needs to be detected in order to call addEventListener with the correct arguments.

Copy link
Author

Choose a reason for hiding this comment

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

Is there any browser detection function which is already in Chart.js code? Then we could use that. I have found Chart.platform but I don't think it can be used to detect IE and Edge. Any thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

@asimkt while re-organizing our helpers, I moved addEvent and removeEvent in platform.core.js but also implemented this feature detection, I hope you don't mind (see #4424).

@etimberg
Copy link
Member

Closing since this got implemented in #4424

@etimberg etimberg closed this Jun 25, 2017
@simonbrunel simonbrunel removed this from the Version 2.7 milestone Aug 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Violation warnings in verbose level chrome dev tools logging
3 participants