Skip to content

Use MutationObserver to detect when canvas is attached#5955

Closed
simonbrunel wants to merge 2 commits intochartjs:masterfrom
simonbrunel:chore/mutation-observer
Closed

Use MutationObserver to detect when canvas is attached#5955
simonbrunel wants to merge 2 commits intochartjs:masterfrom
simonbrunel:chore/mutation-observer

Conversation

@simonbrunel
Copy link
Member

@simonbrunel simonbrunel commented Jan 5, 2019

Using CSS animations to detect when the canvas is attached causes issues when dealing with CSP. Instead, we can use the MutationObserver API to efficiently detect when a node is added or removed, which is now widely adopted (https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver).

MutationObserver is not supported on IE10, however we use this feature only to detect when a node is attached to the DOM. Checking if MutationObserver is defined allows us to keep support for old browsers when the node is already attached, which is the case in most situations.

Any help testing this PR on different browsers (desktop / mobile) is more than welcome! We need to verify that responsiveness still work as expected when the canvas is attached after the chart is created. I think Vue.js or React projects could be concerned.

This PR partially drops support for IE10: should we release at v2.8 or v3 since it could be a breaking change for some projects?

Closes #5952
Fixes #5208

Using CSS animations to detect when the canvas is attached causes issues when dealing with CSP. Instead, we can use the MutationObserver API to efficiently detect when a node is added or removed, which is now widely adopted (https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver).

MutationObserver is not supported on IE10, however we use this feature only to detect when a node is attached to the DOM. Checking if MutationObserver is defined allows us to keep support for old browsers when the node is already attached, which is the case in most situations.
@etimberg
Copy link
Member

etimberg commented Jan 5, 2019

I will try this on a few browsers and report back. In terms of 2.8 vs 3.0, our homepage says "IE9+" so we would need to update that. I think its safer to do as a major version upgrade unless we keep some backwards compatibility

@simonbrunel
Copy link
Member Author

simonbrunel commented Jan 5, 2019

With MutationObserver, we lose the ability to detect when a node become 'displayed' (blocker), for example when one of its parent or itself transit from display: none to display: block (#2114 and more). That's the remaining failing unit test.

@simonbrunel
Copy link
Member Author

I can't find a decent solution to handle display, which is a common use case (for example when displaying charts in different tabs). Using MutationObserver would require to observe style mutations for all elements and then check if the canvas visibility have changed. I guess this will be catastrophic in term of performance when integrated in complex SPA, though we could monitor only branches to our canvases, but this would require complex observe/disconnect logic (e.g. when re-parenting a canvas). But anyway, observing style attribute mutations doesn't work if display is changed via CSS rules (e.g. .tab.active > .content { display: block }), which is also a common practice.

Even if the keyframe/animation method is not a "modern" solution, it works in all use cases currently reported (except in shadow DOM, but I think it's the same issue with MutationObserver). It also performs much better because we don't need to monitor all elements.

I'm open to any suggestion to make it works efficiently but it seems that it will be really complicated and risky to use MutationObserver to replace the current animation approach. IMO, we should investigate another direction to fix/support CSP than trying to get rid of the injected stylesheet.

@benmccann
Copy link
Contributor

@etimberg Chart.js doesn't work with IE9. The homepage says "IE9+" only because it's running an old version of the code. The latest code in https://github.com/chartjs/www.chartjs.org says IE11 as does https://www.chartjs.org/docs/latest/developers/. Any idea how we can get the latest code deployed to the homepage?

@simonbrunel
Copy link
Member Author

I'm closing this PR since it doesn't fully work and could break existing responsive projects. We can re-open later if we figure out an optimal working way to detect display style changes.

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.

3 participants