PT-4711/4712/4714: Fix three per-pane content zoom defects - #2841
Closed
rolfheij-sil wants to merge 198 commits into
Closed
rolfheij-sil wants to merge 198 commits into
rolfheij-sil wants to merge 198 commits into
Conversation
…Factor Delete the app-wide Ctrl+=/-/0 zoom chord branches from main.ts's before-input-event handler and the now-unused resetZoomFactor, freeing the chords for per-pane content zoom. Rewire zoomIn/zoomOut to step through adjustZoomFactor (clamp + one-decimal rounding) instead of raw float addition, fixing accumulated drift that eventually made the settings validator reject a legitimate step. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gkwyu1GZbkdGdPXaygcerV
Replace the View menu's bare role: 'viewMenu' entry with an explicit submenu so the native zoomIn/zoomOut/resetZoom roles no longer swallow Cmd+=/-/0 before the per-pane content-zoom commands see them. Adds hidden numpad-accelerator duplicates (Electron allows only one accelerator per item) and the three new View-menu labels to en/es localization. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gkwyu1GZbkdGdPXaygcerV
…erlays Adds hasAnyDialogRequest() to the dialog service shard and hasOverlayOfType() to the overlay store, composed into a single isAnyDialogOpen() query in a new dialog-open.util.ts. A "dialog open" is either a live docked PAPI dialog request or an active modal overlay. The renderer window-chrome key listener (a later task) uses this to no-op content-zoom chords while a dialog has focus. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gkwyu1GZbkdGdPXaygcerV
Registers a per-window keydown listener that turns Ctrl/⌘+`+`/`-`/`0` into content-zoom actions for the window's active tab and area, covering the case where keyboard focus is on the window's own chrome (tab headers, reference box, toolbar buttons) rather than inside a web view's iframe, which the in-view bootstrap script already handles. No-ops while a dialog is open and skips events targeting or inside an iframe, leaving those to the bootstrap. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gkwyu1GZbkdGdPXaygcerV
…the app-zoom entries The Zoom category still documented the removed main-process before-input-event chords (zoom-in/zoom-out/reset-zoom) and carried TODO(PT-4577) notes on the content-zoom entries about those chords shadowing them on Windows/Linux. Delete the stale app-zoom entries (platform.zoomIn/zoomOut no longer have a default chord), and rewrite content-zoom-in/out/reset to describe the current handling: the in-view bootstrap for focus inside a web view, the new window-chrome listener for focus on the tab bar/reference box (no-op while a dialog is open), and the macOS View menu's explicit accelerator - citing all four files that implement it. content-zoom-wheel and the unrelated Enhanced Resources zoom entries (a self-contained per-web-view listener, unaffected by the chord move) are left as they were. Adds a small regression test pinning the Zoom category's shape and checking every one of its location paths resolves to a real file on disk. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gkwyu1GZbkdGdPXaygcerV
…ents Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gkwyu1GZbkdGdPXaygcerV
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gkwyu1GZbkdGdPXaygcerV
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gkwyu1GZbkdGdPXaygcerV
… parity test Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gkwyu1GZbkdGdPXaygcerV
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…anguage Settings bullet Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ent zoom Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… PT9 index row Correct two forward claims in the content-zoom ADRs: adr-zoom-composition overstated PT-4582's scope for the Text Collection grid's per-resource zoom (it marks the grid pane's own area and leaves the per-resource zoom in place; moving it onto the platform mechanism is not scheduled), and adr-per-web-view-state-lives-in-the-definition overstated PT-4585 (it decides whether to prune the memory setting at all, not how). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… the area docs Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…anded Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… of a cast Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…talog, notes and ADR for the Simple tab menu Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…pporter settings get their own group Reorders the General settings group so visible settings read interface language, zoom factor, content zoom, then the hidden book-keeping entries. Interface mode is now hidden, since the Simple/Power toggle lives in the profile popover and a Settings entry would be a redundant second switch. Request timeout and the registration reminder move into a new Supporter settings group, for settings a support person adjusts rather than day-to-day translation settings. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…a decimal Add PercentStepper, a +/-/reset control that displays a factor as a percentage and clamps every step to the caller's own min/max/step rather than the shared zoom constants, so callers with different bounds don't get a control that silently ignores them. Optimistic display keeps two quick presses from racing the stored value's round-trip through the platform's subscription. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ecimal factor Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…o a tenth Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ent wording for the zoom stepper Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ses are confirmed Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…presses step from the last emitted factor Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… no bootstrap Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…reset Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Typing \ in the Scripture editor's Standard view opens a command palette the platform renders in the renderer's own document, outside the web view, so it stayed at interface size inside a zoomed pane. The same held for the Enter palette, the footnote editor's palette, and every popover and context menu a pane asks the platform for. Placement was already correct; only the drawing scale was not. Anchored overlays now take the requesting pane's content scale, with their width and height capped by the space Radix reports divided by that scale, so a zoomed pop-up stays inside the window. Centred palettes and modal dialogs are not anchored to content and keep interface scale. The anchor's size is also multiplied by the frame zoom, the way its position already was, so a pop-up from a whole-frame-scaled pane is placed against the size the trigger really has on screen. A caller's own explicit maxWidth/maxHeight now combines with the zoom cap via CSS min() once the pane is zoomed, instead of replacing it outright, so an explicit cap can no longer let a zoomed overlay grow past the window. At interface scale the caller's value is used exactly as before. Every new type-visible member reaching papi.d.ts (OverlayContextMenuPresentationalProps.contentScale, and contentZoomOverlayStyle itself) carries the standard @experimental marker; papi.d.ts is regenerated to match. OverlayContextMenu is part of the generated declarations (pre-existing), so its earlier direct import of the content-zoom service dragged the whole service onto the extension-facing surface — three whole modules, including two test-only seams (__setContentZoomDepsForTesting, __flushContentZoomWritesForTesting) that must never be public API. OverlayHost — not part of the generated declarations — now reads getContentZoomScaleForWebView and getWebViewContentScale once per overlay and passes the results down as plain contentScale/frameScale props; none of the three overlay components imports the zoom service anymore. papi.d.ts no longer declares shared/models/content-zoom.model, shared/utils/content-zoom.util, or renderer/services/web-view-content-zoom.service, and the two test seams no longer appear anywhere in it. getWebViewContentScale and getContentZoomScaleForWebView keep the experimental tags added for the earlier leak, since they describe those helpers' stability regardless of whether the bundler currently emits them. The new OverlayContextMenuProps.contentScale field this introduces on the connector itself is tagged the same way. The connector-wiring coverage lives in two places, split by what each seam owns. overlay-host.component.test.tsx covers the host's OWN read-and-pass-down: one test per overlay kind asserting contentScale (mocking getContentZoomScaleForWebView), plus a separate pair of tests for popover and command palette asserting frameScale independently (mocking getWebViewContentScale, with a value distinct from the content scale so the two props cannot be silently swapped or one silently dropped). Each of the three store-connected components additionally has its own connector-level test — rendering the real component directly with explicit contentScale/frameScale props and no service mocks at all, since none of them reads a service anymore — proving its own forwarding into its presentational component. OverlayCommandPalette's other store-connected tests exercise the connector for unrelated reasons (filter mirroring, selection clamping, resolve/ dismiss); none of them pinned this forwarding, which is why it gets its own case alongside them, driving the anchored branch with a sized request.anchor so both scales are exercised together. The three store-connected components (OverlayPopover, OverlayCommandPalette, OverlayContextMenu) are plain pass-throughs for the scale props they receive. Two mocks in overlay-command-palette.commit-equivalence.test.tsx (a getWebViewContentScale stub and an onDidDisconnectClient stub whose comment named the content-zoom service's settings read) were dead once the connectors stopped reading that service directly; removed, confirmed by running the suite with both gone. Each case proven to bite by neutering the style helper, by dropping the caps, by applying the style to the centred branch, by dropping the anchor multiplication, by reverting the conditional-spread guard to plain keys, by adding an accidental zoom to the modal dialog shell, by deleting each overlay kind's contentScale/frameScale prop from OverlayHost's JSX, and by deleting each connector's own forwarding into its presentational component (each of the three, one prop at a time). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyKxUPr99ahZUQZgSBuBBG
…ed them The pop-up zoom rule and the extension-author guidance both described pop-ups rendered inside a web view only. They now also cover the ones the platform renders outside it on a pane's request, which reach the pane's scale through the zoom service rather than a CSS variable they cannot see. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyKxUPr99ahZUQZgSBuBBG
jsdom implements no CSS zoom, so the unit cases can only assert what is handed to the pop-up. This one opens the palette at 100% and at 200% and compares the boxes, and checks it stays inside the pane. Proven to bite by reverting the overlay change and watching the growth assertion fail. Live run also confirmed the Radix arrow is badly displaced at 150% and 200% zoom (pinned to the content box's own edge instead of tracking the trigger) — reported with measurements and screenshots in the task report, not fixed here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyKxUPr99ahZUQZgSBuBBG
Radix positions Popover.Arrow by writing a raw pixel offset onto its own wrapper; a wrapper inside a CSS-zoomed element has that offset re-scaled by the browser on top of Radix's own (already zoom-aware) computation, so the arrow lands off its trigger — collapsing to the content's own corner at 150% and 200%. Move the zoom and all sizing (width/maxWidth/maxHeight) onto a new inner div that wraps only the palette/popover content, leaving Arrow as PopoverContent's other, unzoomed child. While moving the sizing, fix the default-cap defect it carried: the zoom cap combination was gated on the CALLER having passed an explicit maxWidth, so a zoomed pop-up with no caller cap lost its own 320px/500px default entirely and stretched to the full available width. Gate on the zoom cap alone and combine it with the resolved (caller-or-default) value instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyKxUPr99ahZUQZgSBuBBG
Asserts the marker palette's arrow stays centred on its trigger at 100%, 150% and 200% content zoom. Uses "that great city" rather than the existing "Yahweh" trigger, since a trigger near the content's own left edge cannot distinguish a correct offset from one collapsed to zero. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyKxUPr99ahZUQZgSBuBBG
Radix requires a pop-up's arrow to be a descendant of its content element and positions it by writing an inline pixel offset. Inside a zoomed element the browser reads that offset as a pre-zoom length and scales it again, so the arrow missed its trigger by offset x (scale - 1) -- and where the true offset was small, the doubled value tripped floating-ui's arrow clamp and collapsed onto the content's own corner. Also corrects the size-cap sentence: the cap combines with whichever bound is actually in force, so a zoomed pop-up with no caller-supplied cap keeps its own default instead of losing it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyKxUPr99ahZUQZgSBuBBG
The write had no reader (a stored `false` and an absent key both resolve `getInitialContentZoomForWebView`'s expectation to "marks none") and could undo a sibling pane's already-recorded `true`: a pane opened before that write landed resolves `false`, reports no area, and its own grace expiry later overwrites the type record with `false`, even though the type does mark areas. Deleting it removes the only path that could downgrade a type record. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyKxUPr99ahZUQZgSBuBBG
getWebViewContentScale (whole-iframe CSS zoom only, 1 for a pane that marks zoom areas) and the content zoom service's getContentZoomScaleForWebView (the scale a pane's content actually draws at) read as near-synonyms picked by name; rename the former to getWebViewIframeZoom and point its TSDoc at the other. Extract the iframe-zoom parse both functions duplicated into a shared parseIframeZoom helper in overlay-coordinates.ts, called by both, so they cannot drift; the service still reaches its iframe through deps.getIframe rather than calling getWebViewIframeZoom directly, keeping the existing test seam. parseIframeZoom is new export surface reaching papi.d.ts, so it carries the same @experimental tag as the renamed function. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyKxUPr99ahZUQZgSBuBBG
…ments Tag OverlayPopoverPresentationalProps/OverlayCommandPalettePresentationalProps' contentScale/frameScale and their connector props' contentScale/frameScale with @experimental, matching the sibling OverlayContextMenuPresentationalProps.contentScale that already carries it — the container-level tag does not reach IntelliSense on extension .d.ts contracts, so each member needs its own. Rewrite comments that only narrated how the code reached its current state ("no longer reads any service", "stopped reading it themselves", "exactly as before content zoom existed") to state the current contract instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyKxUPr99ahZUQZgSBuBBG
The deleted case asserted the dialog carries no `style.zoom`, but the shell never sets up a zoomed pane, takes no scale prop, and OverlayHost never reads one for a modalDialog overlay — there is no zoom read anywhere in this render path to mock, so the assertion was trivially true regardless of what the code does. Recorded the deliberate absence as a doc comment instead: a full-window modal is not anchored to the requesting pane's content, so it is not expected to scale with it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyKxUPr99ahZUQZgSBuBBG
- Drop the hard-coded measurements (~560px, ~553px, ~569px, "16px on every
side") from expectBesideTriggerAndInsideWindow's TSDoc; they depend on
window size, font metrics and trigger position and go stale. Keep only the
invariant: the palette is portalled to the main document, so Radix
collision-avoids it against the window, not the pane.
- Name the "close to the trigger" 24px tolerance as a module-level constant
(MAX_PALETTE_TRIGGER_GAP_PX) with a one-line rationale, rather than an
unexplained literal. Named as a constant rather than derived from the zoom
factor: the doc comment already establishes the gap is fixed and
zoom-independent.
- Note that openPalette's trigger ("Yahweh") deliberately moves between zoom
levels, since the text it comes from can span more than one line and
reflows — the caret box is re-read on every open rather than reused.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GyKxUPr99ahZUQZgSBuBBG
…ht cap Three coverage gaps the reviews turned up, plus a rename. The modal dialog deliberately does not follow the requesting pane's zoom, but nothing pinned that: the only test of it read an element the host hands no scale to, so it could not fail. The mock now echoes both scale props -- as the anchored stubs already did -- so the case reads the host's behaviour rather than the stub's indifference, and passing either prop to the modalDialog branch reds it out. The popover's height cap was covered only in its default form, leaving the caller-supplied branch the cap fix introduced with nothing holding it. The arrow tolerance was named for a width; it measures an X position. Proven to bite, each mutation failing its own case alone and leaving the rest of the file green: - passing both scales to the modalDialog branch: expected '1.5' to be '' - ignoring the caller's maxHeight: min(400px, ...) instead of min(360px, ...) - removing `width: 'auto'`: expected '' to be 'auto' Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyKxUPr99ahZUQZgSBuBBG
Picks up the iframe-zoom read's rename, its note that it does not cover per-area zoom, and the shared parse the content zoom service and the overlay coordinates both call. Generated by `npm run build:types`, never edited by hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyKxUPr99ahZUQZgSBuBBG
The previous wording claimed the pop-up "scales exactly as before" and described the shared class as one the sizing styles overrode from outside. Neither is true: that class supplies the width, the flex layout and the padding, and a pop-up's content renderers return fragments, so their children were direct flex items of it. Moving content inward leaves all of that behind at every scale, not only when zoomed -- so the entry now says the wrapper has to take the three over together, and why splitting them resizes the pop-up silently. Also restores the paragraph break the block was inserted over, which had joined the unrelated OverlayHost sentence onto the end of the arrow discussion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyKxUPr99ahZUQZgSBuBBG
Moving the popover's zoomed content onto an inner wrapper div left PopoverContent's shared tw:w-72 width and tw:gap-2.5/flex layout behind: width:'auto' on PopoverContent overrides the fixed width at every scale, so the popover now sizes to its content instead of a 288px default, and the title/body/actions (rendered as a fragment) lost their flex gap since they are no longer direct children of PopoverContent. Move the width and flex layout classes onto the inner zoomed div so they scale with the zoom like the rest of the content. The command palette's inner div already sets its own width explicitly and is unaffected; its width-comment is trimmed to state the same invariant without narrating what omitting it would do. Add assertions pinning the inner div's width/layout classes at scale 1 and at a zoom, alongside the existing outer-width assertion. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyKxUPr99ahZUQZgSBuBBG
platform.webViewContentZoomTypesWithAreas and its in-memory mirror's
doc said an unset write recorded a type marking no area and that the
record self-corrects in both directions; that write was removed, so
the record now only ever gains true entries and an absent key means
no evidence yet. Narrow both docs to match, and drop
recordTypeMarksAreas' now-dead marksAreas parameter (its only caller
always passed true) so the removed clobbering write can't be
reintroduced by a future caller passing false.
parseIframeZoom takes HTMLIFrameElement | null | undefined so its two
callers (both holding the null getWebViewIframe/getIframe return)
stop needing a `?? undefined` just to satisfy the signature.
getWebViewIframeZoom's TSDoc linked {@link getContentZoomScaleForWebView},
which is neither imported nor defined in that file (importing it would
create a cycle) and so never resolves; spelled out as plain code text
instead.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GyKxUPr99ahZUQZgSBuBBG
The arrow assertion only asserts the arrow's centre is within 2px of
the trigger, which also passes when the arrow has collapsed to the
palette's own left edge, at any zoom factor where the trigger happens
to sit near that edge — an unenforced comment about "about 118px at
100%" was the only thing standing between the check and a defect it
could no longer catch if layout drifted.
Add a precondition, computed from the trigger's and palette's own
geometry only (never the arrow), that the trigger sits meaningfully
away from the palette's left edge — so a collapsed arrow now fails by
construction. Wrap the arrow measurement and its assertion in a
`toPass` retry, since floating-ui's arrow middleware can still
reposition the arrow after the open animation ends.
Verified live: reverting the arrow out of the zoomed subtree into
PopoverContent itself reproduces the original defect shape (a raw
pixel offset re-scaled by an ancestor's zoom) — the fixed test passes
cleanly at 100% (no rescale at factor 1), and fails with a clean
per-assertion signal at 150% ("arrow centred on its trigger at 150%",
expected <= 2, received 65.84) and 200% ("...at 200%", expected <= 2,
received 209.86); the new precondition assertion passed at both
factors in the same reverted runs, so the arrow assertion is what
catches the regression, not the precondition failing for an unrelated
reason.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GyKxUPr99ahZUQZgSBuBBG
"used to provide" narrated the prior commit's change instead of the rule the assertion pins. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyKxUPr99ahZUQZgSBuBBG
The committed declarations still described the removed "marks none" write and claimed the type record self-corrects in both directions. Also picks up parseIframeZoom accepting null and the cross-reference that TSDoc could not resolve. Generated by `npm run build:types`, never edited by hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyKxUPr99ahZUQZgSBuBBG
translateCoordinates already holds the iframe it looked up, so it parses the zoom from that element instead of asking for the same element again by id. The area-types validator's null guard had no test reaching it: undefined is rejected a line earlier by the typeof arm, and typeof null is 'object', so only null exercises that branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GyKxUPr99ahZUQZgSBuBBG
rolfheij-sil
marked this pull request as ready for review
September 19, 2026 12:34
rolfheij-sil
requested review from
irahopkinson,
lyonsil and
tjcouch-sil
as code owners
September 19, 2026 12:34
rolfheij-sil
force-pushed
the
pt-4584-notes-content-zoom
branch
from
September 21, 2026 10:36
2672ef4 to
afa9e7e
Compare
2 tasks
rolfheij-sil
added a commit
that referenced
this pull request
Sep 24, 2026
…2844) Three follow-ups to per-pane content zoom (epic PT-4575), superseding #2841, #2842 and #2843. Resources views zoom their own content (PT-4582, PT-4583): the Text Collection grid (text-collection), Bible Texts (bible-texts), Commentaries (commentaries), Model Text (model-text) and Enhanced Resources (main, entries, footnotes) each mark their own areas and remember the level per project; selectors, ribbons, toolbars and resize handles stay at interface scale. Distinct area ids keep the resource panes apart, since their definitions all carry the container project. Enhanced Resources' private zoom (scripturePaneZoom state, keydown handler, menu items, strings and shortcut entries) is retired in favour of the platform mechanism. The Text Collection grid's Ctrl+wheel now uses the shared createContentZoomWheelReader in platform-bible-utils, so a trackpad pinch is measured as travel instead of a full step per frame; a parity test pins it against the platform's bootstrap copy. Zoom defects fixed: - PT-4711: the chords follow the pane the user clicked. - PT-4712: platform-drawn menus, context menus and the command palette follow the zoom of the pane that opened them. - PT-4714: a newly opened tab no longer flashes at the wrong size. PT-4713 is withdrawn: the Scripture editor's right-click menu stays at interface scale and no editor change is needed. A contract test pins which extension files may carry a ContentZoomRoot, so a nested (and silently inert) area cannot be added unnoticed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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 free
to 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.
Fixes three defects found by hands-on testing of per-pane content zoom.
A newly opened tab no longer flashes at the wrong size (PT-4714)
A tab whose view scales its own content used to open unscaled, then jump to the Settings default about a second later. The platform could not know, before the view's content mounted, whether that view scales itself — so it waited, and applied the whole-frame fallback only when the wait expired.
The platform now remembers, per view type, whether that type scales its own content, in a hidden setting. A view type that has scaled itself before is recognised on its next open and never gets the whole-frame fallback at all; every other view gets the fallback immediately, on the first frame, with no wait. The first-ever open of a view type nobody has opened before is the one case that still has to guess, and it guesses the way that is right for most views.
The record only ever gains entries in one direction — see Open question for review below.
The zoom chords follow the pane you clicked (PT-4711)
Clicking a footnote moves the caret into the footnote pane, but the view answers that click by moving the caret back to the main text. Ctrl+= / Ctrl+- / Ctrl+0 followed the caret, so they zoomed the main text instead of the footnote the user had just clicked.
The chords now distinguish a focus change the user made from one the view made in response, and stay on the pane the user chose. Ctrl+wheel was already correct — it follows the pointer.
Pop-ups drawn by the platform follow the pane's zoom (PT-4712)
The marker menu, context menus and the command palette are drawn by the platform outside the web view, so they stayed at interface scale inside a zoomed pane — small menus against large text.
Anchored pop-ups now draw at the scale of the pane that opened them. Centred surfaces that belong to the window rather than to a pane — modal dialogs, the window-wide command palette — deliberately do not, and a test now pins that.
Why the zoom sits on an inner wrapper
Radix requires a pop-up's arrow to be a descendant of its content element, and positions the arrow by writing an inline pixel offset on it. Putting the zoom on the content element therefore made the browser scale that offset a second time, landing the arrow well away from its trigger — and, where the true offset was small, collapsing it onto the content's own corner.
The zoom and all the sizing now sit on a wrapper inside the content element, with the arrow left outside that wrapper as a direct child of the content, which is what Radix requires. The pop-up still grows, because CSS
zoomon a child still grows its parent's layout box, and the arrow's offset is now read at face value.That wrapper has to take over width, flex layout and padding together. The shared
PopoverContentsupplies all three, and the pop-up's content renderers return fragments, so their children were direct flex items of it — taking only the padding inward silently resizes and respaces the pop-up at every scale, including 100%.PopoverContentkeepswidth: 'auto'so its fixed-width class cannot reassert itself over the wrapper.Open questions for review
The per-type record only ever gains "scales its own content". It never moves back, and a pane of such a type that scales nothing in its current state — a placeholder shown before a project resolves — stays at 100% while every other pane sits at the user's default, indefinitely.
This is reachable during ordinary startup: the Scripture editor's resource panels mark their zoom area only once content resolves, behind several earlier returns that mark nothing, and in Simple mode those panels mount in Column 3 before a project does.
This is deliberate and was decided rather than overlooked: wait forever is the only wait that never flashes, and any bounded wait trades a guaranteed no-flash for a bounded stale-at-100% window. The no-flash side was chosen. It is written up here so a reviewer can weigh it, not because it is unresolved.
Pop-ups are not bounded by their own pane. No overlay ties Radix's collision boundary to the requesting pane, so a platform-drawn pop-up collision-avoids against the window and can paint slightly outside the pane that opened it. This predates content zoom and reproduces at 100%; it is called out because zoom makes it easier to notice.
Hidden views
Nothing here is geometry-driven sync across views, so there is no hidden-tab catch-up to design. The zoom push writes CSS custom properties, the grace timer's expiry does a liveness probe, and the scale read is an inline-style read — all of which work correctly in a
display: nonepane. Pop-ups are drawn on demand from the visible pane.Verification
typecheck,lint,format:checkclean; core suite 5,636 passed; workspace suites green.tests/isolated/notes-content-zoom/2/2. E2Etests/isolated/scripture-editor/11/13 — all six content-zoom tests pass, including the clicked-footnote chord case. The two failures (formatted-default-simple-mode,paragraph-style-trigger-column-floor) fail identically on this branch's base, verified by a control run; both are blocked by the first-run onboarding tour covering the toolbar in simple mode.AI-assisted — session 1
🤖 Generated with Claude Code
https://claude.ai/code/session_01GyKxUPr99ahZUQZgSBuBBG
This change is
Coordination with #2842
#2842 (the editor's context menu following its zoom area) positions its menu by writing a position, reading back where the element actually landed, and correcting by the difference — so it does not predict an offset and is robust to which ancestor carries the zoom. This PR changes exactly that: the zoom moves from the Radix content element onto an inner wrapper.
That should be absorbed, but it has only been measured against the current arrangement, not this one. So: whichever of #2841 and #2842 merges second re-measures the footnote editor pop-up — a right-click inside the pop-up at 200% must put the menu at the pointer, checked by comparing the menu's rect against the click point, not by assuming the rebase was clean.
Worth knowing for that measurement: a
transformon an ancestor re-parents the containing block of aposition: fixeddescendant, and Radix's popper wrapper carries one. Nothing in this PR places a fixed-positioned element inside a pop-up — the popover's virtual anchor is a sibling outside the popper wrapper, and the arrow is positioned withleft/toprather thanfixed— but that is the shape to check for if either side adds one.Code Review Summary
Branch: pt-4711-content-zoom-defects
Base: origin/pt-4584-notes-content-zoom (stacked — PR #2825, which is itself stacked on #2821)
Date: 2026-09-19
Review model: Claude Opus 5 (1M context)
Files changed: 28
Overview
Three defects found by hands-on testing of per-pane content zoom, fixed together because they share
the same machinery.
PT-4711 — the zoom chords (Ctrl+=/-/0) followed the caret. Clicking a footnote moves the caret
into the footnote pane, but the view answers that click by moving the caret back to the main text,
so the chords zoomed the text the user had just navigated away from. The in-iframe bootstrap now
distinguishes a focus change the user made from one the view made in response, and keeps the
chords on the pane the user chose. Ctrl+wheel was already correct — it follows the pointer.
PT-4714 — a newly opened tab whose view scales its own content showed unscaled for about a
second, then jumped to the Settings default, because the platform waited out a grace period to
learn whether the view would scale itself. The platform now remembers that answer per web view
type in a hidden setting, so a type it has seen before is recognised before the pane's HTML is
built and never gets the whole-frame fallback at all; every other view gets the fallback on the
first frame with no wait.
PT-4712 — platform-drawn pop-ups stayed at interface scale inside a zoomed pane. They now draw
at the requesting pane's scale. This work introduced a regression and then fixed it: Radix requires
a pop-up's arrow to be a descendant of its content element and positions it by writing an inline
pixel offset, so CSS
zoomon that element made the browser scale the offset a second time. Fixedby moving the zoom and all sizing onto a wrapper inside the content element, leaving the arrow
outside it.
API Changes
papi-shared-types(feedsSettingTypes, visible through@papi/core/@papi/frontend'ssettings.get/set/subscribegenerics): new hidden setting keyplatform.webViewContentZoomTypesWithAreas: { [webViewType: string]: boolean }, tagged@experimental. Additive, non-breaking. Source and regeneratedpapi.d.tsagree.declare module '@papi/core','@papi/backend','@papi/frontend'or'@papi/frontend/react'block has a changed line.
@papi/renderer modules that happen to be in the declaration bundle gained additive,@experimental-tagged exports:contentZoomOverlayStyle(new file);parseIframeZoomandgetWebViewIframeZoominoverlay-coordinates(the latter replaces a module-privategetIframeZoomthat was never exported, so a new export rather than a breaking rename); a newoptional
contentScale?: numberon the context menu's props.lib/platform-bible-react/,lib/platform-bible-utils/and extension.d.tscontracts areuntouched.
Findings
Critical — Must address before merge
None.
Important — Should address before merge
None. All four analysis passes (API/correctness, style/architecture, coverage/compliance, UX)
returned zero Critical and zero Important findings.
Minor — Consider
nullbranch had no test reaching it —undefinedis rejecteda line earlier by the
typeof !== 'object'arm, andtypeof nullis'object', so onlynullexercises that guard. (fixed during review: addedvalidate(null, …)with a commentsaying why that input specifically is the one that reaches it)
translateCoordinateslooked the same iframe up twice — it held an iframe reference andthen called
getWebViewIframeZoom(webViewId), which re-queried the DOM for the same element.(fixed during review: parses the zoom from the element already in hand)
The memory and type-record settings pairs (
(Author: leave it. The pairs are small, alreadyreadMemory/readTypesWithAreas,enqueueMemoryTransaction/enqueueTypesWithAreasTransaction) are near-identical and couldcollapse into one generic implementation.
documented, and each half is independently tested; a generic helper parameterised by setting
key and cache accessors would be harder to read than the duplication it removes.)
The popover and command palette compute their zoom size cap with near-identical logic that(Author: leave it —could move into the existing shared
overlay-content-zoom.util.ts.same reasoning as above.)
(Left asplatform.webViewContentZoomTypesWithAreasis typed{ [webViewType: string]: boolean }while the record only ever gains
trueentries, so the invariant is enforced by prose ratherthan by the type.
booleandeliberately: the one-way behaviour is a decision thatmay be revisited — see Interview Notes — and narrowing a published type would have to be
undone if it is. No behavioural difference either way. A reviewer who disagrees should say so.)
Template Propagation
Shared Regions Modified
None — no
#region shared with <url>markers in any changed file.Extension Config Changes
None — no files under
extensions/were changed.Positive Observations
text recorded. The arrow regression check fails by 65.8px at 150% and 209.9px at 200% against a
2px tolerance when the fix is reverted, and passes at 100% where the zoom style is inert — the
100% pass is the control showing the test is not simply always red.
toEqual(new DOMRect(...))anywhere in the diff — that comparison examines nothing, becauseDOMRect keeps its values on the prototype.
pop-up's own edge, so a collapsed arrow fails by construction rather than by luck.
OverlayHostis deliberately the only module that imports the content-zoom service, keeping theservice — including its test-only seams — off the extension-facing declaration bundle.
parseIframeZoomwas extracted so the two iframe-zoom readers cannot drift apart; the existing'150%'test case is exactly the divergence that would otherwise be possible.Component-Builder-Patterns.mdandExtension-Development-Guide.mdwere all updated in the same change.the diff are the decision log's own sanctioned
**Source:**lines.Interview Notes
Stated purpose: fix three defects found by hands-on testing, bundled in one PR stacked on #2825.
The one-way record — decided, not overlooked. The per-type record moves from "unknown" to
"scales its own content" and never back. A pane of such a type that scales nothing in its current
state (a placeholder before a project resolves) therefore sits at 100% while everything around it
is at the user's default, indefinitely. This is reachable during ordinary Simple-mode startup: the
Scripture editor's resource panels mark their zoom area only once content resolves, behind several
earlier returns that mark nothing, and in Simple mode those panels mount in Column 3 before a
project does. The author was given both options — leave it (never flashes, but an empty panel can
sit at the wrong scale) or give up waiting after a bounded time (no stuck panels, but the flash
returns) — and chose to leave it: "leave it. no flash is good."
Two pre-existing e2e failures, not caused by this branch.
formatted-default-simple-mode.spec.tsandparagraph-style-trigger-column-floor.spec.tsfail onthis branch, and fail identically on the branch's base — verified by a control run at
2672ef451d6, same command and machine. Both are blocked by the first-run onboarding tour coveringthe toolbar in simple mode; both failing specs run in simple mode and neither dismisses the tour,
while every passing content-zoom spec runs in power mode. Author's decision: leave for now, no
ticket.
Verified by hand in the Windows dev app. The author ran all three fixes live on this branch
(
git-wincheckout,npm start) on 2026-09-19 and confirmed each: no flash on a newly opened tab,the chords following a clicked footnote, and the marker menu drawing at the pane's scale with its
arrow on the trigger. Headless e2e had already covered the same three; this was the confirmation
pass, which is what the Windows dev app is for.
One thing still not verified in the running app. No in-app pane currently opens
OverlayPopoverfrom a zoomed area, so that half of the arrow and sizing fix was checked in Storybook rather than
live — the hands-on pass could not reach it either. Its near-identical sibling, the command palette,
is covered by a passing e2e test. Called out rather than glossed, because the two were fixed
together and only one was exercised for real.
A regression this branch introduced and caught. The first version of the pop-up fix moved the
padding onto the new inner wrapper but left the width and flex spacing behind on the shared
component, silently resizing and respacing the popover at every scale including 100%. The visual
check that was supposed to catch it compared the new code at 100% against the new code at 200%, so
a change present at both scales was invisible to it — the control that was missing was new-vs-old.
Found by automated per-commit review, verified in the code, and fixed.
In-Review Quality Check
Two Minor findings were fixed during the review (the validator
nullcase and the duplicate iframelookup). Targeted suites re-run after those changes:
overlay-coordinates.test.tsandcore-settings-info.data.test.ts, 54 tests, all passing. ConfirmedgetWebViewIframeZoomis stillconsumed by
overlay-host.component.tsxand so was not orphaned by the change.Full gates on the preceding commit:
typecheck(4/4 projects),lintandformat:checkall clean;core suite 5,636 passed / 15 skipped; workspace suites green; e2e
tests/isolated/notes-content-zoom/2/2; e2etests/isolated/scripture-editor/11/13 with the twofailures proven pre-existing as described above. All roborev per-commit reviews for this branch were
triaged and closed with evidence, and audited
closed=true.Suggested Review Focus
trade was decided deliberately in favour of never flashing — worth a second opinion, since it
is a look-and-feel judgement rather than a correctness one.
OverlayPopover's half of the pop-up fix is the only part not exercised in the runningapp, for want of an in-app caller from a zoomed pane.
landed and correcting; this PR changes which ancestor carries the zoom. Whichever merges
second must re-measure the footnote editor pop-up rather than assume a clean rebase.
the requesting pane, so a pop-up can paint slightly outside it. Pre-existing, reproduces at
100%, untouched here; flagged because zoom makes it easier to notice.