Repository navigation
docs(react-component-runtime): read layout as little as possible - #10219
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec 9f4de37 |
github-actions
Bot
removed
the
needs:spec-owner-review
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested: INV18 is approved for this head, but as written it contradicts current records that this pull request does not amend.
- The ban on layout reads in scroll and resize paths (
docs/architecture/react-component-runtime.mdline 366) contradictsarchitecture:layer-runtime, which re-reads open layers from ResizeObserver, resize and intersection callbacks, and contradicts INV11's keyboard read on visual-viewport resize and scroll, which INV18 cites as an instance; narrow the ban to the reads those clauses allow, or amend those clauses here. - The limit of one read per change (line 367 and the failure signal on line 529) contradicts INV12 and
component:ResizableFR8, which measure before and after each change, and INV11's docked-bar read; allow the before-and-after pair that a current clause requires, or name those clauses as exceptions. - The verification row (line 529) checks only
getBoundingClientRect,offset*andclient*reads, so thescrollTopandscrollHeightreads in INV18's own gap example, andinnerHeightandgetComputedStylereads, pass review; list every read that forces layout.
Non-blocking: the CSS-first clause restates architecture:component-style-authoring INV2, which this record's guidance names as the owner of that choice. Draft #10126 also adds an INV17 and an INV18 to this record, so one of the two pull requests will need to renumber.
[Automated review]
| `MutationObserver` beside `sharedMutationObserver`.) | ||
| - **INV18 — Layout is read as little as possible.** Placement prefers CSS | ||
| (anchor positioning, container queries, logical properties) over measuring in | ||
| JS; a per-frame, scroll or resize path reads no layout; a remaining layout |
There was a problem hiding this comment.
This ban contradicts current clauses that this pull request leaves unamended. architecture:layer-runtime re-reads an open layer's cover and overflow from ResizeObserver callbacks and resize events, and follows a scrolled anchor frame by frame from intersection reports. INV11's keyboard hook, which the next lines cite as an instance, makes its one read in visual-viewport resize and scroll handlers. Narrow the ban, for example to one batched read before paint where a current clause requires it, or amend those clauses here.
| - **INV18 — Layout is read as little as possible.** Placement prefers CSS | ||
| (anchor positioning, container queries, logical properties) over measuring in | ||
| JS; a per-frame, scroll or resize path reads no layout; a remaining layout | ||
| read names its reason in a comment and reads once per change, not per frame. |
There was a problem hiding this comment.
"Once per change" and the failure signal "more than one read per change" fail INV12, which "measures the layout before and after a reported change", and component:Resizable FR8, which measures the pane before and after the collapse commits. They also fail INV11's docked-bar read, its stated exception. Define a read as one measurement pass and allow the before-and-after pair a current clause requires, or name these as exceptions.
| | INV15 | Unit tests of the animator's value mode and real-browser proofs of its adopters | A value off its channels' timing (the theme's tokens for a program-started change, a spring only where `spec:AST-094` FR45 keeps one), a count past the component's swap size, a count or cross-fade under reduced motion with a coarse or a fine pointer, an intermediate value announced, or a public export fails | | ||
| | INV16 | Review of each Core component that handles a concern Core owns in a shared hook or utility | A Core component that keeps its own effect, listener, or tracker for that concern fails, whether or not it also calls the hook or utility | | ||
| | INV16 (slot presence) | `no-nullish-jsx-guard` strict lint over Core and RichText, with no baseline, and `no-truthy-jsx-guard` strict lint over Core, with no baseline; both catch inline guards only, so review also checks for a presence test hoisted into a variable, converted (`!!value`, `Boolean(value)`), made by an early return or a `??` default, or passed through a prop spread, which the lints pass; empty-slot tests that pass `''`, `false`, `true`, and `null` and compare with an omitted slot, plus `0` as content, in Item, MetadataList, ChatSystemMessage, TreeList, TopNavMegaMenu, RichTextEditor, and RichTextEditorToolbar | A slot given `''`, `true`, or `false` draws a wrapper, lane, border, or separator that an omitted slot does not, an element loses the name it has without the slot, `0` does not render, or a new bare nullish or truthiness presence test, inline or hoisted, a truthiness guard past the baseline, a baseline entry added, or a hand-written copy of the check lands | | ||
| | INV18 | Review of added `getBoundingClientRect`, `offset*` and `client*` reads (evidence gap: no automated finding yet) | A layout read in a per-frame, scroll or resize path, a layout read without a comment that names its reason, more than one read per change, or JavaScript measurement where CSS placement serves | |
There was a problem hiding this comment.
This evidence checks only getBoundingClientRect, offset* and client* reads. The gap INV18 names reads scrollTop and scrollHeight, the keyboard read uses innerHeight and the visual viewport, and other Core sites use getComputedStyle, so a new scroll listener that reads scrollTop passes this review. List every read that forces layout, at least scroll*, inner*, viewport geometry, getComputedStyle and getClientRects.
PR Analysis Report🔎 Preview ·
|
…clauses require, as one batched pass
|
/approve-spec 6b07b80 |
github-actions
Bot
added
needs:spec-owner-review
There was a problem hiding this comment.
Approved pending CI: the heavy CI suites have not run on this head yet; the required build, test and docsite-test checks report them deferred until the pre-merge run. INV18 is approved by an engineering owner for this head, and it now exempts the layout reads that architecture:layer-runtime, INV11, INV12 and component:Resizable FR8 require, which resolves the three earlier conflicts.
Non-blocking suggestions:
- The failure signal in the INV18 verification row fails any read interleaved with writes, while INV18 itself limits that rule to reads in per-frame, scroll and resize paths; align the two.
- Add
elementFromPointandelementsFromPoint, which force layout and which Core calls on pointer paths, to the verification row's list of reads. - Name INV11's docked-bar read beside its keyboard read, since INV11 calls it the one exception to the hook's single read per keyboard change.
- Update the description's Decision and verification bullets, which still describe the first commit; draft #10126 also adds an INV18 to this record, so one of the two will need to renumber.
[Automated review]
| | INV15 | Unit tests of the animator's value mode and real-browser proofs of its adopters | A value off its channels' timing (the theme's tokens for a program-started change, a spring only where `spec:AST-094` FR45 keeps one), a count past the component's swap size, a count or cross-fade under reduced motion with a coarse or a fine pointer, an intermediate value announced, or a public export fails | | ||
| | INV16 | Review of each Core component that handles a concern Core owns in a shared hook or utility | A Core component that keeps its own effect, listener, or tracker for that concern fails, whether or not it also calls the hook or utility | | ||
| | INV16 (slot presence) | `no-nullish-jsx-guard` strict lint over Core and RichText, with no baseline, and `no-truthy-jsx-guard` strict lint over Core, with no baseline; both catch inline guards only, so review also checks for a presence test hoisted into a variable, converted (`!!value`, `Boolean(value)`), made by an early return or a `??` default, or passed through a prop spread, which the lints pass; empty-slot tests that pass `''`, `false`, `true`, and `null` and compare with an omitted slot, plus `0` as content, in Item, MetadataList, ChatSystemMessage, TreeList, TopNavMegaMenu, RichTextEditor, and RichTextEditorToolbar | A slot given `''`, `true`, or `false` draws a wrapper, lane, border, or separator that an omitted slot does not, an element loses the name it has without the slot, `0` does not render, or a new bare nullish or truthiness presence test, inline or hoisted, a truthiness guard past the baseline, a baseline entry added, or a hand-written copy of the check lands | | ||
| | INV18 | Review of every added read that forces layout: `getBoundingClientRect`, `getClientRects`, `offset*`, `client*`, `scroll*` (`scrollTop`, `scrollHeight`, and the rest), `inner*` and `outer*`, `visualViewport` geometry, and `getComputedStyle` (evidence gap: no automated finding yet) | A layout read in a per-frame, scroll or resize path that no current clause requires there, a read interleaved with writes, a read without a comment that names its reason, more than one measurement pass per change beyond the before-and-after pair a current clause requires, or JavaScript measurement where CSS placement serves | |
There was a problem hiding this comment.
Non-blocking: two gaps between this row and the clause. The failure signal fails "a read interleaved with writes" anywhere, while INV18 states "never interleaved with writes" only for the reads it allows in per-frame, scroll and resize paths; scope the signal the same way, or say in INV18 that every read is batched before writes. The list also leaves out elementFromPoint and elementsFromPoint, hit tests that force layout and that Core calls on pointer paths (the gesture host, menu press and Slider).
Exact decision needed
Whether
architecture:react-component-runtimestates a shared rule for reading layout: when Core may measure in JavaScript, and how often.Existing authority searched
architecture:react-component-runtime: INV11 already limits the keyboard hook to one layout read per keyboard change. INV12 governs size changes inside a container. Authoring guidance prefers CSS for presentational responsiveness. No invariant covers layout reads in general.architecture:component-style-authoringowns which CSS mechanism replaces JavaScript observation. This invariant does not decide that; it states when a layout read is allowed.Decision
getBoundingClientRectare not yet checked against the rule.getBoundingClientRect,offset*andclient*reads (evidence gap: no automated finding yet).Rejected alternatives
None.
Claim scope and non-goals
Only INV18, its trigger entry and its verification row. No code changes; the listed sites stay recorded gaps.
Evidence and implementation relationship
Current source: the chat stream scroll hook's scroll handler reads
scrollHeight,scrollTopandoffsetHeight; a source count finds about 49 Core files that callgetBoundingClientRect. No implementation PR.Writing rubric results
Scope
Validation
pnpm check:knowledgegit diff --check