Skip to content

Conversation

@matthewlipski
Copy link
Collaborator

@matthewlipski matthewlipski commented Jan 15, 2026

Summary

This PR fixes an issue when on initial show, the suggestion menu would not correctly flip vertically if there was no space, and would only flip once the items updated.

This would happen because the GenericPopover responsible for positioning and sizing the suggestion menu would do so based on its initial size. However, the menu fetches and renders its items async, so its initial size is far smaller than when it has all of its items.

To fix this, the GenericPopover's position is re-calculated when the suggestion menu items are updated.

Rationale

The suggestion menu issue is pretty annoying for users and causes bad UX.

Changes

  • Added GenericPopoverUpdateContext which contains a function to update the position of the nearest GenericPopover.
  • Made SuggestionMenuWrapper update ancestor GenericPopover position when after items are updated.
  • Changed autoPlacement middleware to flip (functions same as before, but autoPlacement was inconsistent after this change).
  • Added max height of 600px to SuggestionMenuController.
  • Applied same changes to grid suggestion menu.

Impact

N/A

Testing

N/A

Screenshots/Video

N/A

Checklist

  • Code follows the project's coding standards.
  • Unit tests covering the new feature have been added.
  • All existing tests pass.
  • The documentation has been updated to reflect the new feature

Additional Notes

@vercel
Copy link

vercel bot commented Jan 15, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
blocknote Ready Ready Preview Jan 16, 2026 5:42pm
blocknote-website Ready Ready Preview Jan 16, 2026 5:42pm

Request Review

@pkg-pr-new
Copy link

pkg-pr-new bot commented Jan 15, 2026

Open in StackBlitz

@blocknote/ariakit

npm i https://pkg.pr.new/TypeCellOS/BlockNote/@blocknote/ariakit@2363

@blocknote/code-block

npm i https://pkg.pr.new/TypeCellOS/BlockNote/@blocknote/code-block@2363

@blocknote/core

npm i https://pkg.pr.new/TypeCellOS/BlockNote/@blocknote/core@2363

@blocknote/mantine

npm i https://pkg.pr.new/TypeCellOS/BlockNote/@blocknote/mantine@2363

@blocknote/react

npm i https://pkg.pr.new/TypeCellOS/BlockNote/@blocknote/react@2363

@blocknote/server-util

npm i https://pkg.pr.new/TypeCellOS/BlockNote/@blocknote/server-util@2363

@blocknote/shadcn

npm i https://pkg.pr.new/TypeCellOS/BlockNote/@blocknote/shadcn@2363

@blocknote/xl-ai

npm i https://pkg.pr.new/TypeCellOS/BlockNote/@blocknote/xl-ai@2363

@blocknote/xl-docx-exporter

npm i https://pkg.pr.new/TypeCellOS/BlockNote/@blocknote/xl-docx-exporter@2363

@blocknote/xl-email-exporter

npm i https://pkg.pr.new/TypeCellOS/BlockNote/@blocknote/xl-email-exporter@2363

@blocknote/xl-multi-column

npm i https://pkg.pr.new/TypeCellOS/BlockNote/@blocknote/xl-multi-column@2363

@blocknote/xl-odt-exporter

npm i https://pkg.pr.new/TypeCellOS/BlockNote/@blocknote/xl-odt-exporter@2363

@blocknote/xl-pdf-exporter

npm i https://pkg.pr.new/TypeCellOS/BlockNote/@blocknote/xl-pdf-exporter@2363

commit: c910dfd

@nperez0111
Copy link
Contributor

Fix looks like it works to me!

@YousefED
Copy link
Collaborator

Haven't looked at code yet, just the demo. I noticed the menu can still get very tall:

image

Could we look into limiting this and do an overall check on the UX (compare with other apps)? Up to you if it should be part if this PR or separate

@matthewlipski
Copy link
Collaborator Author

I added a max height of 600px - I think for this PR though, an overall UX fix-up is out of scope

Copy link
Contributor

@nperez0111 nperez0111 left a comment

Choose a reason for hiding this comment

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

I'll let @YousefED take one last look at this, since he knows best the edge cases. But this feels right to me

@YousefED
Copy link
Collaborator

Great! The UX is a lot better. One minor bug is the page scroll shifts when navigating with keyboard (hold an arrow key to cycle through the list completely). Not sure if that's an easy fix?

Regarding the new Context; shouldn't autoUpdate detect content size changes automatically? https://floating-ui.com/docs/autoupdate#elementresize

@matthewlipski
Copy link
Collaborator Author

The page scroll shift bug looks like it's unrelated to the changes in this PR (also reproducible on website & main), so I think it's out of scope. I'll make a new issue for it though.

As for the autoUpdate question, that's a good point. I took some time to look into that, and it does seem like autoUpdate is getting triggered from the resize as expected. However, that doesn't seem to re-trigger the middleware? This is the part I'm confused about, since calling update manually works just fine.

@YousefED
Copy link
Collaborator

The page scroll shift bug looks like it's unrelated to the changes in this PR (also reproducible on website & main), so I think it's out of scope. I'll make a new issue for it though.

Makes sense!

As for the autoUpdate question, that's a good point. I took some time to look into that, and it does seem like autoUpdate is getting triggered from the resize as expected. However, that doesn't seem to re-trigger the middleware? This is the part I'm confused about, since calling update manually works just fine.

I had a quick look, I think it does get triggered, but maybe we need to configure it differently? I found this guide, but didn't immediately found a fix.

A different direction I'm thinking of would be to always set a fixed height of 600(?) and measure based of that (or use a virtual element to return 600). That would also solve a requirement that imo we don't want the menu to flip once the users filters and less options get available -> menu resizes -> menu flips (imo it's better to have a stable side).

Can you experiment a bit further based of this? I agree there are quite some edge cases involved!

@nperez0111
Copy link
Contributor

nperez0111 commented Jan 27, 2026

I think autoUpdate depends on there being an actual element, not just a virtual element, to attach the resize observer onto. It's unclear to me whether we currently are giving it a real element or not.

I think that the real problem here is that the suggestion menu items are being rendered asynchronously no? Like why are we doing all of these workarounds for something that is actually synchronous most of the time?

Even if not that, I think that autoUpdate is just an abstraction over doing the same sort of thing that we are doing here anyway no? It will just call update to re-calc and re-layout, so I don't understand why we are putting any more effort here if this solution is sufficient?

@YousefED
Copy link
Collaborator

I think autoUpdate depends on there being an actual element, not just a virtual element, to attach the resize observer onto. It's unclear to me whether we currently are giving it a real element or not.

We're passing a real element, so autoupdate should work

I think that the real problem here is that the suggestion menu items are being rendered asynchronously no? Like why are we doing all of these workarounds for something that is actually synchronous most of the time?

The Suggestion Menus are designed to be Async compatible, because users should be able to reuse the menu for things like user mentions (which might require an API call)

Even if not that, I think that autoUpdate is just an abstraction over doing the same sort of thing that we are doing here anyway no? It will just call update to re-calc and re-layout, so I don't understand why we are putting any more effort here if this solution is sufficient?

Two reasons top of mind:

  • Keep a clean codebase. We invested a lot in the UI Components refactor to clean up this part of the codebase. I want to be careful to introduce workarounds too easily if the functionality can be provided by Floating UI
  • By understanding the problem and Floating UI better, we might be able to find a clean solution that for example also fixes the "flipping while filtering" UX I described above (instead of introducing another work around for that one as well)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants