Keep anchored RTE insert popups inside the viewport - #6110
Conversation
🟡 Waiting for changesLast updated: 2026-08-27 12:32 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6110 — the vertical clamp is correct and fixes the reported formula-menu case; .formulas-menu height is stable across the async MathLive load, so the height measured at nextTick is final. Good comment on why fixed positioning forces a pull-up rather than a flip.
CI passing. Manual QA did not run in this round — no UI verification behind this review.
Three suggestions inline: the height is measured once but .image-upload-modal grows when a file is picked; the new tests pass with nextTick(updatePosition) deleted; and only the vertical axis is clamped. Plus a nitpick on the z-index value.
Acceptance criteria: formula popup fits at 1280x720 (menu ~615px, clamp holds) — verified by code, not in a browser. "No overflow in any non-overflowable container" is met vertically only.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran a phased review pipeline over the pull request diff:
- Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
- Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
- Specialized frontend/backend review passes applied framework-specific lenses where those files changed
- For UI changes: manual QA and an accessibility audit against a live dev server, when available
- Checked CI status and linked issue acceptance criteria
- Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence
| } | ||
| const rect = anchorElement.value.getBoundingClientRect(); | ||
| // The modal is fixed-positioned, so overflow past the bottom of the viewport cannot be | ||
| // scrolled into view: pull the modal up by the overflowing amount instead. When the modal is |
There was a problem hiding this comment.
suggestion: Height is measured once at open (plus scroll/resize), but .image-upload-modal changes height while open: it opens in the drop-zone state (ImageUploadModal.vue:91-110, ~290px) and switches to the preview state on file selection (ImageUploadModal.vue:46-90), adding a file-info row, a 150px preview container and the alt-text field — roughly +155px.
At 1280x720 with the anchor at y=400: highestTop = 720 - 290 - 8 = 422, so top stays 405; after the growth the footer ends around 850px and the Insert button is off-screen — the same failure this PR fixes for formulas.
A ResizeObserver on the measured element, started on open and disconnected in cleanup, covers this and the scroll/resize path in one mechanism, without consumers having to know.
There was a problem hiding this comment.
The natural instinct would be to scroll, and scrolling would fix this. If the container is non-scrollable, that'd be a problem, but right now a non-scrollable container would only happen on a large screen, and a large screen would have enough space to show the new height. We should use KDS components for these floating components once KDS is ready, so I don't want to make this more complex if it will probably be replaced by a more robust approach anyways.
| }); | ||
|
|
||
| it('anchors the modal below the target when it fits in the viewport', async () => { | ||
| createModal(100); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Every test calls createModal(...) before openModal, so the element is already in the DOM with a stubbed offsetHeight when setAnchoredPosition runs updatePosition synchronously. The clamped value is final at that point and the await nextTick() observes the same number — delete nextTick(updatePosition) from the composable and all four tests still pass, even though that line is what makes measurement work in production (the modal is v-if'd on the open state).
Insert the modal during the tick instead — e.g. watch(isModalOpen, () => createModal(200)) before openModal — then assert the top is unclamped before nextTick and clamped after.
setupClickOutside's new signature is also uncovered; a wrong selector there fails silently.
| // but not overflowing past the bottom of the viewport. | ||
| const top = Math.max(VIEWPORT_MARGIN, Math.min(rect.bottom + ANCHOR_GAP, highestTop)); | ||
| popoverStyle.value = { | ||
| position: 'fixed', |
There was a problem hiding this comment.
suggestion: Only the vertical axis is clamped. left: rect.right + translateX(-100%) puts the left edge at rect.right - modalWidth; .formulas-menu resolves to min(90vw, 500px) against the viewport, so an anchor whose right edge is under 500px from the viewport's left edge pushes the menu off-screen — and there is no scroll escape horizontally.
This bites hardest in RTL, where the toolbar mirrors so buttons that sit far from the left edge in LTR sit near it, and translateX(-100%) is script-built so RTLCSS cannot flip it. A symmetric clamp in the same expression closes it:
const left = Math.min(Math.max(rect.right, modalWidth + VIEWPORT_MARGIN), window.innerWidth - VIEWPORT_MARGIN);If it is deliberately out of scope, worth a QA check at ~360px/~768px in an RTL locale rather than a code change.
There was a problem hiding this comment.
At that screen size, we don't render a popup but a modal.
| * @param {String} modalSelector selector of the modal element being positioned. It is used to | ||
| * measure the modal, so that an anchored modal can be kept inside the viewport. | ||
| */ | ||
| export function useModalPositioning(modalSelector) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: The selector's role widens here from "is the click inside?" to "how tall is it?", where a wrong or missing match silently degrades to no clamp (offsetHeight || 0) rather than erroring — and it is a document-wide lookup while several editors can be mounted at once (AnswersEditor.vue, HintsEditor.vue render one per answer/hint), so it matches the first such modal in the document, not this composable's.
ImageUploadModal already has ref="modalRoot" on its root. Taking an element getter instead of a selector string keeps the same API shape and fixes both; the click-outside contains check works off the same element.
| top: 0; | ||
| left: 0; | ||
| z-index: 2; | ||
| z-index: 8; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: 8 reads as arbitrary next to .has-overlay { z-index: 10 } below and z-index: 1000 in FormatDropdown.vue:252 / PasteDropdown.vue:321 — dropdowns in the same toolbar and stacking context. Which KDS guidance is 8 from, and should those two move to it too? A one-line comment naming the layer would stop the next person nudging it.
6f43875 to
002d59c
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6110 — 3 of 5 prior findings resolved; 2 contested and left to your call. No new findings in the delta.
CI passing. Code-only pass — manual QA did not run, so nothing here is visually verified.
The two contested items (useModalPositioning.js:27 modal growth after measurement, :37 horizontal clamping) rest on your answers; not re-raising. One correction to the record on the second: the centered branch keys on isTouchDevice, not viewport width, so narrow non-touch desktop windows still take the popup path rather than the modal one.
Prior-finding status
RESOLVED — contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/composables/useModalPositioning.js:98 — document-wide querySelector matches another editor's modal
RESOLVED — contentcuration/contentcuration/frontend/shared/views/TipTapEditor/tests/useModalPositioning.spec.js — tests passed with nextTick(updatePosition) deleted; setupClickOutside uncovered
RESOLVED — contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/TipTapEditor.vue:381 — z-index: 8 unexplained
CONTESTED — contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/composables/useModalPositioning.js:27 — .image-upload-modal grows after height is measured
CONTESTED — contentcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/composables/useModalPositioning.js:37 — only the vertical axis is clamped
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
marcellamaki
left a comment
marcellamaki
left a comment
There was a problem hiding this comment.
the code changes seem logical here, and i think once we make some further changes to our floating patterns/elements/behavior in KDS, hopefully we will be able to introduce even a more robust solution for this across the board. But for the near term, the tests seem to cover all of the cases well, and some manual QA at a variety of screen sizes confirms the original problem and that this fixes it. 
3 participants
Summary
useModalPositioningto keep anchored RTE insert popups inside the viewport.Grabacion.de.pantalla.2026-08-26.a.la.s.10.12.14.p.m.mov
References
Fixes #5957
Reviewer guidance
Follow guidance in #5957.
AI usage
Found the root cause, asked Claude to implement a fix and tests, then reviewed and tweaked a couple of details, like the
z-indexadjustments.