Render reported before complete

[drewdaemon]

Describe the issue
The library is reporting a complete render one tick too early.

To Reproduce
Add a breakpoint or log statement to the body of the onRenderChange callback for a chart.

Expected behaviour
onRenderChange should only be called with true when the chart has been rendered to the canvas. Instead, it is being called a tick before the chart is drawn.

Screenshots

Screen.Recording.2023-08-02.at.10.14.03.AM.mov

Look for "visualization took to render after data received" in the above recording ^^. It is logged before the chart is visible.

Version (please complete the following information):

• OS: OSX
• Browser: Brave, Safari
• Elastic Charts: 59.1.0

Additional context
This behavior throws off our performance measurements, making it look like we're rendering the chart faster than we actually are.

The problem may originate with this change.

Errors in browser console
None

[nickofthyme]

Related to changes in #1730

[markov00]

As commented here #1730 (comment)
the original callbacks where wrapped around a RAF because:

If the user attaches very long sync function to a callback then the rendering phase of the event loop will start only after that sync function has completed, freezing the chart until the end of the callback.
Adding a RAF will push the callback in the queue and runs it after the rendering phase, reducing the freeze effect.

We changed that mechanism to give the consumer the control over how to handle that call, but we can probably go back, at least for the onRenderChange

[markov00]

@drewdaemon in the meantime can you check this works on lens side, by creating something like that:

const onRenderChangeFn = (rendered: boolean) => {
    window.requestAnimationFrame(() => changeSomethingOnRenderingFunction(rendered));
};

<Settings onRenderChange={onRenderChangeFn}

[drewdaemon]

in the meantime can you check this works on lens side

Thanks @markov00 . This does resolve the issue.

we can probably go back, at least for the onRenderChange

I'm in favor of this. It would be nice to have a fix here in the charts library, since we otherwise have to remember to add this to every expression renderer.

[markov00]

@nickofthyme what do you think about bringing back the RAF only for the onRenderChange?

[nickofthyme]

Yeah I think that's the best solution, I don't see another way to fix it.