Repository navigation
spec(architecture): components are built from minimal DOM (INV7) - #8300
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.
|
|
/approve-spec 88e37b6 |
PR Analysis Report
|
cixzhang
changed the title
github-actions
Bot
added
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.
Approval of this spec change waits on an engineering owner's decision: the earlier /approve-spec named the previous commit and INV7 changed after it, so an engineering owner needs to approve architecture:component-style-authoring INV7 and the architecture:component-theming-surface INV3 amendment as written, or have them amended first, and records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so if they are an engineering owner). Independently of that decision, a few fixes to the INV7 text are needed:
- The surface budget and the INV7 failure signal flag every element beyond a component's roles, including wrappers INV7's own first sentence allows, such as Switch's row with its ref and press handlers.
- The surface budget leaves no element for a themable decorative part, while
component-theming-surfaceINV4 keeps targets on stable visible elements, so Switch's releasedastryx-switch-thumbtarget would have to go. - The layer rule allows a second copy only after a close, so the touch cross-fade that
design:reduced-motionDR2 requires for a presentation switch would fail it, the BottomSheetSwitcher handoff is left undecided, and "the shared exit mechanism" is not defined anywhere. - Exceptions belong in a component's record, which Button, IconButton, ToggleButton, TextInput and RadioList don't have, and the SegmentedControl "Good" example describes width stability the component doesn't have today.
[Automated review]
| - **Surface components at rest** — Button, IconButton, ToggleButton, Item and | ||
| other list rows, SegmentedControl, Switch, Badge, text and other inputs, | ||
| CheckboxInput, and radio controls: no element beyond their roles, and no | ||
| duplicated content, under any pointer. A pointer-specific presentation is | ||
| a change of attributes and styles on the same elements; a pseudo-element | ||
| stands in for an element only as decoration. |
There was a problem hiding this comment.
"No element beyond their roles" leaves no room for the INV5 wrappers INV7's first sentence allows (Switch's row owns a ref and press handlers), and no element for a decorative part that carries a public theme target, which component-theming-surface INV4 places on "a stable visible element that paints". Could the budget name INV5 wrappers and target-bearing painted parts explicitly, one way or the other?
| | INV4 | Nested unrelated-owner browser fixture across hover, focus-within, active, and disabled states | Outer component state activates an inner component's visual treatment | | ||
| | INV5 | Wrapper mutations in default and host-composed render paths | Removing a wrapper changes no semantic, behavior/ref, formatting-context, outside-border-spacing, or required style-bearing DOM boundary | | ||
| | INV6 | Pull-request size report against base, with value-table import fixtures | A changed component with no base-against-head gzip JavaScript or CSS figure; a touch-only feature over 2 KB gzip JavaScript or 1 KB gzip CSS per component that is not flagged; importing a component pulls in value-table rows only other components read | | ||
| | INV7 | Story element counts (`document.body.querySelectorAll('*').length`) at rest and open under a fine and a coarse pointer, base against head, plus render inspection for duplicated content | An element beyond the component's roles or a duplicated or hidden twin of content; a state or variant rendered as an extra element; a pseudo-element repeating content; a layer with two presentations mounted, content mounted twice, more elements under a coarse pointer than a fine one, or an exit copy past 1 second; an added element with no measured counts, or a budget exceeded with no recorded owner exception | |
There was a problem hiding this comment.
"An element beyond the component's roles" fails every INV5-justified wrapper in every component, while INV7 itself allows "a wrapper only where INV5 names its responsibility". Adding "other than an INV5 wrapper" here would make the failure signal match the rule.
| CSS can size and reserve it on the element that shows it (INV2). A layer | ||
| mounts exactly one presentation, and the only second copy is one transient | ||
| exit copy made by the shared exit mechanism and removed within 1 second of | ||
| the close. A change to a component prefers removing or merging nodes; any |
There was a problem hiding this comment.
The only second copy allowed is an exit copy "of the close", but design:reduced-motion DR1–DR2 make a switch between presentations on a coarse pointer a cross-fade with both presentations mounted for up to 200 ms, and BottomSheetSwitcher PR1 presents two sheet dialogs during a handoff for a theme-controlled time. No record names "the shared exit mechanism" either. Naming the permitted transient states and the mechanism would settle these.
| A component may exceed the budgets only through an exception its pull | ||
| request states, with why existing elements cannot do the job and the | ||
| measured element count before and after. Each exception is recorded as its | ||
| own owner decision in the component's record; none is accepted silently. |
There was a problem hiding this comment.
Exceptions come only from a pull request and live "in the component's record", so released structure that already exceeds the budgets (Switch, DateInput's pointer-specific trees) has no route, and Button, IconButton, ToggleButton, TextInput and RadioList have no record to hold one.
| Good: SegmentedControl keeps a segment's width stable when its selected | ||
| label changes weight through CSS on the one label element; Popover mounts its | ||
| surface once and changes placement and presentation through styles. Bad: a |
There was a problem hiding this comment.
SegmentedControl does not keep a segment's width stable today: the selected label goes from weight 500 to 600 with nothing reserving width, so in the Default story List widens from 47.45 to 48.27 px when selected. Stating this as a pattern, rather than as SegmentedControl's behavior, would keep the example accurate.
There was a problem hiding this comment.
Approval of this spec change waits on an engineering owner's decision: architecture:component-style-authoring INV7 and the architecture:component-theming-surface INV3 amendment add new requirements, and the only /approve-spec names the first commit, so an engineering owner needs to approve them as written at the current head, or have them amended first, and records that by commenting /approve-spec <full head SHA> (the author may do so as an engineering owner). Independently of that decision, a few fixes to the text are needed:
- INV3 now says desktop and touch rows are never mounted together, which fails the presentation-switch cross-fade that INV7 itself allows and
design:reduced-motionDR2 requires. - INV7 requires the transient copy to come from one Core mechanism every layer uses, but no record or module provides one, so BottomSheetSwitcher's handoff fails INV7 from merge.
- Exceptions recorded in a draft or newly created component record are not owner decisions, since a draft record is not a rule and the spec-owner gate does not cover changes to it.
- INV7's list of wrapper responsibilities adds an ARIA need and drops INV5's style-bearing boundary, so it reads differently from the INV5 it cites.
[Automated review]
| silently decides future themeability. When desktop and touch render different | ||
| parts or use different theming owners, they use separate anatomy rows. | ||
| parts or use different theming owners, they use separate anatomy rows. Those | ||
| rows describe alternatives that are never mounted together, and both follow |
There was a problem hiding this comment.
INV7 allows one transient copy during a presentation switch, and design:reduced-motion DR2 makes that switch a cross-fade on a coarse pointer, so a popover row and a sheet row are both mounted for up to 200 ms. Adding “outside the transient copy architecture:component-style-authoring INV7 allows” here and in the INV2/INV3 failure signal would keep the two claims from disagreeing.
| element that shows it (INV2). A layer mounts exactly one presentation. The | ||
| only second copy is one transient copy during an exit, a presentation | ||
| switch, or a handoff between sheets, such as the cross-fade | ||
| `design:reduced-motion` DR2 requires; it is made by one Core mechanism that |
There was a problem hiding this comment.
No Core module or current record provides “one Core mechanism that every layer uses”: architecture:layer-runtime assigns layer presence to layerStack, useLayerDismissal and LayerDepthContext and names none, and BottomSheetSwitcher keeps its retained sheet in its own state, so its handoff fails the new “made by a component itself” signal from merge. Either keep only the observable rule, or name the mechanism and its owning record and record the switcher as a known gap.
| A component may exceed the budgets only through an exception its pull | ||
| request states, with why existing elements cannot do the job and the | ||
| measured element count before and after. Each exception is recorded as its | ||
| own owner decision in the component's record, which the pull request |
There was a problem hiding this comment.
Switch, SegmentedControl, CheckboxInput, DropdownMenu, ContextMenu and Lightbox have only draft records, and a draft record is “not a rule” and merges without owner approval, so an exception written there is not an owner decision. Requiring the exception in a current record approved at the pull request's head would close that.
| owns, one element for each themable part a current theme target paints | ||
| (`architecture:component-theming-surface` INV4), and a wrapper only where | ||
| INV5 names its responsibility: a distinct role, behavior or ref, a | ||
| formatting context, outside spacing, or an ARIA need. States and variants |
There was a problem hiding this comment.
INV5 names no ARIA need and does allow a style-bearing boundary when the child has no compatible surface, so this list reads differently from the INV5 it cites. “A wrapper only where INV5 allows one” would avoid the second list.
There was a problem hiding this comment.
Approval of this spec change waits on an engineering owner's decision: architecture:component-style-authoring INV7 and the architecture:component-theming-surface INV3 amendment add new requirements, and the only /approve-spec names the first commit, so an engineering owner needs to approve them as written at the current head, or have them amended first, and records that by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner). One text fix is needed either way: INV7's stopgap makes a layer's own exit or handoff copy a recorded conformance gap but leaves out the presentation-switch copy that INV7, INV3 and design:reduced-motion DR2 call for, so writing "a layer's own exit, presentation-switch or handoff copy" would close it.
[Automated review]
| second of the transition ending. Until that shared mechanism exists, a | ||
| layer's own exit or handoff copy is a recorded conformance gap, not an | ||
| exception. A change to a component prefers removing or |
There was a problem hiding this comment.
This stopgap covers a layer's own exit or handoff copy, but INV7 (lines 129-131) and the amended INV3 also allow a copy during a presentation switch, and design:reduced-motion DR2 requires that cross-fade on a coarse pointer. Until the shared mechanism exists, a layer that implements it makes the copy itself, which the Verification row fails ("a transient copy made by a component itself") with no gap status. Writing "a layer's own exit, presentation-switch or handoff copy" would treat the three cases alike.
There was a problem hiding this comment.
Approval of this spec change waits on an engineering owner's decision: architecture:component-style-authoring INV7 and the architecture:component-theming-surface INV3 amendment add new requirements, and the only /approve-spec names the first commit, so an engineering owner needs to approve them as written at the current head, or have them amended first, and records that by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner). The stopgap now covers a layer's own presentation-switch copy, so no text fix is outstanding in the records. Before that approval, please bring the description's Decision section up to date: it still describes only an exit copy from "the shared exit mechanism", lists wrappers without themable parts, and gives INV3 without the switch copy. An owner deciding may also want INV7 to say whether the transient copy is inert to assistive technology and whether the open-layer budget compares a component's two presentations, since the adaptive Selector opens with 158 elements on a coarse pointer against 149 on a fine one.
[Automated review]
…lement; the titles switch at once Components are built from minimal DOM (meta#8300, INV7): one element per role, no hidden twin of content, and no more elements under a finger than under a mouse. The touch presentation added an aria-hidden proxy element for the lens and cross-faded each title through a hidden twin. The lens is now the radiogroup's ::before, painted from custom properties copied from the selected segment's surface and driven by the animator on that pseudo-element; under Reduced Motion its ::after is the surface that fades out where it is. The titles take their new rendering with the selection, at once, so the label cross-fade and its roles are gone. With no width twin a selection can resize a content-sized control, so a resize hands the surface back only if the track no longer has the size its boxes were measured at. Boxes no longer divide by a scale that `offsetWidth`'s whole-pixel rounding invents, which put the lens up to a fraction of a pixel off its segment.
b5d05f8 to
c92b0b0
Compare
|
/approve-spec c92b0b0 |
github-actions
Bot
removed
the
needs:spec-owner-review
|
/approve-spec 717eb2b |
…style-authoring INV7, theming-surface INV3), decided by AST-017 FR60/DEC-26
717eb2b to
54275fc
Compare
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec 54275fc |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
This spec change is ready to merge from the review's side: an engineering owner approved architecture:component-style-authoring INV7, the architecture:component-theming-surface INV3 amendment and spec:AST-017 FR60/DEC-26 exactly as written, and none of them contradicts another current record. Before merging, please update the description's Decision section, which still names FR56/DEC-25 and describes only an exit copy from "the shared exit mechanism", so it matches the records. Optionally, FR60's verification row could fail "a second presentation outside INV7's transient copy", matching the INV7 and INV3 rows.
[Automated review]
| | FR53 | Changeset classification review against the governing component record's accessibility evidence | a fix that replaces documented keyboard or focus behavior to meet an adopted APG pattern's required parts; a fix that changes a role, state, or accessible name to meet WCAG 2.2 A/AA; a fix that adds an optional prop | An accessibility fix is held for a minor, given a coexistence path, or classified `[breaking]`; or a change that needs an app-layer code edit, or that removes, renames, or narrows public surface, ships as an accessibility `[fix]` | | ||
| | FR54 | Changeset classification review against the surface's current record, then the same tier admission every Changeset receives | a CLI correction the CLI record classifies as `[fix]`; a CLI change the CLI record leaves undecided; a CLI change the CLI record classifies as `[breaking]` on a patch plan; a component change | A non-component change is classified by a rule this record restates instead of its surface record, a surface rule turns on risk or adoption, a surface-classified `[breaking]` change ships in a patch or a surface rule makes it patch-eligible, or a component change is classified by a surface record | | ||
| | FR59 | Pull request review of fine-pointer before-and-after evidence | a touch change that also fixes a defect fine-pointer users hit; a deliberate cross-device consistency change; a touch-only change; a new fine-pointer look proposed with touch work | A touch change alters fine-pointer behavior or appearance with neither reason, shows no fine-pointer before-and-after evidence, or ships a new fine-pointer look without its own owner decision | | ||
| | FR60 | Element counts per story at rest and open, fine and coarse pointer, against architecture INV7 | a surface component at rest; an open layer; a DOM reduction; a recorded exception | An element beyond INV7's budgets with no recorded owner exception, a hidden twin or second presentation, or a DOM reduction that removes a theme target or documented anatomy, or that is classified as breaking | |
There was a problem hiding this comment.
Optional wording: this failure signal fails "a hidden twin or second presentation" without the transient-copy allowance that the INV7 row (docs/architecture/component-style-authoring.md:292) and the INV3 row (docs/architecture/component-theming-surface.md:444) carry. Once the shared Core mechanism exists, read alone it would fail the cross-fade design:reduced-motion DR2 requires. Suggest: "a hidden twin, or a second presentation outside INV7's transient copy".
Exact decision needed
What correct component construction looks like for every Astryx component: how many elements a component renders, how states and variants are expressed, and how a change keeps the DOM minimal.
Existing authority searched
architecture:component-style-authoringINV5: a wrapper needs a named responsibility, judged per wrapper; INV2: CSS owns CSS-resolvable presentation.architecture:component-theming-surfaceINV3: desktop and touch that render different parts get separate anatomy rows; nothing says they are never mounted together.architecture:component-test-sufficiencyINV11 andcomponent:BottomSheetSwitcherPR1 (one painted scrim): local only.Decision
architecture:component-style-authoringINV7 — components are built from minimal DOM:document.body.querySelectorAll('*').lengthper story, at rest and open, fine versus coarse pointer.architecture:component-theming-surfaceINV3: separate desktop and touch anatomy rows are alternatives never mounted together, and both follow INV7.spec:AST-017gains FR56 and DEC-25 (Cindy, 2026-10-24): components are built from minimal DOM under INV7's two budgets and exception path, and reducing undocumented DOM is compatible (FR3). INV7 andarchitecture:component-theming-surfaceINV3 cite DEC-25 indeciding_specs, so the observable rule has a deciding decision (architecture:knowledge-contractsINV18). FR56/DEC-25 follow FR55/DEC-24 from spec(AST-017): touch work changes desktop behavior only for a defect or deliberate consistency #8347; whichever merges second rebases.Rejected alternatives
Claim scope and non-goals
Construction and element counts only. Render or listener budgets, which components adopt touch presentations, and the CI that runs the measurement are outside this PR.
Evidence and implementation relationship
Recent touch presentation work adds wrappers to surface controls and can mount both presentations of a layer. Conforming components belongs to separate implementation pull requests. No implementation is included.
Writing rubric results
Scope
Validation
pnpm check:knowledge(CI lint)git diff --check