perf(navigation): add the per-screen Activity behavior for covered screens - #98575
Draft
dariusz-biela wants to merge 24 commits into
Draft
perf(navigation): add the per-screen Activity behavior for covered screens#98575dariusz-biela wants to merge 24 commits into
dariusz-biela wants to merge 24 commits into
Conversation
Replace react-freeze with React <Activity> for covered (non-top) screens. A hidden Activity keeps its state, cleans up its effects and processes updates at background priority. - nonTopScreenBehavior: 'none' | 'freeze' | 'activity' replaces the freezeNonTopScreens navigator flag, so freeze stays available as a fallback and a navigator or a single screen picks its own behavior. - useScreenActivityMode hides a screen when it is covered or its navigator chain loses focus. Hiding is immediate, revealing waits for the navigation transition to end. - CustomViewWrapper neutralizes the display none a hidden Activity commits, so the navigator's card visibility decides what is on screen. - Rolled out to modal stacks, RightModal, SearchFullscreen, Workspace and Split navigators. The root stack keeps its current behavior.
- Add deprioritize and deprioritized to the project dictionary, so the spellcheck job passes. - Merge ScreenActivityWrapper/index.native.tsx into index.tsx. The two files were identical apart from their comments, because CustomViewWrapper already resolves per platform. - Share one NonTopScreenWrapperProps between the wrapper and the descriptor mapping instead of declaring the same shape twice. - Correct the isScreenBlurred doc. A covered screen often stays visible, which is the whole point of CustomViewWrapper. - Write display contents on the element even where MutationObserver is missing, and key the guard on the element so a new node is served too. - Say in useIsScreenCovered where each half of the covered state comes from, and note that the wrapper reads the navigator's focus. - Split hyphenated clauses into sentences, and state that the native view config was validated on Fabric.
A hidden Activity unmounts the effects of its subtree, so useRHPWidth deregistered its route whenever the screen was covered and the RHP container snapped back to the single width. - useRHPWidth deregisters only when the route has actually left the navigation state, so a hide leaves the registration in place. - WideRHPContextProvider keeps the registered routes and deregisters them from a navigation state listener, which covers a route closed while its screen was hidden and never runs its cleanup again. - extractPresentNavigationKeys counts preloaded routes as present, so a preloaded route is not mistaken for a closed one.
…reen-activity-core
React hides a covered screen with an inline 'display: none !important' in the mutation phase, and layout effects force layout later in the same commit. The MutationObserver only ran on the microtask checkpoint after that commit, so the value really landed: the layout tree of the whole covered screen was torn down and built again on the way back. In the tab-inbox-to-workspaces canary that cost 4.0ms of layout and 6.9ms of style recalculation per pass against 0.3ms and 0.8ms on main. Patching the element's own style declaration to ignore every write to 'display' removes both passes. The observer stays as the fallback for a React version that writes the style attribute as a whole, and normally never fires. Canary, 5 iterations of tab-inbox-to-workspaces on one dev server session: layout 4.01ms -> 0.30ms (main 0.30ms), recalc style 6.91ms -> 2.43ms (main 0.76ms), layout passes 8 -> 5, style passes 11 -> 8.
Fabric never received the hiding write in the first place, because the view config resolves display to 'contents' before the payload leaves JS (ReactFabric-dev.js cloneHiddenInstance). What it did receive was a different value on every toggle: the visible wrapper had no display at all, so Yoga read the default flex, and the hidden one read contents. That difference is not free. YogaLayoutableShadowNode::updateYogaProps dirties a node only when its Yoga style really changed, so every hide and reveal dirtied the wrapper and relaid out its subtree, and the same value flips ForceFlattenView, so the native view was destroyed and created again on each toggle. Pinning the value both states share leaves nothing to diff. The wrapper is now always flattened, which is also one native view less per screen, and it matches the web wrapper, which is display contents from its first render.
Shorten every comment introduced here to the information that is not obvious from the code, and drop the navigator level notes that described the rollout as a demonstration, which is no longer what this branch does.
useIsScreenCovered carried a single expression, so it moves into the hook that consumed it. useScreenActivityMode becomes useScreenActivityState and returns the pair the wrapper needs, the Activity mode and the covered flag its accessibility state follows. Drops the per screen Activity log along with routeKey and routeName, which existed only to feed it, and the comment on getCommonNavigationOptions that described nonTopScreenBehavior from a file that knows nothing about it.
The hook has nothing navigator specific in it, so it does not belong in the ScreenActivityWrapper folder. Its doc comment now says how it differs from useIsResizing, which answers a similar question for tooltips on web only.
Rotating a device always changes the width, so the orientation check never fired on its own. Dropping it removes the isPortrait helper and the second tracked value. The height stays out of the filter because the soft keyboard resizes the window on Android and on mobile web.
Reverts the move to src/hooks and the width only filter. The store keeps the orientation check, and its header now says why the flag lives in a module store rather than in each screen.
The rollout in Expensify#98254 activates Activity screen by screen, so no navigator opts its screens in wholesale anymore. Split keeps freezing its covered screens through the option, which preserves the behavior it had on main.
The fix only matters while an RHP screen can be hidden by Activity, and no screen opts in anymore. The RHP migration PR is where it belongs.
The double effect mount in dev exercises the same cleanup and re-run lifecycle as a hide and reveal cycle, so a screen has to run clean under StrictMode before it can migrate to Activity. Production React builds render StrictMode as a no-op.
Covers the descriptor wrapping decisions, the Activity mode state machine (first render, cover, deferred reveal, window size changes) and the shared window size change store.
React only double-invokes effects for content that mounts inside an already committed StrictMode fiber, because the dev traversal driving the cycle stops at the first newly placed fiber. A StrictMode mounting together with the screen therefore double-rendered but never re-ran the effects, so the qualification gate missed the very lifecycle it exists for. StrictModeMountGate defers the children by one commit in dev, and the USE_ACTIVITY_SCREEN_STRICT_MODE_IN_DEV flag turns the gate off locally to keep double renders out of performance measurements.
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
1 participant
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.
Explanation of Change
This PR ships the core mechanism of the accepted proposal in #98254: a per-screen navigation option that decides what happens to a screen while another screen covers it, with React
<Activity>as the new behavior andStrictModeas the qualification gate for every screen that will migrate to it. The mechanism is identical to the one evaluated at full scale in the demo PR #97990, but here it ships disabled, so screens can opt in one by one as the rollout progresses.In the app
In the code
nonTopScreenBehavior: 'none' | 'freeze' | 'activity'replaces thefreezeNonTopScreensnavigator flag. It is a normal navigation option, so a navigator can set it for all of its screens and a single screen can override it. The split navigators select'freeze', which preserves their current behavior; everything else stays at'none'.wrapDescriptorsWithNonTopScreensBehaviorreplaceswrapDescriptorsWithFreezeand picks the wrapper per screen from that option. Persistent screens (web sidebars) are never wrapped.ScreenActivityWrapperrenders a screen inside<Activity>and decides its mode: hiding is immediate, revealing waits for the navigation transition to end, and the screen stays visible for its first render and for the duration of a window width change.CustomViewWrapperkeeps the hidden content painted, inert and out of the accessibility tree on both platforms.'activity'are additionally wrapped inStrictMode, whose double effect mount in dev exercises the same cleanup and re-run lifecycle as a hide and reveal cycle. This is the qualification gate from the rollout plan, documented incontributingGuides/STRICT_MODE.md. Production React builds renderStrictModeas a no-op.Deliberately out of scope
StrictMode.Fixed Issues
$ #98254
PROPOSAL: #98254
Tests
Offline tests
N/A - this PR only changes how covered screens are wrapped for rendering and makes no API, Onyx or network changes.
QA Steps
Same as tests.
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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari