fix(charts): keep the skia canvas usable when React re-runs Canvas effects on web - #100714
dariusz-biela wants to merge 5 commits into
Conversation
|
|
WebGLRenderer.dispose() loses the canvas's WebGL context on every layout-effect cleanup, but an <Activity> hide (and StrictMode's DEV double-invoke) re-runs the effects on the same element, where a lost context is permanent - the reveal can never create a surface again and the hidden backdrop shows a white block where the chart was. Patch skia to defer the release by one microtask and lose the context only once the canvas has really left the document, mirroring upstream PR Shopify/react-native-skia#4027 (issue Expensify#3976): a hidden screen keeps its context and its last presented frame, the reveal reuses the live context like the resize path always has, and a real unmount still releases the context slot deterministically.
4249cad to
09bf361
Compare
…m fix Shorten the in-patch comment to three lines, drop the dead null guard on the canvas, and condense the details.md reason. Upstream Expensify#4027 was closed in favor of Expensify#4002, which merged on 2026-09-02 and is not in any release yet; record that, and that its canvas size reset would blank a hidden Activity backdrop, so a Skia bump does not fully replace this patch.
…ssion Before patch 004 every unmount released the WebGL context synchronously, background screens included, so the leak of a screen unmounted while hidden does not match the pre-Activity baseline. Say so instead of claiming parity.
|
|
||
| dispose() now defers the release by one microtask and only loses the | ||
| context when the canvas has left the document. React runs cleanup before | ||
| detaching the host node, so isConnected is false one microtask later only |
There was a problem hiding this comment.
If I'm reading the PR and React's commit order right, this one line looks off: "React runs cleanup before detaching the host node". That's true foruseLayoutEffect and it is what upstream #4002 does, but in 2.4.18 dispose() runs from the passive useEffect cleanup, which React flushes after it removes the node.
So on this version isConnected alone already separates a real unmount from an Activity hide and the queueMicrotask is carrying forward a guarantee we'll need again once we bump past #4002, where the caller is a layout effect.
I think it is worth rewording, so nobody later measures isConnected, sees it's already false and drops the deferral as dead code.
There was a problem hiding this comment.
Good catch, you're right: in 2.4.18 dispose() runs from a passive useEffect cleanup, so isConnected is already false at cleanup time on a real unmount. Dropped the microtask and made the check synchronous in e85fcca, details.md now explains why upstream needs the deferral and this version does not.
In 2.4.18 dispose() runs from a passive useEffect cleanup, which React flushes after removing the host node, so the canvas is already detached on a real unmount and still connected across an Activity hide. The microtask only matters for a layout-effect caller like upstream's; drop it and say why in details.md.
…ecisely Upstream Expensify#4002 skips the same eager context release, but its dispose() already frees the surface, the GrContext, the CanvasKit handle and the drawing buffer at every hide, so only the context slot survives there. Saying it has "the same gap" understated this patch's leak.
cc5a486 to
a1d710e
Compare
Reviewer Checklist
Screenshots/VideosAndroid: mWeb ChromeScreen.Recording.2026-09-14.at.17.35.33.moviOS: mWeb SafariScreen.Recording.2026-09-14.at.17.35.51.movMacOS: Chrome / SafariScreen.Recording.2026-09-14.at.17.22.37.mov |
Explanation of Change
Adds patch 004 to
@shopify/react-native-skiaso a<Canvas>on web survives React re-running its layout effects on the same DOM node. TodayWebGLRenderer.dispose()loses the canvas's WebGL context on every effect cleanup, and a lost context is permanent for that element, so the re-run can never create a surface again and the chart stays blank. This is a prerequisite for hiding chart screens with React<Activity>(theScreenActivityWrapperrollout, #98254) and it also fixes the same failure under StrictMode's dev double-invoke.In the app
nonTopScreenBehavior: 'activity'yet, and StrictMode is a dev-only tool.USE_REACT_STRICT_MODE_IN_DEVon, Reports charts (bar, line, pie) render instead of falling back to "Unable to display chart".In the code
patches/@shopify/react-native-skia/@shopify+react-native-skia+2.4.18+004+defer-webgl-context-loss-to-unmount.patch:dispose()now loses the context only when the canvas has already left the document, which is the real-unmount case. In 2.4.18 it runs from a passive effect cleanup, which React flushes after detaching the host node, so a plainisConnectedcheck is enough and a real unmount still releases its context deterministically (browsers cap a page at ~16 live contexts). An Activity hide or a StrictMode re-run keeps the element connected, so the new renderer reuses the live context like the resize path already does.dispose()runs from a layout effect cleanup, before the node is detached. Upstream also zeroes the canvas size on cleanup, which would blank a hidden Activity screen's backdrop, so a Skia bump does not fully replace this patch.details.mdrecords that.Known regression
dispose()frees the surface, theGrContext, the CanvasKit context handle and the drawing buffer at every hide, so only the context slot survives there. Freeing the buffer is the canvas size reset that would blank a hidden screen's backdrop, which is why this patch does not copy it.Fixed Issues
$ #98254
PROPOSAL: N/A
Tests
Web (Chrome) only, the patch touches
SkiaPictureView.web.js.src/CONFIG.tssetUSE_REACT_STRICT_MODE_IN_DEVtotrueand start the web app (npm run web).rangeMinor WebGL error appears in the JS console.Offline tests
N/A - the patch changes how a web
<canvas>releases its WebGL context, no network or Onyx code is touched.QA Steps
Web (Chrome and Safari) and mWeb, the patch only touches the web renderer.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web QA Steps
1-qa-steps-charts.mp4
Before fix behavior with Activity
2-main-activity-hide-breaks-chart.mp4
After fix behavior with Activity
3-fixed-activity-hide-keeps-chart.mp4