facebook / facebook/astryx

[Bug] lab/Drawer hardcodes its close delay against a themeable hold

Open
#4,664 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
13.1k
Forks
1.1k
Avg merge
1d 15h
Merged PRs (30d)
687

Description

### Description

`lab/Drawer` defers `dialog.close()` so its slide-out can play, and holds the drawer rendered meanwhile by transitioning `display` with `allow-discrete`. The hold is a theme value; the delay is a literal. They are the same number today by coincidence, and drift apart as soon as a theme changes.

`Drawer.tsx`:

```ts
// the hold — themeable
transitionProperty: 'transform, max-width, display',
transitionDuration: durationVars['--duration-medium'],
transitionBehavior: 'allow-discrete',
'@media (prefers-reduced-motion: reduce)': {
transitionDuration: '0.01s',
},

// the delay — literal
const duration = window.matchMedia('(prefers-reduced-motion: reduce)').matches
? 10
: 250;
closeTimeoutRef.current = setTimeout(() => {
dialog.close();
...
}, duration);
```

Four cases:

| theme | hold | delay | result |
|---|---|---|---|
| default (`--duration-medium: 410ms`) | 410ms | 250ms | inside the hold |
| `prefers-reduced-motion` | 10ms | 10ms | exactly on the boundary |
| shipped **y2k** (`motion.medium: 250`) | 250ms | 250ms | exactly on the boundary |
| any theme below 250ms | < 250ms | 250ms | **outside the hold** |

In the last row `close()` runs after `display` has already flipped to `none`. A `` opened with `showModal()` blocks the whole document for as long as it is in the top layer whether or not it is rendered, so that window is a page that looks normal and is not interactive. A browser that then fails to un-block on close leaves it that way, with no JavaScript error — which is #4290, the same failure `MobileNav` had.

Nothing validates motion values, so `defineTheme({motion: {medium: 200}})` is enough to reach the last row.

### Suggested fix

Derive the delay from the hold actually in effect rather than assuming it, as `MobileNav` now does in #4388 (`resolveCloseDelay`, reading `getComputedStyle(dialog).transitionDuration` and closing at a fraction of it). Note browsers serialise computed `

Reduced motion should shorten the *delay*, not the hold. Shortening both leaves no slack at all — one slow frame between the commit and the close macrotask and the drawer has already stopped being rendered.

### Wider point

Both components would be safe without per-component guards if theme motion values were floored where themes are defined (`expandMotionScale` / `defineTheme`). That is one change covering every component rather than one patch per component, and it also closes the `medium: 0` case that no per-component fix can reach. Raised here rather than in #4388 because it is an API call.

### Astryx Version

`main` (`packages/lab/src/Drawer/Drawer.tsx`)

Contributor guide

Open the contributing guide

Research direction

Start in packages/lab/src/Drawer/Drawer.tsx and compare MobileNav’s resolveCloseDelay from #4388, then inspect expandMotionScale and defineTheme where motion values are defined. Confirm the chosen fix keeps dialog.close() within the computed transition hold for custom and reduced-motion themes, including very small durations.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
frontend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.