[BUG] iOS click event issue and instanceof failing

[Flamenco]

I have a runtime that adds charts to multiple canvases on the same page. I have a list of charts in the left column, and every time one of those items are clicked, I add a chart on the right column. I am using Vaadin in my runtime stack.

On iOS, after I add a chart, the Vaadin stack no longer receives any click events.

So I set events=[] hoping that would solve the problem. It did not.

So instead of adding canvas, I added iframes, then added canvas to the iframes, and fed that canvas to Charts.js, in hopes of isolating the click eater.

Charts.js would not accept the canvas or it's context2d because BOTH of the instanceof checks in platform.dom.js. fail when the canvas is coming from the iframe. When I remove the type checks, Charts.js works like a charm.

	return {
		acquireContext: function(item, config) {
			if (typeof item === 'string') {
				item = document.getElementById(item);
			} else if (item.length) {
				// Support for array based queries (such as jQuery)
				item = item[0];
			}

			if (item && item.canvas) {
				// Support for any object associated to a canvas (including a context2d)
				item = item.canvas;
			}

			if (item instanceof HTMLCanvasElement) {
				// To prevent canvas fingerprinting, some add-ons undefine the getContext
				// method, for example: https://github.com/kkapsner/CanvasBlocker
				// https://github.com/chartjs/Chart.js/issues/2807
				var context = item.getContext && item.getContext('2d');
				if (context instanceof CanvasRenderingContext2D) {
					initCanvas(item, config);
					return context;
				}
			}

			return null;
		}

Environment

• Chart.js version: 2.5
• Browser name and version: iOS safari and chrome
• Link to your project: n/a

[etimberg]

@Flamenco off the top of my head I'm not sure what's causing this. Do you have a fiddle that reproduces this? Does this only happen on Safari for iOS?

Have you tried maybe breakpointing the click event in chrome and seeing what's handling it? I think the remote chrome debugging will work OK for doing that

[Flamenco]

This shows the instanceof issue:
https://jsfiddle.net/spungin/2sk21h4x/

The click issue is a bit tricky, as it is in a stack that has many layers and cannot be posted. Ideally if I set events=[], no event listeners should be registered for mouse or touch events. I might strip them out of the source and see if that solves my issue on that front.

[Flamenco]

Seems like the iframe puts some kind of proxy around the elements...

[Flamenco]

The click issue only happens on iOS safari (and chrome because apple makes google use their crap internally). So more likely touch events then click events.

[etimberg]

Everything in the iframe has it's own prototype. canvas_iframe instanceof iframe.contentWindow.HTMLCanvasElement returns true. This is as expected because the frame needs to be completely independent from the rest of the page and a script inside a frame could change window.HTMLCanvasElement to something else and it would be wrong to break the content outside of the frame in the rest of the page.

If Chart.js were included inside the iframe it would have the correct definition and would work as you expect. In terms of adding event listeners, we only do that if specified per https://github.com/chartjs/Chart.js/blob/master/src/core/core.controller.js#L704-L707

[Flamenco]

It does work when I remove the instanceof checks, so perhaps Chart.js should determine validity in a different way.

[Flamenco]

BTW

canvas_iframe instanceof iframe.contentWindow.HTMLCanvasElement returns true.

That's good to know! Thanks.

[Flamenco]

Should I close this, or were you thinking about updating the code to be more inclusive?

[etimberg]

My inclination is to close this. @simonbrunel thoughts?

[Flamenco]

If I send in a valid context or canvas, that actually does work, I thinks its bug to reject it in acquireContext()

[etimberg]

The big issue here is sending a context from a different frame. I think you should just include Chart.js inside the iframe and the problem will go away. I don't know that we want to support rendering in one frame with a canvas from another. That seems like a bit of an anti-pattern

[Flamenco]

No, I don't think so. The big issue is you are forcing your library to conform to an implementation and not an interface. Calling my use-case an anti-pattern is just plain off-base. The problem is a weak design pattern in Charts.js.

For example, I have developed context2d adapters/renderers that generate PDF and SVG. I send my context2d compatible object into Charts.js and get VECTOR quality charts in PDF and SVG. But I have to hack your library because it discriminates. Also, in this case, there is no IFRAME that I am running the code in. (https://github.com/MrRio/jsPDF/blob/master/plugins/context2d.js)

In the example initially posted, I am dynamically creating an iframe and canvas; it makes no sense to load a separate version of Charts.js and configuration into each one when it is absolutely not necessary, and the charts are controlled/integrated with other elements in the main window, outside the IFRAME.

I can't understand why anybody in their right mind would want to limit the use of a library in such a way. IMHO, if the canvas has a context2d and the context2d has a canvas, that should be good enough to pass the test.

[simonbrunel]

The context2d adapters/renderers is a special case which is not fully compatible with the platform.dom.js (neither a requirement). It should be a different platform implementation (platform.adapter.js maybe?), much simpler (actually just a few lines), that only deal with a context2d compatible interface (i.e. no responsiveness or DOM interactions).

However, I agree that the IFRAME use case should be handled by platform.dom.js. If it works for you and it doesn't break unit tests, I'm fine with changing these checks for:

	// ...
	if (item && item.canvas) {
		// Support for any object associated to a canvas (including a context2d)
		item = item.canvas;
	}

	// To prevent canvas fingerprinting, some add-ons undefine the getContext
	// method, for example: https://github.com/kkapsner/CanvasBlocker
	// https://github.com/chartjs/Chart.js/issues/2807
	var context = item && item.getContext && item.getContext('2d');
	if (context) {
		initCanvas(item, config);
		return context;
	}

	return null;

[Flamenco]

@simonbrunel That all makes sense to me :)

Splitting the adapter use case into a non-responsive adapter platform seems like a wise choice.

Another thought to your solution is to and add an else if after (instead of replacing) the current code, and set a flag in the config so the internals can check if the canvas was embedded, if the need arises.

[simonbrunel]

Having a valid canvas is currently one of the requirements of platform.dom.js and that's mainly why the context2d adapter case is not fully compatible with this implementation. I think the else if you are referring to, should be: else use platform.adapter.js as fallback, but that should happen much earlier, outside platform.dom.js, while determining the target platform.

[Flamenco]

I mean something like this for the first use case (not adapter):

if (item instanceof HTMLCanvasElement) {
				// To prevent canvas fingerprinting, some add-ons undefine the getContext
				// method, for example: https://github.com/kkapsner/CanvasBlocker
				// https://github.com/chartjs/Chart.js/issues/2807
				var context = item.getContext && item.getContext('2d');
				if (context instanceof CanvasRenderingContext2D) {
					initCanvas(item, config);
					return context;
				}
			}
else if (item && item.getContext && item.getContext('2d') {
				config._isInIframe = true;
				initCanvas(item, config);
			      return context;
}

[simonbrunel]

Your else if condition includes the first one and doesn't really mean: "the canvas is in an IFRAME" but "item is not a HTMLCanvasElement because ????". And there are more reliable ways to detect that the canvas is contained in IFRAME (e.g. canvas.ownerDocument !== document), so basically your code could be written like that:

    var context = item && item.getContext && item.getContext('2d');
    if (context) {
        config._isInIframe = item.ownerDocument !== document;
        initCanvas(item, config);
        return context;
    }

But handling canvas in IFRAME should not become a special case across the code so I would not add this extra config._isInIframe until we really need it. Also removing the two instanceof can help to fix #3696 and #3887 :)

[Flamenco]

This is true. I am just showing a case in point: to have an else statement that detects the condition and sets a flag, whilst keeping your current code. MIght come in handy some day.

[mgetzbw]

@simonbrunel just tested your fix in the context of #3887, it works. But the resize detection iframe doesn't seem to pick up on the window resizes; this is however as best I can tell a completely separate bug (the container is position:relative;).

[simonbrunel]

@mgetzbw that sounds great, thanks for testing! For the resize detection, are you able to share a live test case of the issue?

[mgetzbw]

@simonbrunel not at this time unfortunately. It's most likely an issue I should probably file against Salesforce's aura framework as the iframe is being created in the primary dom. I suspect it has to do with the fact they intercept all event handlers to keep components from messing with each other. If you're using an onresize handler they seem to be blocking it.

[simonbrunel]

The "canvas in iframe" issue has been reported in this ticket #4102

[etimberg]

Resolved in #4165