Repository navigation
fix: Suggestion menu overflow issue - #2363
matthewlipski wants to merge 5 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@blocknote/ariakit
@blocknote/code-block
@blocknote/core
@blocknote/mantine
@blocknote/react
@blocknote/server-util
@blocknote/shadcn
@blocknote/xl-ai
@blocknote/xl-docx-exporter
@blocknote/xl-email-exporter
@blocknote/xl-multi-column
@blocknote/xl-odt-exporter
@blocknote/xl-pdf-exporter
commit: |
|
Fix looks like it works to me! |
|
Haven't looked at code yet, just the demo. I noticed the menu can still get very tall: 
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 |
|
I added a max height of 600px - I think for this PR though, an overall UX fix-up is out of scope |
nperez0111
left a comment
nperez0111
left a comment
There was a problem hiding this comment.
I'll let @YousefED take one last look at this, since he knows best the edge cases. But this feels right to me
|
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 |
|
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 |
Makes sense!
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! |
|
I think 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? |
We're passing a real element, so autoupdate should work
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)
Two reasons top of mind:
|
… at 600px The side now depends only on the trigger and the visible area (a fixed 200px menu), so nothing needs to be remembered: filtering cannot flip the menu, and reopening, scrolling or resizing re-choose the side. Uses platform.detectOverflow like autoPlacement does, and measures above only when the menu does not fit below. Raises the maximum height to 600px. Adds tests for asynchronously loaded items (#2363), page scroll, window resize, and the gap to the trigger while filtering (#2155).
|
#3190 (draft) addresses the same problem with the approach discussed above: the side is decided from a fixed height, not the measured menu, so it does not depend on when the items load, and the menu keeps its side while filtering. It needs no context or |
…side and width while filtering (#3190) * fix(core): don't reposition the suggestion menu on its own scroll The scroll listener captures every scroll in the editor, including the menu's own. Each one recomputed the menu's position and re-rendered it, so a menu with little room could not be scrolled to its end. Only a scroll of an element that contains the trigger can move it. Part of #2771 * fix(react): place the suggestion menu below unless cramped, keep its side while filtering The size middleware capped the menu to the room on its side, and autoPlacement then measured the capped menu: it fit where it was, so a menu near the bottom of the screen stayed squeezed below the trigger, and in WebKit it swung between the sides. A placement middleware now keeps the menu below unless there is less than 200px of room there and more above. It chooses the side when the menu opens and again only when the trigger moves, so the menu does not jump while the user filters the items. Both suggestion menu controllers use it. Closes #2771 * fix(react): cap the suggestion menu at 400px With a lot of room, the menu took nearly the whole height of the screen. It now takes at most 400px; a longer list scrolls. * fix(react): decide the suggestion menu's side from geometry only, cap at 600px The side now depends only on the trigger and the visible area (a fixed 200px menu), so nothing needs to be remembered: filtering cannot flip the menu, and reopening, scrolling or resizing re-choose the side. Uses platform.detectOverflow like autoPlacement does, and measures above only when the menu does not fit below. Raises the maximum height to 600px. Adds tests for asynchronously loaded items (#2363), page scroll, window resize, and the gap to the trigger while filtering (#2155). * test: update slash menu screenshots for the 600px cap; place the cursor through the API The dark slash menu and the slash menu page up/down screenshots show the menu capped at 600px. They are also about 11px narrower than the old images: that difference predates this change (the old images were captured in an environment that renders the menu wider; main renders it 339px wide too) and stayed within the screenshot tolerance until now. The placement tests place the cursor through the editor API: a window resized with page.viewport is scaled, so mouse coordinates miss. The async items test now asserts that the menu is still empty at its first check. * fix(react): keep the suggestion menu's width while filtering The menu was as wide as its widest item, so it shrank and grew while the user filtered the items. The middleware now keeps the floating element at the widest width the menu has had since it opened, and the menu fills the floating element (one shared CSS rule). The menu can still grow, e.g. when items load asynchronously; each opening starts at its own width. * test: reopening the suggestion menu, and wait for WebKit's late scroll event Adds tests that a menu opened again elsewhere chooses its side and height again, and starts at its own width. The page scroll tests wait for the expected state instead of a fixed second: WebKit delivers the scroll event about a second late.
|
Closing in favour of #3190 (merged), which fixes the same problem along the lines discussed above: the side is decided from a fixed height, so it does not depend on when the items load, and the menu keeps its side and width while filtering. It also has the 600px maximum height. Thanks for the groundwork here! |
This branch was successfully deployed
3 participants
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
GenericPopoverresponsible 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
GenericPopoverUpdateContextwhich contains a function to update the position of the nearestGenericPopover.SuggestionMenuWrapperupdate ancestorGenericPopoverposition when after items are updated.autoPlacementmiddleware toflip(functions same as before, butautoPlacementwas inconsistent after this change).SuggestionMenuController.Impact
N/A
Testing
N/A
Screenshots/Video
N/A
Checklist
Additional Notes