Fix #22581 by introducing an artificial delay#22601
Fix #22581 by introducing an artificial delay#22601chrisdavies merged 5 commits intoelastic:masterfrom
Conversation
waitForRenderComplete function in reporting
chrisdavies
added
Feature:Visualizations
| // we wait for the event loop to flush before resolving the promise. This | ||
| // seems to correct the timing issue and prevents us from capturing | ||
| // visualization screenshots before they're rendered. | ||
| visualization.addEventListener('renderComplete', () => setTimeout(resolve)); |
There was a problem hiding this comment.
We could probably put the setTimeout in a .then at the bottom of this function... (e.g. return Promise.all(renderedTasks).then(new Promise(r => setTimeout(r))); That would prevent us from queuing up a setTimeout per visualization, but would still give us the desired effect of waiting for the event loop to flush before resolving.
💚 Build Succeeded |
💚 Build Succeeded |
|
retest |
💚 Build Succeeded |
|
retest |
💚 Build Succeeded |
|
retest |
stacey-gammon
left a comment
stacey-gammon
left a comment
There was a problem hiding this comment.
As discussed offline, I think it's fine to check this in as a short term fix so we can both fix the tests and the legit reporting bug, but the longer term fix should ideally be in the visualization code to emit the renderComplete flag only after the visualizations are loaded in the DOM.
Lets file an issue for that to keep track and then we can loop back around to this.
|
For the record, the related bug is here: |
|
Gah! Gradle failing again. |
|
retest |
💔 Build Failed |
|
retest |
timroes
left a comment
timroes
left a comment
There was a problem hiding this comment.
LGTM as a workaround for the broken vis behavior until we can fix that properly
|
Gradle failed to build again. |
…81-blank-report-visualizations
💔 Build Failed |
💔 Build Failed |
|
retest |
💔 Build Failed |
…81-blank-report-visualizations
💔 Build Failed |
|
hmmm, looks like this might not be the solution. Latest failure is snapshot failure with no pie chart.
jenkins test this |
|
Yup. We need a robust way to determine if a visualization is actually present and visible in the DOM. Looks like timing-based solutions are not going to work. The original code waited for the visualizations to report "ready", then resized the browser, then rendered the report. We now resize the browser, wait for "ready", then render the report. The resize in the original seems to have introduced a long enough delay that the visualizations actually were ready by the time we started capturing. We reordered things due to another bug, so I don't think we want to change the order. Instead, we want to come up with a robust way of reporting "ready". While we wait for that solution, we can come up with workarounds such as introducing a longer delay following the "ready" event. |
💔 Build Failed |
|
Thinking about this this AM, and what kind of workarounds we might put in place.
For the latter, we could use a function that looks something like this: function waitForDOMToSettle(fn, ms) {
let timeout;
const node = document.body;
const observer = new MutationObserver(onChange);
function onChange() {
clearTimeout(timeout);
timeout = setTimeout(doneObserving, ms);
}
function doneObserving() {
observer.disconnect();
fn();
}
observer.observe(node, { childList: true, subtree: true });
onChange();
} |
💔 Build Failed |
|
retest |
💚 Build Succeeded |
Introduce a delay into reports to allow visualizations time to appear in the DOM. This is intended as a temporary (and hacky) workaround until we come up with a more robust way to determine that all of the visualizations on the page are ready for capture.
Introduce a delay into reports to allow visualizations time to appear in the DOM. This is intended as a temporary (and hacky) workaround until we come up with a more robust way to determine that all of the visualizations on the page are ready for capture.
4 participants
Chromium reports occasionally render a blank initial visualization. It seems that this might happen due to visualizations triggering
renderCompleteprior to actually being in the DOM. This change introduces asetTimeoutin thewaitForRenderCompletefunction which will hopefully mitigate the issue.