Uh oh!
There was an error while loading. Please reload this page.
fix(settings): segmented track, portalled popup layering, and a first-party time picker - #1511
Merged
Conversation
Three control-level defects, all visible in the settings cards: - The `.maka-segmented` track carried `flex-wrap: wrap`, so any locale whose labels ran wider than the row folded an option onto a second line. The en locale hit it: "Follow system / 中文 / English" is ~25% wider than the zh labels, and "English" dropped below the track, turning one switch into a ragged two-row plate (measured: 50px tall, buttons on two baselines). The track now refuses to wrap or shrink; the label stack beside it already carries `min-width: 0` and absorbs the squeeze, and below the 460px card container the whole row stacks. - The track sat on `--radius-pill` (999px) while every field and select it shares a card with sits on the 6/8/12 scale, so it read as a foreign shape. Track moves to `--radius-surface` (8px) with segment buttons on `--radius-control` (6px) — concentric across the 2px track padding. This lands on all five Segmented consumers, including the usage-stats range selector. - The daily-review 执行时间 field inherited the Input primitive's `w-full` under a flat 140px cap, so `08:00` plus the picker glyph (~80px of ink) left a ~60px dead strip inside the field. It now hugs its content on the same right rail as the switch rows above it. Measured before/after over the `settings-general` (en) and `settings-daily-review` fixtures via CDP: track 50px→26px tall on one baseline, radii 999px→8px/6px, time field 140px→94px wide. The segmented contract test is re-pinned to the new intent (no-wrap, no-shrink, concentric radii) rather than the old wrap assertion. Note: the radius shrink form `calc(var(--radius-*) - Npx)` is NOT used here. The radius-converge contract splits corner values on whitespace, so it only admits the unspaced `calc(var(--x)-2px)` — which is invalid CSS (the spec requires whitespace around `-`) and computes to 0px. The tier token says the same thing and actually renders. Filed separately.
…e time picker **默认权限模式 was unclickable.** Base UI renders Select popups `position: static` inside an absolutely-positioned positioner, so the `z-[var(--z-overlay)]` that `SelectPopup` carried was inert — a static box ignores z-index entirely. What actually kept settings selects above `.settingsModal` (z-index 35) was the `.settingsSelectPositioner` class, applied by hand at each call site. `PermissionModeSelect` was the one Select in the codebase that never got it, so its popup painted *under* the modal: present in the DOM with `aria-expanded="true"`, invisible on screen, and unclickable because the modal won every hit-test. That reads exactly as "clicking does nothing at all". The layer now lives on wrapped `SelectPositioner` / `PopoverPositioner` in ui.tsx, so every consumer gets it by construction instead of by remembering a class name, and the inert layer is off the popups. `z-index-contract` grows a case pinning both halves — the positioners must carry the layer, the popups must not, since a z-index there reads as protection that isn't there. Verified with trusted CDP input (`Input.dispatchMouseEvent`), which respects paint order: before, clicking an option left the trigger on 询问权限; after, it commits 自动执行. Synthetic `el.dispatchEvent` cannot see this class of bug — it bypasses hit-testing, so a DOM-level check passes against a popup nobody can click. **The daily-review time field is now ours.** `<input type="time">` was the only Settings control outside the design system: WebKit draws its own popup — platform-accent columns, platform metrics, no dark mode — and no `::-webkit-*` selector reaches it, so the only way onto the design system was to own the popup. New `TimePicker` primitive: same field chrome as its neighbours, two columns (hours, minutes on a 5-min grid), same `HH:MM` value contract as the element it replaces, so the persisted config shape is unchanged. Two ordering hazards it has to survive, both found by measurement: Base UI mounts the popup before measuring it (every rect reads 0, so a naive scroll write commits 0 — indistinguishable from "already at the top"), and then moves focus into it, whose scroll-into-view overwrites whatever was set. Centring is therefore gated on `onOpenChangeComplete`, and initial focus is pinned to the *selected* hour rather than row 00 — better for keyboard users, and it lands the column in the right place. Adds the Popover primitive wrapper (Select is the wrong shape here: two independent columns have no single selected item for it to own). Gates: 2855 desktop + 250 ui tests green, biome clean, 0 dead CSS. Also plans the drag-to-grant permission onboarding in docs/permission-onboarding-plan.md — technique only, no vendor code.
Astro-Han
approved these changes
Jul 27, 2026
Astro-Han
left a comment
Contributor
There was a problem hiding this comment.
Approved. The current fixes are ready to merge. The following TimePicker findings are non-blocking and can be addressed in a separate follow-up PR:
- Preserve the full existing
HH:MMcontract, including minute values outside the default 5-minute grid. - Keep hour and minute changes atomic so a config save in progress cannot silently drop the second selection.
- Complete the custom listbox’s keyboard and accessibility semantics, including arrow-key navigation, a managed tab stop, and an accessible dialog name.
CI is green, and the formatting, lint, UI, and desktop test suites also pass locally.
Uh oh!
There was an error while loading. Please reload this page.
jackwener added a commit
that referenced
this pull request
Jul 27, 2026
…SS (#1514) * fix(design-system): stop the radius contract from mandating invalid CSS `search-modal.css` rendered square corners where the concentric formula asks for 4px, and the governance test was the reason. CSS requires whitespace around `+`/`-` inside `calc()`. Without it the declaration is a parse error and the browser drops it, so the box falls back to radius 0. Measured in the renderer: calc(var(--radius-modal)-8px) -> 0px (dropped) calc(var(--radius-modal) - 8px) -> 4px (intended) The contract accepted only the broken spelling. Two rules combined to force it: `findCssOffenders` split corner values on `/\s+/`, which tore the valid form into three nonsense corners (`calc(var(--radius-modal)`, `-`, `8px)`) and rejected it; and `CALC_ALLOW_RE` used `\s*-\s*`, so the unspaced form passed. The only spelling that satisfied both was the one the browser throws away. - `splitCorners()` splits on whitespace at paren depth 0, so a calc() stays one corner and genuine multi-corner values still split. - `CALC_ALLOW_RE` now requires the whitespace CSS requires, making the invalid spelling a contract failure rather than the mandated one. - `search-modal.css:32` moves to the valid form. Verified live: the `.maka-search-modal-input-row` rule now computes 4px. Tailwind arbitrary values keep the unspaced spelling, deliberately — a literal space there would have to be written `_`, and Tailwind emits valid CSS either way. Confirmed against the built bundle: source rounded-[calc(var(--radius-md)-1px)] -> built calc(var(--radius-md) - 1px) normalized source border-radius: calc(var(--radius-modal)-8px) -> built calc(var(--radius-modal)-8px) verbatim So the TSX scanner keeps a lenient `isWhitelistedTailwindCalc` (which also accepts `_`-escaped spaces) while the CSS scanner is strict. Same rule, two languages; the token allowlist and shrink-only constraint apply to both. Found while landing #1511, which had to route around the contract by spelling a shrink as its tier token. Gates: 2858 desktop tests green, biome clean, 0 dead CSS. * fix(design-system): close the last loose copy of the calc spacing rule Review of this PR found that `radius-nesting-contract.test.ts:63` is a SECOND gate on the same declaration `search-modal.css` shipped broken, and it still matched `\s*-\s*` — so after tightening the converge contract it became the only remaining place that would accept the unspaced spelling CSS drops as a parse error. Two gates on one site are only worth having if they agree. Also corrects that entry's label: the selector lives in `styles/search-modal.css`, not `sidebar.css`. The label is what the failure message points a future reader at, so a stale one sends them to the wrong file. Gates: 2874 desktop tests green, biome clean.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Four control-level defects reported from the settings surface, plus a plan for the permission-onboarding work.
1. Interface language control broke in English
.maka-segmentedcarriedflex-wrap: wrap, so any locale whose labels ran wider than the row folded an option onto a second line. English hit it — "Follow system / 中文 / English" is ~25% wider than the zh labels, and "English" dropped below the track, turning one switch into a ragged two-row plate.Measured over the
settings-general(en) fixture: 50px tall on two baselines → 26px on one.The track now refuses to wrap or shrink; the label stack beside it already carries
min-width: 0and absorbs the squeeze, and below the 460px card container the whole row stacks as before.2. Segmented radius was off the design scale
The track sat on
--radius-pill(999px) while every field and select it shares a card with sits on the 6/8/12 scale, so it read as a foreign shape. Track →--radius-surface(8px), segment buttons →--radius-control(6px), concentric across the 2px track padding. This lands on all fiveSegmentedconsumers, including the usage-stats range selector.Deliberately not the
calc(var(--radius-*) - Npx)shrink form: the radius-converge contract splits corner values on whitespace, so it only admits the unspacedcalc(var(--x)-2px)— which is invalid CSS (the spec requires whitespace around-) and computes to0px. Verified in the renderer. Filed separately;search-modal.css:32currently ships that bug and renders square corners.3. 默认权限模式 was unclickable — root cause
Base UI renders Select popups
position: staticinside an absolutely-positioned positioner, so thez-[var(--z-overlay)]onSelectPopupwas inert — a static box ignores z-index. What actually kept settings selects above.settingsModal(z-index 35) was the.settingsSelectPositionerclass, applied by hand per call site.PermissionModeSelectwas the only Select that never got it, so its popup painted under the modal: in the DOM witharia-expanded="true", invisible, and unclickable because the modal won every hit-test.The layer now lives on wrapped
SelectPositioner/PopoverPositioner, so consumers get it by construction.z-index-contractgrows a case pinning both halves — positioners must carry the layer, popups must not.Verified with trusted CDP input, which respects paint order: before, clicking an option left the trigger on 询问权限; after, it commits 自动执行. Synthetic
el.dispatchEventbypasses hit-testing and passes against a popup nobody can click — worth knowing for future UI checks.4. Daily-review execution time
Two problems. The field inherited the Input primitive's
w-fullunder a flat 140px cap, so08:00plus the glyph (~80px of ink) left a ~60px dead strip inside its right edge — 140px → 94px, hugging its content on the same right rail as the switch rows.And the popup was WebKit's: platform-accent columns, platform metrics, no dark mode, and unreachable by
::-webkit-*. NewTimePickerprimitive owns it — same field chrome as its neighbours, two columns (hours, minutes on a 5-min grid), and the sameHH:MMvalue contract, so the persisted config shape is unchanged.Two ordering hazards, both found by measurement: Base UI mounts the popup before measuring it (every rect reads 0, so a naive scroll write commits 0 — indistinguishable from "already at the top"), then moves focus into it, whose scroll-into-view overwrites whatever was set. Centring is gated on
onOpenChangeComplete, and initial focus is pinned to the selected hour rather than row 00.Also
docs/permission-onboarding-plan.md— a plan for the Codex-style drag-to-grant TCC onboarding. The load-bearing finding: the whole flow is stock Electron (type:'panel'+focusable:false+showInactive(), andwebContents.startDrag({file, icon})for the drag) except locating the System Settings window, which needsCGWindowListCopyWindowInfo— the one permission-free source, avoiding the chicken-and-egg of using an Accessibility-gated API to request Accessibility. Staged so Stage 1 ships with no native code. Technique only; no vendor code copied.Gates
2855 desktop + 250 ui tests green · biome clean · 0 dead CSS · contracts re-pinned to the new intent rather than deleted.