Chart.js icon indicating copy to clipboard operation
Chart.js copied to clipboard

Attach observer doesn't work when canvas is inside of Shadow DOM

Open abaksha-sc opened this issue 2 years ago • 3 comments

Expected behavior

When chart creates for a canvas node which is not connected to DOM yet, then it creates MutationObserver to handle a moment when canvas will be added. But when canvas node adds to DOM as a part of other node with Shadow DOM then attach handler should be called.

Current behavior

Attach handler not calling when canvas node is inside of Shadow DOM and node adds later to DOM (after chart is created). This happens because Chart.JS checks if canvas is added to DOM through justAddedNode.contains(canvas) and it doesn't work with Shadow DOM. This code is here: https://github.com/chartjs/Chart.js/blob/51441272a781ba575149b214933f0c5b4bafb6ab/src/platform/platform.dom.js#L130

For example MutationObserver handles following added node:

<div>
    <my-chart-component-with-shadow-dom>
         #shadow-root:
              <canvas></canvas>  // <-- it is canvas which Chart.JS waits to be added to DOM
    </my-chart-component-with-shadow-dom>
</div>

With this case added node div will return false for node canvas when in real it contains canvas.

Reproducible sample

https://jsfiddle.net/ryf837xs/1/

The same fiddle without Shadow DOM works correct: https://jsfiddle.net/q4gmfy2p/

Optional extra steps/info to reproduce

No response

Possible solution

Maybe it's enough to check just if canvas.isConnected in MutationObserver callback?

Here: https://github.com/chartjs/Chart.js/blob/51441272a781ba575149b214933f0c5b4bafb6ab/src/platform/platform.dom.js#L130

Like this:

function createAttachObserver(chart, type, listener) {
  const canvas = chart.canvas;
  const observer = new MutationObserver(entries => {
    if (canvas.isConnected) {  // <-- here is enough just to check if canvas connected ?
      listener();
    }
  });
  ...

Context

No response

chart.js version

4.0.1

Browser name and version

Chrome 107

Link to your project

No response

abaksha-sc avatar Nov 29 '22 14:11 abaksha-sc

@abaksha-sc Hi. In this case, I'm offering you to move chart initialization into connectedCallback: https://jsfiddle.net/8wr2bn5j/

const observer = new MutationObserver(entries => {
    if (canvas.isConnected) {
      listener();
    }
  });

your solution will not work, or will, but not always correctly, because

  1. this check doesn't mean what canvas wasn't connected before
  2. mutation observer will not be called if the canvas is added inside the shadow

dangreen avatar Dec 15 '22 12:12 dangreen

@dangreen,

this check doesn't mean what canvas wasn't connected before

as I understand MutationObserver will not be created when canvas is connected allready.

https://github.com/chartjs/Chart.js/blob/51441272a781ba575149b214933f0c5b4bafb6ab/src/core/core.controller.js#L1062-L1063

And after handler will be called (another words canvas.isConnected) then listener will be removed: https://github.com/chartjs/Chart.js/blob/51441272a781ba575149b214933f0c5b4bafb6ab/src/core/core.controller.js#L1040-L1041

mutation observer will not be called if the canvas is added inside the shadow

Why? There is observer for whole document. It will be called anyway. Maybe not with canvas node directly, but for example when whole component will be added (which already contains shadow root and canvas).

https://github.com/chartjs/Chart.js/blob/51441272a781ba575149b214933f0c5b4bafb6ab/src/platform/platform.dom.js#L137


My proposal is just to use the same way in both cases.

Because now initialization uses .isConnected to decide if MutationObserver should be created to handle when canvas will be added, but MutationObserver uses node.contains(canvas). It's strange:

  1. Part of initialization (here is .isConnected):

https://github.com/chartjs/Chart.js/blob/51441272a781ba575149b214933f0c5b4bafb6ab/src/core/core.controller.js#L1062-L1063

https://github.com/chartjs/Chart.js/blob/51441272a781ba575149b214933f0c5b4bafb6ab/src/platform/platform.dom.js#L381-L384

  1. When it creates MutationObserver then it uses node.contains(canvas):

https://github.com/chartjs/Chart.js/blob/51441272a781ba575149b214933f0c5b4bafb6ab/src/platform/platform.dom.js#L127-L133

abaksha-sc avatar Dec 15 '22 13:12 abaksha-sc

Maybe it will be helpful for other developers.

In general this issue prevents creating of ResizeObserver to work with option responsive: true correctly. Because it creates after attaching.

As a workaround I disabled responsive (in this case MutationObserver is not needed at all). Then I create ResizeObserver for my component manually and when it triggers I call chart.resize().

When option responsive is disabled then it decides that canvas is attached:

https://github.com/chartjs/Chart.js/blob/51441272a781ba575149b214933f0c5b4bafb6ab/src/core/core.controller.js#L984-L988

abaksha-sc avatar Dec 15 '22 13:12 abaksha-sc