Repository navigation
docs(spec): MobileNav records its shipped contract as current - #8466
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:spec-owner-review
PR Analysis Report
The exact-head preview is not available yet. Generated by PR Enrichment workflow | View current CI run |
meta-cla
Bot
added
the
CLA Signed
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This spec promotion is on hold for an owner decision: it makes MobileNav's FR1–FR12, ORD1–ORD3, PR1, AR1–AR4 and verification map current, and no engineering owner has approved this exact head yet. Any engineering owner, including the author if they are one, can record that decision by commenting /approve-spec followed by the full head commit SHA. Separately from that decision, a few rows need changes before approval:
- FR7 and FR8 state the 0.01 s reduced-motion slide and the physical-half
side="auto"rule as requirements, which contradicts the current reduced-motion and bidirectionality design records; keep them as Current-state impact gaps and defer those clauses to the design records. - FR2, FR10, FR11 and FR12 misstate what a caller's
id,aria-label,aria-labelledbyandonCanceldo, and FR8 leaves out the case where nothing is focused at open. - The verification map should cite
layerDismissalFamilies.test.tsxfor FR4, and theonClickcancellation and the Togglelabeloverride need focused tests.
[Automated review]
| | FR4 | The native cancel request (Escape) is routed through the shared dismissal stack and requests close only when this drawer is topmost. A click on the overlay outside the Drawer requests close; a click inside does not. The Close button, named by `@astryx.mobileNav.closeNavigation`, requests close. A caller `onClick` on the dialog runs before the backdrop check; one that calls `preventDefault()` cancels that backdrop close. | Current source and tests | | ||
| | FR5 | After a close request the drawer slides out while the dialog stays open, and the dialog closes within its slide-out transition and no later than 250 ms after the request (at once under reduced motion or a zero-length transition). Reopening before then cancels the close. Unmounting closes the dialog. | Current source and tests | | ||
| | FR6 | When the dialog closes, focus returns to the recorded opener if it is still connected and was not the document body; otherwise focus is left alone. A reopen that interrupted a close keeps the original opener. | Current source and tests | | ||
| | FR7 | The Drawer enters from its resolved inline edge, from off-screen to on-screen, and the backdrop fades in from transparent; both run for 0.01 s under reduced motion. The overlay clips the off-screen drawer without becoming a scroll container and allows pinch zoom. | Current source and tests | |
There was a problem hiding this comment.
The last clause of FR7 makes the 0.01 s slide and fade under reduced motion a current requirement. The current reduced-motion design record forbids that (DR1: "MUST NOT move, scale, resize, or spring its content"; DR3: "MUST complete immediately, with no animated property"). Current-state impact already records it as a gap, so consider deferring this clause to the design record, as AppShell FR4 defers to AST-025 FR12, rather than requiring it.
| | FR5 | After a close request the drawer slides out while the dialog stays open, and the dialog closes within its slide-out transition and no later than 250 ms after the request (at once under reduced motion or a zero-length transition). Reopening before then cancels the close. Unmounting closes the dialog. | Current source and tests | | ||
| | FR6 | When the dialog closes, focus returns to the recorded opener if it is still connected and was not the document body; otherwise focus is left alone. A reopen that interrupted a close keeps the original opener. | Current source and tests | | ||
| | FR7 | The Drawer enters from its resolved inline edge, from off-screen to on-screen, and the backdrop fades in from transparent; both run for 0.01 s under reduced motion. The overlay clips the off-screen drawer without becoming a scroll container and allows pinch zoom. | Current source and tests | | ||
| | FR8 | `side="start"` or `"end"` sets the edge. `side="auto"` resolves on each open from the focused element: a center in the left half of the viewport picks `start`, otherwise `end`; before any open it is `end`. | Current source | |
There was a problem hiding this comment.
In right-to-left, this rule puts the drawer on the edge opposite any trigger: a left-half trigger gets start (the right edge), and a right-half trigger gets end (the left edge). That contradicts bidirectionality DR7. Separately, when nothing is focused at open, the side is not resolved again and keeps its last value (MobileNav.tsx:545), which FR8 doesn't state.
| | FR9 | The Drawer is `width` px wide (default 320), never wider than 85% of the viewport, so a strip of the page stays beside it. | Current source and tests | | ||
| | FR10 | The dialog is named by `label`, else a string `header`, else `@astryx.mobileNav.navigation`. A string `header` renders as a level-2 heading; other header content renders as given. | Current source and tests | | ||
| | FR11 | MobileNavToggle renders only while AppShell reports its mobile layout with mobile navigation enabled. It is an icon-only ghost Button named by `label` or `@astryx.mobileNav.toggle.open`, with a menu icon unless `children` replace it; it toggles the drawer, exposes `aria-expanded`, and its `aria-controls` carries AppShell's drawer id, which a MobileNav inside that AppShell applies to its dialog. | Current source, docs, and tests | | ||
| | FR12 | MobileNav forwards its `ref`, `data-testid`, other DOM props, `className`, `style`, and `xstyle` to the dialog. | Current source and tests | |
There was a problem hiding this comment.
Not every DOM prop reaches the dialog. aria-label and onCancel are overwritten (MobileNav.tsx:694-701). A caller id is spread after id={dialogId} and replaces the drawer id that FR2 and FR11 promise, so inside AppShell the Toggle's aria-controls points at nothing. A caller aria-labelledby is forwarded and outranks FR10's naming order. Consider stating these exceptions here.
| target with its `side` state. | ||
| - FR2: the dialog's own id outside AppShell. | ||
| - FR3: focus moving to the drawer panel rather than the Close button. | ||
| - FR4: routing Escape through the shared dismissal stack, the caller `onClick` |
There was a problem hiding this comment.
packages/core/src/Layer/layerDismissalFamilies.test.tsx already covers this for MobileNav: Escape over a required Dialog, and a cancel when the drawer is top-most and when it is not. It can move into the FR4 row of the map and into verified_by.
There was a problem hiding this comment.
This spec promotion is on hold for an owner decision: it makes MobileNav's FR1–FR12, ORD1–ORD3, PR1, AR1–AR4 and verification map current, and no engineering owner has approved this exact head yet. Any engineering owner, including the author if they are one, can record that decision by commenting /approve-spec followed by the full head commit SHA. FR7 and FR8 now defer cleanly to the design records, but a few rows still need changes before approval, separately from that decision:
- FR12 records a caller's
onCancelbeing dropped as current behavior, while the public component API record expects consumer event handlers to reach the element and compose; list it as a Current-state impact gap, as FR2 does forid. - The Closing representative state says the dialog stays open through the slide-out, but FR5 closes it within the slide-out and no later than 250 ms after the request.
- FR3's first focus, FR4's
preventDefault()cancellation and FR11's Togglelabeloverride need focused tests or narrower claims, and the source-only list should name the new caller-prop clauses.
[Automated review]
| | FR9 | The Drawer is `width` px wide (default 320), never wider than 85% of the viewport, so a strip of the page stays beside it. | Current source and tests | | ||
| | FR10 | The dialog is named by `label`, else a string `header`, else `@astryx.mobileNav.navigation`; a caller `aria-labelledby` outranks all three. A string `header` renders as a level-2 heading; other header content renders as given. | Current source and tests | | ||
| | FR11 | MobileNavToggle renders only while AppShell reports its mobile layout with mobile navigation enabled. It is an icon-only ghost Button named by `label` or `@astryx.mobileNav.toggle.open`, with a menu icon unless `children` replace it; it toggles the drawer, exposes `aria-expanded`, and its `aria-controls` carries AppShell's drawer id, which a MobileNav inside that AppShell applies to its dialog unless a caller `id` replaces it. | Current source, docs, and tests | | ||
| | FR12 | MobileNav forwards its `ref`, `data-testid`, other DOM props, `className`, `style`, and `xstyle` to the dialog, except that its own `aria-label` (FR10) and cancel handler (FR4) replace a caller's `aria-label` and `onCancel`, and a caller `id` replaces its id (FR2). | Current source and tests | |
There was a problem hiding this comment.
A caller onCancel never runs: {...rest} is spread and onCancel={handleCancel} then replaces it (MobileNav.tsx:692, :701). Writing that as contract conflicts with the public component API record (INV5: supported event inputs reach the element; INV7: consumer handlers compose). Consider listing it under Current-state impact and marking this clause "(Current-state impact)", as FR2 does for id.
| | AppShell drawer | The same drawer anatomy consumes AppShell mobile state. | Header and navigation content remain caller-owned. | | ||
| | AppShell mobile toggle | Toggle button controls and references the current drawer. | Custom button content may replace the menu glyph. | | ||
| | Non-mobile/disabled host | Toggle button does not render. | MobileNav remains owned by the parent contract. | | ||
| | Closing | Dialog stays open through the slide-out, then focus returns. | Duration by theme. | |
There was a problem hiding this comment.
FR5 and ORD3 close the dialog within the slide-out and no later than 250 ms after the request. With the default 410 ms --duration-medium, the code closes it at about 246 ms, so the dialog does not stay open through the slide-out. Suggest: "Dialog stays open while the drawer starts sliding out and closes within the slide-out (FR5); then focus returns."
| | ID | Invariant | Basis | | ||
| | ---- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------- | | ||
| | FR1 | MobileNav renders one native `<dialog>` Navigation overlay containing a Drawer with a stable Header, Close button, and scrollable Content region. The overlay carries `mobile-nav` with the resolved `side`; Drawer, Header, and Content carry no MobileNav public target. | Current source, docs, and tests | | ||
| | FR2 | `isOpen` and `onOpenChange` win when given; otherwise the drawer reads and changes the enclosing AppShell's drawer state. The dialog uses AppShell's drawer id when there is one, else its own; a caller `id` replaces either (Current-state impact). | Current source and tests | |
There was a problem hiding this comment.
An explicit id={undefined}, which a wrapper forwarding its own optional id passes, also replaces the id: the dialog ends up with none, and the Toggle's aria-controls names nothing. The Current-state impact bullet could say "a caller id prop, even undefined" and cite AppShell FR13 and AR3, the claims this breaks, beside INV5.
| | FR9 | `MobileNav.test.tsx` drawer cap test; `MobileNavWidth.a11y.chromium.spec.ts` | Default and wide `width` on a narrow viewport | A drawer wider than 85% of the viewport fails. | `audit:MobileNav/behavior` | | ||
| | Theming anatomy map | `scripts/check-knowledge.mjs`; `themingTargets.test.ts` | Canonical parent anatomy and current target | Missing, extra, prefixed, stale, or multiply assigned mappings fail repository validation. | `audit:MobileNav/theming` | | ||
|
|
||
| Read from source, with no focused test yet: |
There was a problem hiding this comment.
This list leaves out the caller-prop clauses this revision adds, none of which has a test: the aria-labelledby order (FR10, ORD2), the replaced aria-label and onCancel (FR12), and a caller id inside AppShell (FR2, FR11). FR3's first focus, FR4's preventDefault() cancellation and FR11's label override are critical promises under the component test sufficiency record's INV2, so they need focused tests or narrower claims.
There was a problem hiding this comment.
This spec promotion is on hold for an owner decision: it makes MobileNav's FR1–FR12, ORD1–ORD3, PR1, AR1–AR4 and verification map current, and no engineering owner has approved this exact head yet. Any engineering owner, including the author if they are one, can record that decision by commenting /approve-spec followed by the full head commit SHA. FR12, the Closing state and the caller id gap now match the code; separately from that decision, a few lines still need changes before approval:
- The relationship line says the layer runtime owns only the native-dialog host and MobileNav keeps its own backdrop, but the layer-runtime architecture record on
main(INV14) now gives the runtime the close hold, exit copy and backdrop tint; restate that split and list MobileNav's shortfall under Current-state impact. - Contract at a glance still says the dialog stays open until the slide-out runs, which reads against FR5, and its Governing rules should name the reduced-motion and public-component-api records that two of the gaps rely on.
- The source-only list should add FR10's non-string
headerand FR11's Togglechildren.
[Automated review]
| - `architecture:layer-runtime` owns the native-dialog host and shared layer | ||
| plumbing; MobileNav keeps its own drawer motion and backdrop. |
There was a problem hiding this comment.
On main, the layer-runtime architecture record's INV14 gives the runtime the presentation hold, the shared exit copy and the ::backdrop tint ("A component that needs one of these uses the runtime's rather than building its own"), leaving each component its motion and the backdrop's color and treatment. Its Current gaps already names MobileNav among the components that build their own top-layer mounting, exit or backdrop. Consider splitting this line the same way and listing that shortfall under Current-state impact, as the record does for DR7 and DR2–DR3.
| | Area | Contract | | ||
| | ----------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | Public contract | `MobileNav` with `isOpen`, `onOpenChange`, `header`, `width` (320), `side` (`start`, `end`, `auto`; default `auto`), `label`, and `children`; target `mobile-nav` with a `side` state; `MobileNavToggle` with `label` and `children`. | | ||
| | Behavior | A modal native dialog with a drawer that slides in from one inline edge, a header with a Close button, and a scrolling content region. Escape, the backdrop, and Close request closing; the dialog stays open until the slide-out runs, then focus returns to the opener. | |
There was a problem hiding this comment.
"the dialog stays open until the slide-out runs" reads as the dialog staying open through the whole slide-out, while FR5 and the Closing state close it within the slide-out and no later than 250 ms after the request (about 246 ms with the default 410 ms duration). Suggest: "the dialog stays open while the drawer starts sliding out and closes within the slide-out (FR5), then focus returns to the opener."
| | Builder impact | None. | | ||
| | Compatibility/readiness | Records shipped behavior; no public change. Four implementation gaps: automatic side selection uses physical position, reduced motion still animates, a caller `id` disconnects the Toggle, and a caller `onCancel` never runs (see Current-state impact). | | ||
| | Review checks | Reject a non-modal drawer, a close that hides the drawer before its slide-out runs, focus not returned to a connected opener, a backdrop click inside the drawer that closes it, or a Toggle rendered outside an enabled AppShell mobile layout. | | ||
| | Governing rules | `family:overlay-dismissal`; `architecture:layer-runtime`; `component:AppShell` FR10–FR13; `design:bidirectionality` DR7; `architecture:component-theming-surface`. | |
There was a problem hiding this comment.
The Compatibility/readiness row names four gaps, but this cell links neither design:reduced-motion DR1–DR3, which FR7 defers to, nor architecture:public-component-api INV5 and INV7, which the id and onCancel gaps cite. Consider adding both here and to the relationships list at line 243.
| | FR9 | `MobileNav.test.tsx` drawer cap test; `MobileNavWidth.a11y.chromium.spec.ts` | Default and wide `width` on a narrow viewport | A drawer wider than 85% of the viewport fails. | `audit:MobileNav/behavior` | | ||
| | Theming anatomy map | `scripts/check-knowledge.mjs`; `themingTargets.test.ts` | Canonical parent anatomy and current target | Missing, extra, prefixed, stale, or multiply assigned mappings fail repository validation. | `audit:MobileNav/theming` | | ||
|
|
||
| Read from source, with no focused test yet: |
There was a problem hiding this comment.
No test renders a non-string header (FR10, "other header content renders as given") or a Toggle with children (FR11), so both belong in this list. Two listed items already have tests and could move into the map: a caller id replacing the standalone id (MobileNav.test.tsx:145-162) and the 250 ms bound (MobileNavCloseEdgeCases.test.tsx:501-536).
…ds; states prop exceptions
b81b902 to
a12ee4e
Compare
There was a problem hiding this comment.
This spec promotion is on hold for an owner decision: it makes MobileNav's FR1–FR12, ORD1–ORD3, PR1, AR1–AR4 and verification map current, and no engineering owner has approved this exact head yet. Any engineering owner, including the author if they are one, can record that decision by commenting /approve-spec followed by the full head commit SHA. The layer-runtime relationship line and Contract at a glance now match the layer-runtime record and FR5; separately from that decision, two record fixes remain before approval:
- The INV14 gap under Current-state impact and the Compatibility/readiness cell list MobileNav's own close hold and backdrop but not its own top-layer mounting (its own
dialog.showModal()call), which the layer-runtime architecture record's INV14 also assigns to the runtime. - The source-only list should add FR11's icon-only Button and Toggle
label, and a calleridinside AppShell (FR2, FR11).
[Automated review]
| - **MobileNav builds its own close hold and backdrop.** MobileNav keeps the | ||
| dialog open through its close with its own timer and paints its own backdrop | ||
| fade. `architecture:layer-runtime` INV14 has a layer use the runtime's | ||
| presentation hold and backdrop tint rather than building its own. |
There was a problem hiding this comment.
This gap lists MobileNav's own close hold and backdrop, but MobileNav also mounts its own top layer: its open effect calls dialog.showModal() itself (MobileNav.tsx:577-579). The layer-runtime architecture record's INV14 lists top-layer mounting first among the mechanisms the runtime owns, its verification row names "a component building its own copy of a mechanism INV14 lists (top-layer mounting, …)" as the failure, and its Current gaps names MobileNav. Suggest: "MobileNav mounts its own top layer, holds its own close, and paints its own backdrop", and the same in the Compatibility/readiness cell (line 53).
| - FR2: the dialog's own id outside AppShell. | ||
| - FR8's shipped resolution. | ||
| - FR10 and ORD2: the `label` prop, a caller `aria-labelledby` outranking the | ||
| name, the level-2 heading for a string `header`, and other header content | ||
| rendering as given. | ||
| - FR11: rendering only in an enabled AppShell mobile layout, the ghost variant, | ||
| the menu icon, and `children` replacing it. |
There was a problem hiding this comment.
Three untested clauses are missing here: FR11's icon-only Button and a Toggle label replacing the default name (no test passes label to MobileNavToggle; its suite checks only the default name), and a caller id replacing AppShell's drawer id inside AppShell (FR2, FR11; only the standalone case, MobileNav.test.tsx:145-162, is tested).
…ining untested clauses
There was a problem hiding this comment.
This spec promotion is on hold for an owner decision: it makes MobileNav's FR1–FR12, ORD1–ORD3, PR1, AR1–AR4 and verification map current, and no engineering owner has approved this exact head yet. Any engineering owner, including the author if they are one, can record that decision by commenting /approve-spec followed by the full head commit SHA. The top-layer gap and the source-only list now match the code; separately from that decision, a few record fixes remain before approval:
- FR12 records a caller
aria-labelbeing dropped as contract, while the public component API record (INV5) has supported ARIA props reach the element, as the Avatar record applies it; list it under Current-state impact likeonCancel. - The right-to-left Current-state impact bullet should say every
side="auto"trigger opens the drawer on the far side, not only a trigger on the left. - FR5's "at once under reduced motion" should say the close lands on the next task, never in the commit that requests it, as its tests require, and the description should list all the gaps.
[Automated review]
| | FR9 | The Drawer is `width` px wide (default 320), never wider than 85% of the viewport, so a strip of the page stays beside it. | Current source and tests | | ||
| | FR10 | The dialog is named by `label`, else a string `header`, else `@astryx.mobileNav.navigation`; a caller `aria-labelledby` outranks all three. A string `header` renders as a level-2 heading; other header content renders as given. | Current source and tests | | ||
| | FR11 | MobileNavToggle renders only while AppShell reports its mobile layout with mobile navigation enabled. It is an icon-only ghost Button named by default by `@astryx.mobileNav.toggle.open`, with a menu icon unless `children` replace it; it toggles the drawer, exposes `aria-expanded`, and its `aria-controls` carries AppShell's drawer id, which a MobileNav inside that AppShell applies to its dialog unless a caller `id` replaces it. | Current source, docs, and tests | | ||
| | FR12 | MobileNav forwards its `ref`, `data-testid`, other DOM props, `className`, `style`, and `xstyle` to the dialog, except that its own `aria-label` (FR10) replaces a caller's, a caller `onCancel` never runs (Current-state impact), and a caller `id` replaces its id (FR2). | Current source and tests | |
There was a problem hiding this comment.
A caller aria-label is accepted by the type (BaseProps extends React.HTMLAttributes without omitting it) and dropped here (MobileNav.tsx:692-699). architecture:public-component-api INV5 has supported ARIA inputs reach the element, and current component:Avatar FR9 applies it ("a caller aria-label, role, or aria-hidden wins"). Like onCancel and id, this needs a Current-state impact entry and a "(Current-state impact)" marker; otherwise two current records make different claims.
| - **Automatic side follows physical position.** With `side="auto"`, a trigger | ||
| whose center is in the left half of the viewport opens the drawer at the | ||
| inline start, otherwise at the inline end. In a right-to-left layout the | ||
| inline start is the right edge, so a trigger on the left opens the drawer on | ||
| the far side. `design:bidirectionality` DR7 requires a layered surface to keep | ||
| its start and end relationship to its trigger. |
There was a problem hiding this comment.
In right-to-left a trigger in the right half resolves to end, which is the left edge (MobileNav.tsx:549-551, :203-212), so every trigger, including the usual menu button on the right, opens the drawer on the far side. Suggest: "so in a right-to-left layout every trigger opens the drawer on the far side."
| | FR2 | `isOpen` and `onOpenChange` win when given; otherwise the drawer reads and changes the enclosing AppShell's drawer state. The dialog uses AppShell's drawer id when there is one, else its own; a caller `id` replaces either (Current-state impact). | Current source and tests | | ||
| | FR3 | Opening calls `showModal()`, and records the focused element as the opener; where focus lands inside the open drawer is uncontracted here. While open, the page's root clips its overflow and keeps its scrollbar gutter; closing releases both. | Current source and tests | | ||
| | FR4 | The native cancel request (Escape) is routed through the shared dismissal stack and requests close only when this drawer is topmost. A click on the overlay outside the Drawer requests close; a click inside does not. The Close button, named by `@astryx.mobileNav.closeNavigation`, requests close. A caller `onClick` on the dialog also runs; whether it can cancel the backdrop close is uncontracted here. | Current source and tests | | ||
| | FR5 | After a close request the drawer slides out while the dialog stays open, and the dialog closes within its slide-out transition and no later than 250 ms after the request (at once under reduced motion or a zero-length transition). Reopening before then cancels the close. Unmounting closes the dialog. | Current source and tests | |
There was a problem hiding this comment.
"At once" would allow a close in the same commit, which MobileNavCloseVisibility.test.tsx:119-148 exists to reject ("This pins deferral, not duration"); the close lands on the next task (MobileNavCloseEdgeCases.test.tsx:393). Suggest: "on the next task, never in the commit that requests it, under reduced motion or a zero-length transition".
There was a problem hiding this comment.
This spec promotion is on hold for an owner decision: it makes MobileNav's FR1–FR12, ORD1–ORD3, PR1, AR1–AR4 and verification map current, and no engineering owner has approved this exact head yet. Any engineering owner, including the author if they are one, can record that decision by commenting /approve-spec followed by the full head commit SHA. The aria-label gap, the right-to-left bullet and FR5's next-task close now match the code; separately from that decision, one record fix remains before approval:
- FR4 leaves a caller
onClickthat callspreventDefault()able to cancel the backdrop close while calling that uncontracted, but the public component API record (INV7) allows consumer cancellation only where the API promises it, so FR4 should either promise it, as the SideNavCollapseButton and TopNavMenu records do, or list it under Current-state impact. - The description's "Existing authority searched" still mentions two recorded gaps and a layer runtime that owns only the native-dialog host; matching it to the record would help the approver.
[Automated review]
| | FR1 | MobileNav renders one native `<dialog>` Navigation overlay containing a Drawer with a stable Header, Close button, and scrollable Content region. The overlay carries `mobile-nav` with the resolved `side`; Drawer, Header, and Content carry no MobileNav public target. | Current source, docs, and tests | | ||
| | FR2 | `isOpen` and `onOpenChange` win when given; otherwise the drawer reads and changes the enclosing AppShell's drawer state. The dialog uses AppShell's drawer id when there is one, else its own; a caller `id` replaces either (Current-state impact). | Current source and tests | | ||
| | FR3 | Opening calls `showModal()`, and records the focused element as the opener; where focus lands inside the open drawer is uncontracted here. While open, the page's root clips its overflow and keeps its scrollbar gutter; closing releases both. | Current source and tests | | ||
| | FR4 | The native cancel request (Escape) is routed through the shared dismissal stack and requests close only when this drawer is topmost. A click on the overlay outside the Drawer requests close; a click inside does not. The Close button, named by `@astryx.mobileNav.closeNavigation`, requests close. A caller `onClick` on the dialog also runs; whether it can cancel the backdrop close is uncontracted here. | Current source and tests | |
There was a problem hiding this comment.
A caller onClick that calls preventDefault() does cancel the backdrop close today: the dialog runs composeEventHandlers(onClickProp, handleDialogClick) (MobileNav.tsx:700), which stops at the first prevented event. The public component API record's INV7 allows consumer cancellation only "where that public API promises cancellation", so "uncontracted here" leaves an unrecorded shortfall. Consider either promising it here, as SideNavCollapseButton FR4 and TopNavMenu FR6 do (and adding it to the source-only list), or listing it under Current-state impact against INV7, like the onCancel gap.
There was a problem hiding this comment.
This spec promotion is on hold for an owner decision: it makes MobileNav's FR1–FR12, ORD1–ORD3, PR1, AR1–AR4 and verification map current, and no engineering owner has approved this exact head yet. Any engineering owner, including the author if they are one, can record that decision by commenting /approve-spec followed by the full head commit SHA. FR4 now promises the caller onClick cancellation that the public component API record requires, and nothing else needs to change before approval. Two optional wording fixes: the reduced-motion gap could note that the backdrop's 0.01 s fade already meets the touch cross-fade rule, and the description's open-work line could name the open pull requests that rewrite MobileNav's page lock and close hold (#8098, #8119, #8374).
[Automated review]
| own timer, and paints its own backdrop fade. `architecture:layer-runtime` | ||
| INV14 has a layer use the runtime's top-layer mounting, presentation hold, and | ||
| backdrop tint rather than building its own. | ||
| - **Motion under reduced motion.** Under reduced motion the Drawer's slide and |
There was a problem hiding this comment.
Optional wording: on a coarse pointer the backdrop's 0.01 s opacity fade is already an opacity-only change within 200 ms, which design:reduced-motion DR2 allows, so only the drawer's slide (every pointer) and the backdrop fade on a fine pointer (DR3) fall short. Something like "the Drawer still slides for 0.01 s, and on a fine pointer the backdrop still fades" would keep the gap exact.
|
/approve-spec 181877f |
github-actions
Bot
added
spec-auto-merge
Exact decision needed
Promote
component:MobileNavfrom draft to current: record the shipped behavior of MobileNav and MobileNavToggle as its contract, keeping the draft's anatomy, theming map, owners, and open question. No behavior, prop, default, target, or export changes.Existing authority searched
mobile-navtarget,nonedispositions for Drawer, Header, and Content, and OQ1 on a Drawer target) is kept.component:AppShell(current) FR10–FR13 decide when the drawer renders, hold it through a close, and provide the drawer state and id.family:overlay-dismissal(current, MobileNav a member) owns Escape and close-request ordering;architecture:layer-runtimeINV14 gives the runtime top-layer mounting, the presentation hold, the exit copy, and the backdrop tint.design:bidirectionalityDR7,design:reduced-motionDR1–DR3,architecture:layer-runtimeINV14, andarchitecture:public-component-apiINV5 and INV7 govern the six recorded gaps.Decision
component:MobileNav,authority: current, approved by cixzhang: the native modal dialog and its parts, AppShell state wiring, opening and scroll lock, the three close requests, the deferred close and its 250 ms bound, focus return, entry motion, side selection, the 85% width cap, naming, MobileNavToggle, and prop forwarding (FR1–FR12).Rejected alternatives
None.
Claim scope and non-goals
Only the claims inside the ownership boundary. The record ends its verification map with the claims read from source that no focused test checks yet.
Evidence and implementation relationship
MobileNav.test.tsx,MobileNavCloseEdgeCases.test.tsx,MobileNavCloseVisibility.test.tsx,MobileNavCloseTiming.test.ts,MobileNavFocus.test.tsx,MobileNavEntryAnimation.test.tsx,MobileNavScrollbarGutter.test.tsx,MobileNavToggle.test.tsx,MobileNavReopen.test.tsx, and the pinch-zoom and width real-browser specs.Current-state impact items (each verified in source):
side="auto"picks the edge from the opener's physical half of the viewport, so in right-to-left every opener gets a far-side drawer (design:bidirectionalityDR7).design:reduced-motionDR2–DR3).architecture:layer-runtimeINV14).id, evenundefined, replaces the drawer id, so inside AppShell the Toggle'saria-controlsnames nothing (architecture:public-component-apiINV5;component:AppShellFR13, AR3).aria-labelandonCancelon the dialog are dropped (architecture:public-component-apiINV5, INV7).Writing rubric results
human-apiquestion.Scope
Validation
pnpm check:knowledge: alignedgit diff --check: clean