fix(ios): honour zPosition when capturing with useRenderInContext (#677) - #696
Open
gre wants to merge 3 commits into
Open
fix(ios): honour zPosition when capturing with useRenderInContext (#677)#696gre wants to merge 3 commits into
gre wants to merge 3 commits into
Conversation
added 3 commits
September 5, 2026 13:25
…ntent Adds a failing E2E reproduction for #677, ahead of any fix. `useRenderInContext` was plumbed but never exercised: before this commit the option appeared only in the README, the TurboModule spec types and the Objective-C++ that reads it — no example screen, no E2E test. That is why the regression went unnoticed. Mechanism: `-[CALayer renderInContext:]` walks `sublayers` in array order and ignores `zPosition`. Under Fabric, `RCTViewComponentView` stops using `layer.backgroundColor` and appends a `_backgroundColorLayer` whenever it cannot express the border through plain CoreAnimation properties (see `useCoreAnimationBorderRendering`), keeping it behind the content solely via `zPosition = -1024`. Appended last, it is painted last through `renderInContext:` — over the content — so the view is captured as a flat block. `drawViewHierarchyInRect` goes through the render server and is unaffected. The new screen renders three cards with identical content, differing only in the style that selects RN's rendering path: no border, `borderWidth: 1`, and `borderWidth: 1` with `overflow: hidden`. The third one separates the appended sublayer branch from the `createMaskLayer` branch, which needs a different fix. Assertions compare the two capture strategies against each other rather than against reference snapshots, so they hold across iOS versions, device scales and font rendering. `example/e2e/helpers/pixels.js` decodes the PNG the library actually wrote — the existing snapshot matcher compares byte sizes of device screenshots, which cannot see this class of bug. The bordered-card tests are expected to fail until the fix lands. The Detox iOS CI step is `continue-on-error`, so this does not turn the build red. Refs #677
`-[CALayer renderInContext:]` paints sublayers in `sublayers` array order and ignores `zPosition`, which the live compositor honours. Fabric depends on that difference: `RCTViewComponentView` appends a `_backgroundColorLayer` (and a `_borderLayer`) whenever a view's border cannot be expressed through plain CoreAnimation properties, and keeps them behind the content only by giving them `zPosition = -1024`. Appended last, they are painted last through `renderInContext:` — over the content — so the view is captured as a flat block. Sorting each `sublayers` array by zPosition before rendering restores what the compositor would have drawn. The sort is stable, so layers sharing a zPosition keep their array order, which is Core Animation's own rule. Only arrays whose order actually differs are touched, and the tree is restored in a `@finally` inside a `CATransaction` with actions disabled, so nothing is ever presented. Scope: the change is confined to the `if (renderInContext)` branch, which no other example screen or E2E test exercises. Measured on iPhone 17 Pro / RN 0.84.1 — before the fix, `border` and `nested-border` captured as flat blocks (uniqueColors = 1) while `plain`, `border-clipped` and `scroll-border` were already correct; after it, all five match `drawViewHierarchyInRect`. Two findings the reproduction settled, both recorded in PLAN-677.md: - `overflow: hidden` is unaffected, because `clipsToBounds` flips `useCoreAnimationBorderRendering` back to true and no extra layers are appended. The mask branch is not involved, so reordering is enough and no manual re-implementation of compositing is needed. - The configuration #677 actually reports — a ScrollView captured with `snapshotContentContainer` whose items carry the border — does NOT reproduce here, with or without this change. What is fixed shares #677's signature without being provably the same instance, so the issue should not be closed on this commit alone. Refs #677
- Skip the new E2E suite on Android. `useRenderInContext` is iOS-only and `result: 'tmpfile'` returns an emulator-side path the host test process cannot read, so `beforeAll` would throw and take all five tests down on `npm run test:e2e:android`. jest testMatch picks the file up for both configurations, so the guard has to be explicit. - Reorder the current sublayers on restore instead of assigning the pre-render snapshot back. `renderInContext:` drives `display`/`drawInContext:`, so a delegate adding or removing a sublayer mid-render would otherwise have its addition dropped or its removal undone, corrupting the live view. - Clamp the region origin from above in `clampRegion`. An out-of-bounds x/y read past the buffer, which turned the statistics into NaN and collapsed `uniqueColors` to 1 — indistinguishable from "flat block", i.e. a false bug signal rather than an error. - Declare `expect` as a devDependency. It only resolved by hoisting out of jest's tree, which breaks silently under a stricter installer. - Flip the HomeScreen entry to `status: 'tested'`. It shipped as `'bug'` next to StyleFilters (#578), which is genuinely still broken; leaving both red made them indistinguishable. - Stop tracking PLAN-677.md. It is a session working document, not a repo deliverable; its useful residue lives in the code comments, the test screen and the PR description. All five cards still match `drawViewHierarchyInRect` after the restore change. Refs #677
There was a problem hiding this comment.
🟡 Changes recommended
The new example/test additions include a dead file reference and a few misleading/inaccurate comments that should be corrected before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes an iOS/Fabric rendering discrepancy in useRenderInContext captures by temporarily reordering CALayer.sublayers to respect zPosition during renderInContext:, and adds an example screen + Detox E2E pixel assertions to exercise this path in the example app.
Changes:
- iOS: sort each layer’s
sublayersbyzPositionbeforerenderInContext:and restore ordering afterward. - Example app: add a dedicated
RenderInContextscreen demonstrating the bordered-view failure mode and exposing capture URIs for tests. - Detox E2E: add pixel-level PNG decoding/diff utilities and a new test that compares
drawViewHierarchyInRectvsrenderInContext:outputs.
File summaries
| File | Description |
|---|---|
| ios/RNViewShot.mm | Sort/restore sublayer order around renderInContext: to honor zPosition. |
| example/src/screens/RenderInContextTestScreen.tsx | New repro/diagnostic screen for useRenderInContext behavior (and test hooks). |
| example/src/screens/HomeScreen.tsx | Adds navigation entry for the new RenderInContext test screen. |
| example/App.tsx | Registers the new RenderInContext screen in the navigation stack. |
| example/e2e/tests/render-in-context.test.js | New Detox test that captures cards in both modes and asserts pixel equivalence. |
| example/e2e/helpers/pixels.js | PNG decode + region statistics + pixel diff helpers (pixelmatch/pngjs). |
| example/package.json | Adds expect, pixelmatch, and pngjs devDependencies for the new E2E assertions. |
| example/package-lock.json | Locks added dependencies. |
Review details
Files not reviewed (1)
- example/package-lock.json: Generated file
Suppressed comments (2)
example/src/screens/RenderInContextTestScreen.tsx:186
- The intro text says “the three cards should look identical”, but this screen renders 5 cards (
plain,border,border-clipped,nested-border,scroll-border). This makes the instructions misleading for manual testing.
Capture both ways: the three cards should look identical in both
columns. Any card that comes back as a blank block is the bug.
example/e2e/tests/render-in-context.test.js:73
- The setup comment says each button captures “all three cards”, but
CARDScontains 5 entries. The second line also refers to “the three cards”.
// Capture through both strategies. Each button captures all three cards.
// The buttons sit below the three cards, so they start off-screen.
- Files reviewed: 7/8 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+84
to
+90
| NSMutableArray<CALayer *> *restored = [NSMutableArray arrayWithCapacity:current.count]; | ||
| for (CALayer *sublayer in original) { | ||
| if ([current containsObject:sublayer]) [restored addObject:sublayer]; | ||
| } | ||
| for (CALayer *sublayer in current) { | ||
| if (![original containsObject:sublayer]) [restored addObject:sublayer]; | ||
| } |
Comment on lines
+16
to
+18
| * That last line matters: this file reproduces a real bug with #677's exact | ||
| * signature, but not #677's exact setup. See PLAN-677.md for what that leaves | ||
| * open. |
Comment on lines
+108
to
+112
| * The reporter captures a vertical ScrollView with `snapshotContentContainer`, | ||
| * whose items carry the border. The minimal `border` card above shows the bug | ||
| * does not need any of that — but this card reproduces #677 as filed, so the | ||
| * fix is verified against the configuration actually reported and not only | ||
| * against our reduction of it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR — help wanted on one specific point
This fixes a real, measured iOS capture bug and adds the first test coverage
useRenderInContexthas ever had. But it does not reproduce #677 as filed, and I would like a second opinion on whether it actually solves the reporter's problem.The bug
-[CALayer renderInContext:]paints sublayers insublayersarray order and ignoreszPosition, which the live compositor honours.Fabric depends on that difference. In
RCTViewComponentView.mm,useCoreAnimationBorderRenderingis false when a border is present, opaque, and the view does not clip:In that case RN stops using
layer.backgroundColorand appends sublayers, keeping them behind the content solely viazPosition:Appended last, it is painted last through
renderInContext:— over the content. The view is captured as a flat white block.The fix
Sort each
sublayersarray byzPositionbefore rendering, restore afterwards. The sort is stable, so layers sharing a zPosition keep array order — Core Animation's own rule. Only arrays whose order actually differs are touched; restore happens in a@finallyinside aCATransactionwith actions disabled, so no reordered frame is ever presented.The change is confined to the
if (renderInContext)branch. On the old architecture it is a no-op:RCTView.m'sreactZIndexSortedSubviewsalready keeps array order aligned withzPosition.Measured results
iPhone 17 Pro, RN 0.84.1, pixel-level comparison of the actual captured PNGs:
plain— no borderborder—borderWidth: 1uniqueColors = 1, flat blockborder-clipped—+ overflow: hiddennested-border— border on a child of the capture rootscroll-border— the reporter's exact setupVerified by actually reverting the native change and rebuilding, not by reasoning.
The configuration #677 reports does not reproduce. A vertical
ScrollViewcaptured withsnapshotContentContainer: true, whose items carryborderWidth: 1— exactly what the reporter describes — renders identically in both modes, with and without this change.So this PR fixes a bug with #677's signature without being provably the same instance of it.
What I could not explain: why
nested-borderbreaks (border on a descendant of the capture root) whilescroll-borderdoes not. One unverified lead —snapshotContentContainerresizesscrollView.frame, forcing a layout pass, which might remount content layers after the background layer and fix the order by accident. If that is right, the bug is mount-order dependent and therefore intermittent, which would explain why only some of the reporter's items lose their content.Questions for reviewers:
snapshotContentContainer?zPosition-aware render?I would not close #677 on this PR alone. The reporter's minimal repro — which I had declared unnecessary — is worth asking for after all.
Also in here
useRenderInContextwas documented and plumbed but exercised nowhere — no example screen, no E2E test. That is why this went unnoticed. This adds both.example/e2e/helpers/pixels.jsdecodes the PNG the library actually wrote. The existingsnapshot-matcher.jscompares byte sizes ofdevice.takeScreenshot()output, which answers a different question and cannot see this class of bug.Assertions compare the two capture strategies against each other rather than against reference snapshots, so they hold across iOS versions, device scales and font rendering.
Not addressed
The "shadows make capture slow" part of #677 is a separate, upstream issue:
snapshotContentContainerresizing the ScrollView frame forces a synchronous layout of every item, each regenerating its box-shadow layer image — O(N) in RN, not in this library.🤖 Generated with Claude Code
https://claude.ai/code/session_01KPH1Rc1m1tVDWTytveAa4T