fix(mobile): keep the grid tile's frame above its screen background on iOS - #1655
Conversation
janicduplessis
left a comment
There was a problem hiding this comment.
Fresh review of #1655 (fixes #1651). I did not implement this change. I read the issue, the diff, React Native 0.88.0-rc.1's ViewShadowNode.cpp, RCTViewComponentView.mm and UIView+ComponentViewProtocol.mm, Gesture Handler 3.3.0's RNGestureHandlerButton*.mm, device-zoom.ts, and every Card/Touch user in apps/mobile/src. I did not run the app.
Correctness
The fix is correct.
ViewShadowNode::initializesetsformsStackingContextwhen!viewProps.collapsable, so thescreenView becomes a stacking context and the frameTouchand placeholder mount inside it. Without the prop, the View has onlybackgroundColorand layout props, so it getsFormsViewbut notFormsStackingContext. Its children are hoisted into the Card as siblings after it.RNGestureHandlerButtoninserts_underlayLayerat sublayer index 0 in init (L137) and again in everylayoutSubviews(L394). On iOS the button uses the defaultmountChildComponentView:fromUIView+ComponentViewProtocol.mm, which callsinsertSubview:atIndex:. The ordering the PR describes follows from that. After layout, the Card's layers are[underlay, screen, placeholder, ...]. When the frameTouchreplaces the placeholder at view index 1, it goes to layer index 1, beforescreen, so it sits under the#0C0A11background. This also accounts for the issue's report that a Web tile painted first and then went black: the frame was present when the Touch first mounted, and a later frame -> null -> frame remount put it back one place too early.- Inside the screen View (
RCTViewComponentView), the background goes onlayer.backgroundColorbecause the view has no border and no radius, so there is no extra sublayer to shift indices. The new parent does not have the same problem. - Zoom (
openDeviceViewer):measureInWindowruns on theTouchref, which is still its own host view. Window coordinates don't depend on which ancestor the view is mounted under, and nothing else indevice-zoom.tswalks the hierarchy.useZoomedAwayonly changes theImageopacity. The zoom is not affected. - Android:
collapsable={false}only keeps a View that already rendered. It is harmless there.
Actionable
- Comment wording is inaccurate. The comment says "Fabric flattens a View with only a background into the Card". The View is not flattened: it forms a view, and its children are hoisted into the Card. The PR description gets this right ("keeps it as a view and hoists its children"). Suggested wording: "Fabric hoists the children of a View that has only a background into the Card, ...". Apart from that, the comment fits the comment policy. It names the external constraint (Gesture Handler's index-0 underlay layer plus UIKit's
insertSubview:atIndex:), and the code alone cannot explain it. - The PR description misstates why
DeviceTileescapes. It saysDeviceTileworks "because its screen View hasonLayout". The more basic reason is thatDeviceTile'sCardhas noonPress, soCardrenders a plainView, not a Gesture Handler button.onLayoutdoes make its screen View a stacking context, but the misordering needs a Gesture Handler button parent, and that tile has none. Please correct the sentence so nobody later relies on "add an event handler" as the general fix.
Non-blocking notes
-
Other late-mounted children still land one place early inside Gesture Handler buttons, but I found none that is visible:
- In this tile, the
frame && errorstaleText, thedevice.pageURLTextand the "Page failed to load"Pillare still hoisted into the Card (metaandbadgehave no background). They mount one place early, but nothing they are placed under overlaps them. WorkspaceRow: the chipPills andBuildProgressBarmount late into the rowTouchthrough the flattenedbody/chipsViews. They don't overlap their siblings.Button(loadingswitches theActivityIndicator/Icon) andActionToast(pendingActivityIndicator): a child mounted late at index 0 lands under the underlay layer. That layer has opacity 0 for thecardandopacityfeedbacks, so nothing shows.MacChip's absolutely positioned dot overlaps the icon, but both mount at the same time, so the order is correct. It would break only if the dot were ever mounted conditionally.BuildCards'PlatformCardswitches its header and progress bar insidestyles.platform(padding and gap only, so flattened into the Card). Nothing overlaps.
The risk note in the PR matches what I found. A future absolutely positioned overlay or badge mounted conditionally inside a
Card/Touchwould hit the same bug. - In this tile, the
-
Upstream: I also found no matching Gesture Handler issue (searched
underlay insertSubview,button children order underlayLayer, andbutton child hidden behind background ios). Consider filing one. Its link would then be a valid, shorter replacement for part of the comment under the policy's "direct issue link" rule. -
Tests: No unit test is justified. A Jest render can't observe native z-order, and the before/after screenshots on iOS and Android are the right evidence.
-
Docs: No user-facing guidance changes.
apps/mobile/README.mdandwebsite/docsdon't need updates. -
CI
checkpasses. The branch is one commit with a conventional title.
Description
On iOS, every Devices grid tile showed a black screen while its frames kept arriving: the tile's own
#0C0A11screen background was painted on top of the frame. The fix iscollapsable={false}on the tile'sscreenView, so the frame mounts inside it.Root cause, confirmed in a view hierarchy dump of the running app:
screenView has a background but nothing that makes it a stacking context, so Fabric keeps it as a view and hoists its children into the nearest stacking context (ViewShadowNode.cpp). That is the Card, a Gesture HandlerTouchablesince feat(mobile): use Gesture Handler Touchable for presses #1537.CALayerat sublayer index 0 (init, each layout). UIKit'sinsertSubview:atIndex:counts that layer, so a child mounted after layout lands one place too early. The frame replaces the "Waiting for a frame" placeholder after layout, so it lands under thescreenbackground.DeviceTileon the workspace screen escapes this because itsCardhas noonPress, so it renders a plainViewrather than a Gesture Handler button.Solution
Upstream and risk: the ordering bug is in Gesture Handler and is still on its
main; I found no upstream issue. OtherCard/Touchparents could mis-order late-mounted children the same way, but it only shows when such a child overlaps an earlier sibling, and I saw no other visible case.No unit test: a Jest render cannot observe native z-order. The screenshots are the verification.
lldb check of the UIKit behavior
On the simulator, with a bare
CALayerinserted at index 0 of a view holding subviewa,insertSubview:b atIndex:1produces subviews[b, a].Test plan
With
pnpm run mock-server --port 7851and a dev build paired viapnpm run dev:pair --mock --port 7851, open Devices from the menu.Fixes #1651