Canvas opacity not updating with slow loading images

[danieltbrennan]

Noting a bug that we've encountered when working with an image service that is a bit slower to load than is typical. It seems that a scenario can arise where none of the conditions necessary to update the opacity of the canvas from 0 to 1 in pendingUpdate will ever be met.

atlas/src/modules/canvas-renderer/canvas-renderer.ts

Lines 810 to 826 in caa79a7

	if (!ready && this.visible.length === 0) {
	// If its still not ready by 500ms, force it to be.
	setTimeout(() => {
	this.canvas.style.opacity = '1';
	this.firstMeaningfulPaint = true;
	}, 500);
	}
	if (!this.firstMeaningfulPaint && ready && this.visible.length) {
	// Fade in the canvas?
	this.canvas.style.opacity = '1';
	// We've not rendered yet, can we render this frame?
	this.firstMeaningfulPaint = ready;
	// We need to return true here to ensure our update is done.
	return true;
	}

Best as I can tell the cause of this is that when images are loading slowly schedulePaintToCanvas is incrementing imagesLoaded multiple times, meaning that this line can result in a negative value for imagesPending

atlas/src/modules/canvas-renderer/canvas-renderer.ts

Line 173 in caa79a7

this.imagesPending = this.imagesPending - this.imagesLoaded;

Something as simple as checking for this.imagesPending - this.imagesLoaded < 0 or even just a timeout on the opacity update fixes this in terms of setting the canvas visibility. Happy to set up a PR with a small fix like that but feels like it may also be a symptom of an issue elsewhere. So far the easiest way to replicate/test this has been to use in-browser network throttling at a low speed with the current stories in the repo.

[stephenwf]

Ahh because if (!this.imagesPending) will be true if it's negative. I'll have a look at replicating. I'll have a look soon to see if I can figure out when it's incrementing imagesLoaded multiple times.

[danieltbrennan]

@stephenwf I looked a bit wasn't able to make any clear conclusions about the exact cause of the repeated calls but in talking about this with @abrin we had an idea that we might be able to use the image id as a key to effectively dedupe them (or at least prevent the extra incrementing of imagesLoaded).

We're currently actively working on a new canvas panel implementation in one of our apps where this issue is causing some challenges from a UX perspective, so anything we can do to work around it even if as a temporary measure would be a great help.

[stephenwf]

If the fix in #50 is enough to unblock, I can get that merged and tagged for you 👍 alternatively, I can disable the fade-in (again!)