tradingview
Race condition on browser load when resolving `getRgbStringViaBrowser()`
Lightweight Charts™ Version: 5.0.9
This is a rather rare race condition on browser load when translating colours via getRgbStringViaBrowser(). I've only observed this during plugin development because there's nothing else really to load the page, and so LWC loads in very quickly. I don't think this is a particularly important issue to resolve - just a bit of an oddity when developing.
Steps/code to reproduce:
Replicating with LWC is tricky (adding a createPriceLine() to a series may occasionally work in a new plugin). Otherwise, the easiest way is to just use raw JS in a HTML page:
<!DOCTYPE html>
<html lang="en">
<head></head>
<body>
<script>
function getRgbStringViaBrowser(color) {
const element = document.createElement('div');
element.style.display = 'none';
// We append to the body as it is the most reliable way to get a color reading
// appending to the chart container or similar element can result in the following
// getComputedStyle returning empty strings on each check.
document.body.appendChild(element);
element.style.color = color;
const computed = window.getComputedStyle(element).color;
console.log(`${color} => ${computed}`);
document.body.removeChild(element);
return computed;
}
getRgbStringViaBrowser('red');
setTimeout(() => {
getRgbStringViaBrowser('red');
}, 100);
</script>
</body>
</html>
Actual behavior:
All colours are converted to some browser default colour because it hasn't yet initialised all page state yet.
Expected behavior:
Colours to be properly converted.
Screenshots:
See the grey price backgrounds (even though price lines are different colours with no axisLabelColor specified, so the axis labels should fall back to the price line colour):
A simple 'fix' would be to just not cache the colour conversion if the browser is not ready yet. Wrong colours will be returned initially still, but repainting will probably happen soon after. I'm not sure what a better approach would be.
diff --git a/src/model/colors.ts b/src/model/colors.ts
index 315959994..7b18dbf2f 100644
--- a/src/model/colors.ts
+++ b/src/model/colors.ts
@@ -187,7 +187,9 @@ export class ColorParser {
(match[4] ? parseFloat(match[4]) : 1) as AlphaComponent,
];
- this._rgbaCache.set(color, rgba);
+ if (document.readyState === 'complete') {
+ this._rgbaCache.set(color, rgba);
+ }
return rgba;
}
I think complete is the only state that is safe, judging from the following:
<!DOCTYPE html>
<html lang="en">
<head></head>
<body>
<script>
function getRgbStringViaBrowser(color) {
const element = document.createElement('div');
element.style.display = 'none';
// We append to the body as it is the most reliable way to get a color reading
// appending to the chart container or similar element can result in the following
// getComputedStyle returning empty strings on each check.
document.body.appendChild(element);
element.style.color = color;
const computed = window.getComputedStyle(element).color;
console.log(`${document.readyState}: ${color} => ${computed}`);
document.body.removeChild(element);
return computed;
}
getRgbStringViaBrowser('red');
let i = 0;
const interval = setInterval(() => {
getRgbStringViaBrowser('red');
if (i++ > 100) {
clearInterval(interval);
}
}, 1);
</script>
</body>
</html>
This is interesting. 🤔 I'm unable to reproduce the issue myself but it is likely because I'm just testing with the very small code sample you provided.
Do you notice error when your project is compiled (or is it just during development with Hot Module Reloading)?
Any specific version of Chrome that you are testing on?
I think adding the gate for the cache (check that document.readyState === 'complete') might be an issue in some cases. Depending on the page it could take a long time for this state to be reached. So we might also need a timeout, maybe after 50ms we consider that it is safe to cache even if the readyState isn't 'complete'.
Ah, I see. The code example is using the generated index.html file of a new plugin, where Vite is used to serve it. So this could just be a development environment issue (not specific to hot reloading, I don't think). Simply saving the example as a html file and opening it directly in a web browser does not reproduce the problem.
I don't notice any errors when compiling.
Chrome version: Version 141.0.7390.54 (Official Build) (arm64)
True. I guess if the page takes that long to load though, then an uncached getRgbStringViaBrowser() probably won't make much difference 🙂 Since this is probably a dev environment issue only, we could also just ignore this edge case.
I think the problem with having a timeout is that correctness will be sacrificed for attempting to improve loading speeds. It is unclear what additional overhead an uncached getRgbStringViaBrowser() will actually incur relative to the total page loading (can it be that much?). If readyState !== 'complete', then it is more than likely any computed values are incorrect, where caching them will mean they will remain incorrect for the rest of the app's lifecycle.
I guess at the top of _parseColor(), we could add something like:
if (document.readyState !== 'complete') {
return [232, 230, 227, 1] as Rgba;
}
Or whatever the default we would like. That way, we don't waste any resources trying to convert until the browser is able to do so properly (which will also avoid caching incorrect things).