Keep the Button press nudge from clobbering caller translates

[imnasnainaec]

Summary

Button's pressed style nudges the button down 1px with tw:active:not-aria-[haspopup]:translate-y-px. Every Tailwind v4 translate utility writes the same --tw-translate-y custom property and then sets the translate shorthand from it, so the nudge replaces a caller's translate rather than adding to it — and :active outranks the caller's unprefixed utility. A Button centered with tw:top-1/2 tw:-translate-y-1/2 — a common way to place an icon button inside an input — therefore jumps down half its own height for as long as it is held.

Compiled both utilities with this repo's Tailwind to confirm the collision:

.tw\:-translate-y-1\/2 {
  --tw-translate-y: calc(calc(1 / 2 * 100%) * -1);
  translate: var(--tw-translate-x) var(--tw-translate-y);
}
.tw\:active\:not-aria-\[haspopup\]\:translate-y-px {
  &:active { &:not(*[aria-haspopup]) {
    --tw-translate-y: 1px;
    translate: var(--tw-translate-x) var(--tw-translate-y);
  } }
}

Moving the nudge to the transform property fixes it for every caller at once: per css-transforms-2 the browser applies translate, rotate and scale first and transform after, so the two compose instead of one overwriting the other. Callers get centering-plus-press with no workaround and no comment explaining one. tw:transition-all is already in the same base string, so the press animation is unchanged.

Why review this

Not part of the current epic. Found while reviewing sillsdev/interlinearizer-extension#214, where two icon buttons jumped mid-click and were worked around downstream — the cause is here, and every consumer that centers a Button with a translate will hit it independently. lib/platform-bible-react/src/stories/shadcn-ui/tooltip.stories.tsx:989 is a live instance in our own Storybook, so we currently demonstrate the broken idiom to anyone copying the pattern.

Changes

• button.tsx — tw:active:not-aria-[haspopup]:translate-y-px → tw:active:not-aria-[haspopup]:transform-[translateY(1px)], with a // CUSTOM: annotation that names what still collides (see the tradeoff note below). The original line arrived unannotated with the preset apply in a61ca3b, so without the annotation the next shadcn apply would silently revert this.
• button.tsx — annotated the pre-existing --radius-md → --tw-radius-md renames in the xs/sm/icon-xs/icon-sm variants, which came in unannotated with 5525683. src/index.css imports the theme with prefix(tw), which emits @theme variables prefixed, so the boilerplate's unprefixed name resolves to nothing and those sizes lose their rounding entirely. Same reasoning as above: unannotated, so a preset apply reverts them silently.
• button.test.tsx (new) — two tests, one per silently-revertible customization: the nudge stays on transform and is not an active:-modified translate utility, and the clamped sizes keep var(--tw-radius-md). Both fail when the corresponding customization is reverted (checked).
• button.stories.tsx — new PressNudgeWithCallerTranslate story: an icon button centered with tw:-translate-y-1/2 inside the package's own Input, the exact idiom the old nudge broke.

No call sites needed changing: callers that center with translate utilities are fixed by this, and the one -translate-y-1/2 clear-button in platform-scripture's find component is a raw <button>, not this one.

Testing

• npm run typecheck in lib/platform-bible-react — passes
• prettier --check on the changed files — passes (shadcn-ui/ is ESLint-ignored)
• Recompiled the prefixed class with the repo's Tailwind and confirmed it emits transform: translateY(1px) and no longer touches --tw-translate-y
• vitest --project=unit button.test.tsx — 2 passed; re-run with both customizations reverted to confirm both fail
• vitest --project=storybook button.stories.tsx — 17 passed, including the new story
• Visual check still wanted: open Shadcn/Button → PressNudgeWithCallerTranslate, hold the clear button, confirm it stays vertically centered and still nudges 1px

Notes for the reviewer

• The tradeoff this makes. Compiling against the repo's Tailwind: translate-* writes translate, scale-* writes scale, and plain rotate-* writes rotate — all separate properties, so all three now compose with the nudge. What writes transform itself, and so still collides with it, is skew-*, rotate-x/y/z-*, the transform/transform-gpu/transform-[…] utilities, and inline style={{ transform }} (which now wins, since inline styles beat classes). No Button in this repo carries any of those. That is the right way round — the common idiom composes and the rare one loses — and the // CUSTOM: block records it. One second-order effect: because the individual properties are applied first, the 1px is measured in the button's own space, so a caller rotate-* or scale-* would rotate or scale the nudge.
• Not rebuilding dist/ here; happy to add a rebuild if the PT team wants this out to consumers with this PR rather than the next build.
• interlinearizer's ArcOverlay works around the old behavior downstream and should move back to translate utilities once this lands.

Why the tests are unit tests and not a play function

The pressed geometry is the thing worth testing, and a browser play function looks like where it could run for real — the storybook vitest project runs stories in Chromium and this file is already in it. It cannot work: Storybook's play-context userEvent dispatches synthetic events, and :active is a browser input state that untrusted events do not set. Measured it — a [MouseLeft>] press in that project leaves button.matches(':active') === false and the position delta at exactly 0, with or without the fix, so the assertion would be non-falsifiable in the direction that matters. (@storybook/addon-vitest imports only server from @vitest/browser/context, not its CDP-backed userEvent.) Real geometry coverage would need a browser-mode test outside the storybook project, which is a new vitest project and out of scope.

What is left to assert deterministically is the class itself, and jsdom does that faster and without Playwright. So the split is: unit tests pin the classes, story is the human check for the geometry.

Risk Level

Low — one utility class, no API change. Worst case is the 1px press nudge not rendering for some caller, which is cosmetic and visible in Storybook.

AI Involvement

AI-assisted. Claude diagnosed the custom-property collision, verified it by compiling both utilities against the repo's own Tailwind, wrote the class change and the // CUSTOM: annotations, and added the tests and story. Opened as a draft pending my own review of the diagnosis and the visual check above.

Devin review: https://app.devin.ai/review/paranext/paranext-core/pull/2667

---

This change is

[imnasnainaec]

🤖 [Claude AI Review] — PR #2667

Reviewer panel: security, scope, ux, tests, clarity (default panel, auto-narrowed — no C# files, so contracts and architecture were dropped).

The diagnosis in the PR body is correct and I re-verified the mechanism independently: translate-* compiles to the translate property and transform-[…] to transform, the browser applies translate before transform, and the two now compose. The common case (Button with no caller translate) is visually identical — same 1px offset, and both properties establish a stacking context and a containing block, so nothing changes there. tw:transition-all covers both properties, so the press animation is unchanged.

Two findings were verified by running things rather than reading them, and both hold.

Perspective	Blocking	Warning	Suggestion
Security	0	0	0
Scope	0	1	3
UX	0	1	3
Tests	0	1	3
Clarity	0	0	5
Total distinct	0	3	8

Five findings needing a decision are posted inline. The rest are below.

Headline: the fix does not reach any consumer without a dist/ rebuild

This is the inline comment on button.tsx:22, repeated here because it decides whether the PR ships a fix or a no-op. dist/ is deliberately committed (.gitignore:6), and dist/index.js still contains translate-y-px (1 hit) and zero hits for translateY(1px). The PR body's justification for skipping the rebuild — "the committed build is already stale for this file" — does not hold: the last pre-branch commit touching PBR source (824065f) changed only *.stories.tsx, which are not part of the lib build, and the last commit that did change bundled source (ca07cca) shipped a rebuilt dist/ in the same commit. dist was in sync at the PR base. Since no in-repo product code centers a Button with a translate utility, the entire practical benefit accrues to out-of-repo consumers, who can only consume dist/.

---

Informational findings

🤖 Claude AI · [Scope] [UX] [Clarity] (suggestion) — button.tsx:16-21

Three overlapping observations on the new // CUSTOM: block, pulling in opposite directions — worth deciding as one.

It under-claims one thing: the comment explains the collision that was removed but not the one that was introduced, and the new one is subtler. In Tailwind v4, rotate-* and skew-* write the transform property, not a dedicated one — the compiled CSS confirms it (transform:var(--tw-rotate-x,) … var(--tw-skew-y,)) — whereas translate-* compiles to translate: and scale-* to scale:. So the fix trades a clobber of caller translate-* for a clobber of caller rotate-*/skew-*/transform-gpu/arbitrary transform-[…]. Second, because CSS applies translate/rotate/scale before transform, the nudge is now expressed in the element's local space: a Button with tw:rotate-90 would nudge sideways instead of down, one with tw:scale-50 would nudge 0.5px. Neither was true of the old nudge.

No caller in the repo hits any of this today — the only tw:transform uses are bare, on non-Button icons (search-bar.component.tsx:61, direction.stories.tsx:102,112), and no Button carries scale-*/rotate-* — so the trade is a clear net win. But the constraint is now invisible at the call site, and the // CUSTOM: block is the durable record a future shadcn apply reviewer will consult.

It over-explains another: at 6 lines this is the longest // CUSTOM: block in the whole shadcn-ui/ directory (every other one is 2-4 lines, with a single 5-line outlier in alert.tsx), and it says the same thing twice — the concrete repro ("jumped down half its own height while held") and the closing mechanism sentence ("transform is a separate property that the browser applies after translate") are two statements of one fact. "so the two now stack" also reads as diff narration rather than a description of the code as it stands.

Suggested resolution: cut the doubled explanation, spend the reclaimed line naming what composes (translate, scale) and what does not (rotate, skew, transform-*, inline style transform). Net length roughly unchanged, strictly more information.

Cross-ref: context-menu.tsx:76 — comparable non-obvious CSS fix annotated in 2 lines including the ticket number (PT-3877)

---

🤖 Claude AI · [Scope] [Tests] (suggestion) — button.stories.tsx:303

The success criterion — "pressed Button stays vertically centered while held" — has no automated verification, and the PR's framing that the geometry "could not run for real" is stronger than the constraint actually is.

The repo already runs stories against real Chromium: vitest.config.ts:38-60 defines a storybook project with browser.provider: 'playwright', this file's meta carries tags: ['autodocs', 'test'], and eight play-driven stories in this very file already run there — two of which already assert Button classes. So the browser lane exists and this component is already in it.

The cited Testing-Guide caution ("Avoid Animation-Sensitive Assertions in Story Interaction Tests") is accurate for the naive approach — toHaveStyle mid-transition would flake — but it is a caution about how to assert, not proof that the geometry cannot be asserted. A held press is reachable via userEvent.pointer({ keys: '[MouseLeft>]', target: button }), and the transition race has a deterministic seam rather than a sleep: await Promise.all(button.getAnimations().map((a) => a.finished)) before reading getBoundingClientRect(). The signal is also enormous rather than marginal — with the bug the icon-xs button drops ~13px, with the fix ~1px — so the assertion would not be tuning-sensitive.

Not arguing this must land in this PR; the class-string tests do catch the regression the PR is most worried about, and the story's docs string honestly states it is the manual check. Raising it so the gap is a deliberate choice rather than an assumed impossibility.

---

🤖 Claude AI · [UX] (suggestion) — button.stories.tsx:313

The story renders an enabled button whose accessible name promises an action ("Clear search") but which has no onClick, next to a readOnly input that gives no visual signal that typing is refused. Keyboard and screen-reader users on the autodocs page can Tab to the button, activate it, and get no feedback — a broken affordance in a page that documents the component. fn is already imported at line 2, so onClick={fn()} costs nothing; alternatively drive defaultValue from state and have the button clear it, which also makes the demo match the real SearchBar behavior. Keep the input editable either way — :active on the button is unaffected by input interactivity, so nothing about the pressed-state demo is lost.

The other display-only stories in this file (VariantsDemo, SizesDemo) read as static swatches; this one reads as a working search field, which sets the expectation that it works.

---

🤖 Claude AI · [UX] [Clarity] (suggestion) — button.stories.tsx:325-326

The story description is a single ~500-character, five-sentence paragraph where every other docs.description.story in this file is one short sentence (lines 53, 74, 104). It mixes three audiences: the reader instruction ("Hold it down: it should nudge 1px and stay vertically centered"), the CSS rationale, and the test-strategy note. The one line a docs reader actually needs is buried in the middle. Storybook renders these as markdown, so a lead sentence plus a short bulleted "why" would read far better.

Two specifics: sentence three is the third copy of the --tw-translate-y explanation in this PR, and sentence five names button.test.tsx, which in turn names this story at button.test.tsx:17 — a circular cross-file pair that goes stale silently if either file is renamed. The pointer from the test to the story is the more useful direction of the pair.

Separately, the claim that tw:-translate-y-1/2 is "the standard way to center an icon button inside an input" overstates repo convention: the package's own SearchBar centers its clear button with tw:inset-y-0 tw:my-auto (search-bar.component.tsx:84), not a translate. Softening to "one common way" avoids steering extension authors — or a future SearchBar refactor — toward the pattern that triggered this bug.

---

🤖 Claude AI · [Tests] [Clarity] (suggestion) — button.test.tsx:24-25

Of the two assertions in test 1, line 25 is the durable one and line 24 is the brittle one. Line 25 encodes the actual rule ("the nudge must not be an active:-modified translate utility"); line 24 pins one exact spelling of the fix. A behavior-preserving rewrite — transform-[translate(0,1px)], an added !, a different modifier order — breaks line 24 while the pressed geometry is unchanged. Line 25 already catches the stated shadcn apply revert on its own (confirmed by the revert run); line 24's only independent detection value is "nudge deleted entirely", which a positive regex (expect(className).toMatch(/active:\S*transform-\[translateY/)) would catch just as well while surviving cosmetic rewrites.

The regex on line 25 is correct in both directions — verified, not eyeballed. Against the reverted class tw:active:not-aria-[haspopup]:translate-y-px: active: matches, \S* spans not-aria-[haspopup]:, then translate- matches. Against the current class it does not match, because transform-[translateY(1px)] contains translateY( and never the literal translate-. \S* cannot cross whitespace, so it is confined to one class token. No false positive today.

But it passes only because of that one-character distinction — the trailing hyphen in translate- not matching the capital Y in translateY( — which is easy to misread as a bug, and which the next editor could silently break by "simplifying" the pattern to /active:\S*translate/. A short note ("the trailing hyphen is load-bearing") would make it legible. The 12-line block above the test explains the CSS mechanism but says nothing about how this assertion is constructed.

---

🤖 Claude AI · [Clarity] (suggestion) — button.test.tsx:8-13

The first paragraph of the test's comment block restates the mechanism already explained in button.tsx:16-21 — shared --tw-translate-y, the caller's centering being replaced, transform composing. That is the second of three copies of the same explanation in this PR (the third being the story description). The second paragraph (lines 15-18) is the part that earns its place: the jsdom limitation and why the assertions pin a class rather than geometry is reasoning the test code cannot show. Dropping the first paragraph leaves the test comment saying only what is true of the test.

It would also bring the block in line with the sibling test file, where each it carries its own 3-4 line note rather than one block covering both. As written the block sits above the first it but describes both, so it reads as a comment on the first test only.

Cross-ref: context-menu.test.tsx:49,69 — per-it comments, each scoped to the case it precedes

---

_{Generated by /review-pr. Security review found nothing. AI review is advisory — human approval still required.}

[imnasnainaec]

@imnasnainaec reviewed all commit messages and resolved 5 discussions.
Reviewable status: 0 of 3 files reviewed, all discussions resolved.

[katherinejensen00]

Thanks for fixing this button bug, Danny! Just one small thing to look at and then you are all set. Thanks for doing so many rounds of review already

@katherinejensen00 reviewed 3 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on imnasnainaec).

---

lib/platform-bible-react/src/stories/shadcn-ui/button.stories.tsx line 310 at r2 (raw file):

      <Button
        aria-label="Clear search"
        className="tw:absolute tw:inset-e-1 tw:top-1/2 tw:-translate-y-1/2"

tw:inset-e-1 in the new story is not a real Tailwind utility — the clear button won't be positioned at the input's end. · Low

The whole shadcn-ui/ directory positions inset-inline-end with tw:end-* (e.g. tw:absolute tw:top-2 tw:end-2); there are zero inset-e-* usages in the repo, and Tailwind v4 has no inset-e-* (it's end-*). So tw:inset-e-1 generates no CSS: with only tw:absolute tw:top-1/2 tw:-translate-y-1/2 taking effect, the X renders at the container's start edge, not inside the input's end. The vertical-centering-under-press behavior the story exists to demonstrate still shows, so it's cosmetic — but this story is explicitly the "correct idiom to copy," and the PR's own rationale is that we shouldn't "demonstrate the broken idiom to anyone copying the pattern." Change to tw:end-1.

[imnasnainaec]

@imnasnainaec reviewed all commit messages and made 1 comment.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on katherinejensen00).

---

lib/platform-bible-react/src/stories/shadcn-ui/button.stories.tsx line 310 at r2 (raw file):

Previously, katherinejensen00 wrote…

tw:inset-e-1 in the new story is not a real Tailwind utility — the clear button won't be positioned at the input's end. · Low

The whole shadcn-ui/ directory positions inset-inline-end with tw:end-* (e.g. tw:absolute tw:top-2 tw:end-2); there are zero inset-e-* usages in the repo, and Tailwind v4 has no inset-e-* (it's end-*). So tw:inset-e-1 generates no CSS: with only tw:absolute tw:top-1/2 tw:-translate-y-1/2 taking effect, the X renders at the container's start edge, not inside the input's end. The vertical-centering-under-press behavior the story exists to demonstrate still shows, so it's cosmetic — but this story is explicitly the "correct idiom to copy," and the PR's own rationale is that we shouldn't "demonstrate the broken idiom to anyone copying the pattern." Change to tw:end-1.

Changed to tw:end-1 in a5a7fc5 — agreed the story should show the idiom the components actually use.

One clarification: tw:inset-e-1 is a real utility. Compiled against our pinned Tailwind 4.2.2, both emit identical CSS:

.tw\:end-1     { inset-inline-end: calc(var(--tw-spacing) * 1); }
.tw\:inset-e-1 { inset-inline-end: calc(var(--tw-spacing) * 1); }

So the clear button was positioned correctly before this change. It's also already in use — tooltip.stories.tsx:981 (inset-s-3) and :992 (inset-e-0).

Worth a decision: Tailwind's language server treats inset-e-* as canonical and now warns on tw:end-1 — that hint is where the inset-e-1 came from in the first place. Nothing in CI or the AGENT files enforces either direction. Settle on end-* as house style, or follow the tooling?

(drafted by Claude, modified by me)

[katherinejensen00]

@katherinejensen00 reviewed 1 file and all commit messages, and resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved.

[Sebastian-ubs]

Headline: the fix does not reach any consumer without a dist/ rebuild

Right. Without building platform-bible-react this will not reach any consumer