Skip to content

fix(super-editor): flip tooltip when clipped by viewport - #3836

Open
dani-beltran wants to merge 18 commits into
superdoc:mainfrom
dani-beltran:main
Open

fix(super-editor): flip tooltip when clipped by viewport#3836
dani-beltran wants to merge 18 commits into
superdoc:mainfrom
dani-beltran:main

Conversation

@dani-beltran

@dani-beltran dani-beltran commented Jul 17, 2026

Copy link
Copy Markdown

Fixes #3786

Tooltip positioning wasn't detecting viewport collisions, it only clamped horizontally, so tooltips could still overflow on top.

This PR fixes the issue, while also refactoring some other components using the same duplicated logic. Also created a repro test for the bug.

Before After
Screenshot 2026-07-18 at 01 31 32 Screenshot 2026-07-18 at 01 32 48

Refactor notes:

  • Extracted the positioning logic into a standalone util function to reduce component complexity and avoid duplicated code.
  • SdTooltip, FontFamilyCombobox and ToolbarDropdown, all share the new utility for positioning
  • Expanded the utility to support additional alignments (bottom-start, bottom-end, etc.) to match FontFamilyCombobox's alignment behavior
  • Added support for calculating available space to fit dropdown height, needed by ToolbarDropdown
  • Added support for boundary elements in contrast to just using the viewport as the boundary. Could be useful for components like ContextMenu that depend on the editor container and not the viewport. Not implemented here though, since ContextMenu uses a specific plugin for this type of logic.
  • Added unit tests for the new utility functions

Demo of how FontFamilyCombobox and ToolbarDropdown components behave after the change. They retain the same behavior as before. 👇

output

@dani-beltran
dani-beltran requested a review from a team as a code owner July 17, 2026 23:37
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0967f00061

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +108 to +109
case 'top-start':
top = triggerRect.top - contentHeight - offset;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Clamp flipped long dropdowns to the viewport

When a bottom-start dropdown flips upward, this subtracts the full content height before callers apply the new maxHeight; for example the existing long-menu case with a 500px menu, trigger top=260, and only 248px available above computes top=-244px, then renders a 248px-tall menu mostly offscreen. Use the constrained render height or clamp to the gutter as the previous dropdown code did, otherwise long toolbar/font dropdowns near the bottom still get clipped after flipping.

Useful? React with 👍 / 👎.

@dani-beltran dani-beltran Jul 18, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed 🟢

.fade-in-scale-up-transition-enter-active,
.fade-in-scale-up-transition-leave-active {
transform-origin: bottom center;
transform-origin: center;

@dani-beltran dani-beltran Jul 17, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the animation origin to be position agnostic, since the tooltip could be now on top or bottom positions. It looks good, barely noticeable difference. However, this is modifying the original animation, so I am considering changing it.

@dani-beltran dani-beltran Jul 20, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a simple solution to make the animation origin match the dynamic tooltip position. 🟢

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c7b4b2b2a9

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

const { placement = 'top', offset = 0, flip = true } = options;
const triggerRect = triggerElem.getBoundingClientRect();
const contentWidth = contentElem.offsetWidth;
const contentHeight = contentElem.offsetHeight;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Use scrollHeight when deciding dropdown flips

For toolbar/font dropdowns, menuPosition.maxHeight is kept in component state after a close, so the element created on the next open can already be constrained by the previous max-height. Measuring offsetHeight here then uses the clipped height rather than the full menu height; for example a 500px menu reopened near the bottom with a stale 120px maxHeight and 200px below/400px above will no longer flip upward, leaving the long menu clipped below. The previous inline dropdown logic used scrollHeight for this decision, so this should measure the scrollable height with an offset fallback.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but using scrollHeight instead of offsetHeight will mess up content components that are not dropdowns. I will add some way of detecting that is a dropdown or add a parameter in the function to indicate it is.

@dani-beltran dani-beltran Jul 20, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed 🟢! I added a way of detecting if the content element is a scrollable component like the dropdown.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

https://github.com/superdoc-dev/superdoc/blob/488f8eaa8ff40f75e5aa6a1cb3348f1576faebad/packages/super-editor/src/editors/v1/components/toolbar/FontFamilyCombobox.vue#L109
P2 Badge Clamp using the widened font popup width

When the font list first opens, menuPosition.minWidth is still 0px, so getAnchoredPosition() can measure and clamp the popup using a width smaller than the trigger. This line then widens the popup to rect.width; for a combobox near the right viewport edge with short font names, the final right edge can overflow even though the old code clamped against rect.width. Clamp with an effective width of at least the trigger width, or recompute after applying the min width.

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@dani-beltran

Copy link
Copy Markdown
Author

💡 Codex Review

https://github.com/superdoc-dev/superdoc/blob/488f8eaa8ff40f75e5aa6a1cb3348f1576faebad/packages/super-editor/src/editors/v1/components/toolbar/FontFamilyCombobox.vue#L109

P2 Badge Clamp using the widened font popup width
When the font list first opens, menuPosition.minWidth is still 0px, so getAnchoredPosition() can measure and clamp the popup using a width smaller than the trigger. This line then widens the popup to rect.width; for a combobox near the right viewport edge with short font names, the final right edge can overflow even though the old code clamped against rect.width. Clamp with an effective width of at least the trigger width, or recompute after applying the min width.

ℹ️ About Codex in GitHub

This is not really happening.
For bottom-start, left is set to triggerRect.left
After applying minWidth = triggerWidth, the popup's right edge lands at triggerRect.right the same as the trigger. If the trigger isn't overflowing, neither will the popup. The review's concern doesn't hold.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 711d00be09

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

placement: Placement,
options: { offset?: number; boundary?: HTMLElement | null } = {},
) => {
const { offset = 0, boundary = null } = options;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Pass the gap as an options object

Every call site introduced here passes a numeric third argument to this helper (..., computedPlacement, gap), and the new offset tests do the same, but destructuring a number treats it like an empty options object so offset always falls back to 0. When toolbar/font dropdowns open near the viewport edge, their top includes the 4px gap while maxHeight does not subtract it, so the menu can still render into the gutter/clip; accept numeric offsets or update the callers to pass { offset: gap }.

Useful? React with 👍 / 👎.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 09e1ab2112

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +188 to +190
const availableAbove = Math.max(0, aboveBottom - GUTTER);
const availableLeft = Math.max(0, rect.left - offset - GUTTER);
const availableRight = Math.max(0, getBoundaryRight(boundary) - rect.right - offset - GUTTER);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Respect the boundary origin when measuring space

When a boundary element is supplied (the context menu now passes the editor surface), this still measures availableAbove from viewport 0 instead of from boundary.getBoundingClientRect().top (and availableLeft has the same issue). In an editor surface that starts below the top of the viewport, a menu opened near the lower part of a short surface can flip upward because this counts space outside the editor; the returned top is then clamped to the viewport gutter rather than the surface, so the context menu can still render above/outside the editor boundary. Use the boundary rect's top/left as the lower clamps and subtract them from the available-space calculations.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed 🟢

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a04b194217

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +547 to +551
const menuPosRect = new DOMRect(parsedX, parsedY, 0, 0);
const { left, top } = getAnchoredPosition(menuPosRect, menuRef.value, {
placement: 'bottom-start',
offset: 0,
boundary: getEditorSurfaceElement(props.editor),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve containing-block coordinates for context menus

When the editor lives under an ancestor that creates a fixed-position containing block, the context-menu plugin intentionally converts the emitted menuPosition into containing-block-relative CSS coordinates before this component receives it (see context-menu.js around the findContainingBlockAncestor adjustment). This new DOMRect treats those adjusted coordinates as viewport coordinates and compares them to the editor surface's viewport getBoundingClientRect(), so near the bottom of a transformed/contained editor it overestimates the space below and fails to flip/clamp the menu. Convert the point back to viewport coordinates for collision detection, or keep both the point and boundary in the same coordinate space before assigning the returned CSS left/top.

Useful? React with 👍 / 👎.

@dani-beltran dani-beltran Jul 30, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ContextMenu uses a specific plugin already handling the positioning logic and changing it will increase the complexity of this PR too much. So I reverted changes to ContextMenu.

@dani-beltran
dani-beltran requested a review from a team as a code owner July 30, 2026 15:58

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 7 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread packages/super-editor/src/editors/v1/components/toolbar/FontFamilyCombobox.vue Outdated
Comment thread packages/super-editor/src/editors/v1/utils/anchored-position.ts Outdated
Comment thread packages/super-editor/src/editors/v1/utils/anchored-position.ts Outdated
Comment thread packages/super-editor/src/editors/v1/utils/anchored-position.ts Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Toolbar tooltips are cut off by edge of the window

1 participant