Repository navigation
feat: Portals cleanup - #3046
matthewlipski wants to merge 8 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| // Given a target element, checks whether a `.bn-root` element is somewhere up the DOM tree, as | ||
| // one is necessary to apply correct theming & styling. If one doesn't exist, creates one and | ||
| // returns it, both as a React node and HTML element. Otherwise, just returns the target element or | ||
| // null if the target is undefined. | ||
| function usePortalRoot(target: HTMLElement | undefined): { | ||
| root: HTMLElement | null; | ||
| themingContainer: ReactNode; | ||
| } { | ||
| const rootProps = useBlockNoteViewContext()?.portalRootProps; | ||
|
|
||
| const [needsContainer, setNeedsContainer] = useState<{ | ||
| target: HTMLElement; | ||
| value: boolean; | ||
| }>(); | ||
| const [containerElement, setContainerElement] = useState<HTMLElement | null>( | ||
| null, | ||
| ); | ||
|
|
||
| useEffect(() => { | ||
| if (target) { | ||
| setNeedsContainer({ target, value: !target.closest(".bn-root") }); | ||
| } | ||
| }, [target]); | ||
|
|
||
| if (!target || needsContainer?.target !== target) { | ||
| return { root: null, themingContainer: null }; | ||
| } | ||
|
|
||
| if (!needsContainer.value) { | ||
| return { root: target, themingContainer: null }; | ||
| } | ||
|
|
||
| return { | ||
| root: containerElement, | ||
| themingContainer: createPortal( | ||
| <div {...rootProps} ref={setContainerElement} />, | ||
| target, | ||
| ), | ||
| }; | ||
| } |
There was a problem hiding this comment.
I didn't look at this deeply, but my first thought is whether we can just always have a container? I would rather not rapidly mount/unmount elements based on effects of what their parent tree looks like.
There was a problem hiding this comment.
We could, it would just clutter the DOM somewhat since we could get .bn-roots nested within other .bn-roots. Also for the record, the container element should only get mounted once
@blocknote/ariakit
@blocknote/code-block
@blocknote/core
@blocknote/diagram-block
@blocknote/mantine
@blocknote/math-block
@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: |
| // Default to the ambient portal target (a themed `.bn-root`) so popovers | ||
| // inherit light/dark mode instead of the document body's, and escape the | ||
| // mobile formatting toolbar's horizontal scroll clip. | ||
| const contextPortal = usePortalContext(); |
There was a problem hiding this comment.
We need better naming for this. what's the difference between portalRoot and contextPortal? portalRoot also comes from a context 🤷♂️
There was a problem hiding this comment.
Updated naming, hopefully it's a bit clearer. Found it kinda difficult to have names that are not too long and also not too vague.
| () => resolvePortalTarget(portalElements?.default), | ||
| [portalElements?.default], | ||
| ) ?? | ||
| editorDOMElement?.parentElement ?? |
There was a problem hiding this comment.
if this defaults to editorDOMElement?.parentElement, I don't think portalTarget is a descriptive name for this
There was a problem hiding this comment.
I'm also curious if there are race conditions here or whether we can initialize things simpler.
useEditorDOMElement will be null initially, and then passed as <PortalTarget>. Than once things are mounted, it will be set to the bn-container (BlockNoteViewContainer)
There was a problem hiding this comment.
To avoid confusion - portalTarget is put in a React a context so that any popovers further down the render tree can use it to figure out where they need to portal to. So editorDOMElement?.parentElement is not special just because it's the editor container element, portalTarget is just saying to any popovers that they should portal there if they need to. In any case, hopefully things are clearer with the updated naming.
…nts (#3052) * Added demo * Replaced demo toolbar with actual toolbar * Properly fixed dropdowns * - Fixed link button popover close dismissing virtual keyboard - Made mobile toolbar no longer experimental & part of default UI - Updated example * Added transition to toolbar * Replaced hook with pure CSS * Added docs * Removed redundant `direction` prop * Refactored `useVirtualViewportRect` * Fixed some SSR issues * Removed `portalRoot` prop * Implemented PR feedback * Removed unused component * Small fix * Increased mobile formatting toolbar tap target size * - Fixed `getActiveStyles` with empty selections - Removed hover styles for mobile - Gated comment and link buttons to only show when selection is not empty * Refactored `FormattingToolbarController` to handle whether the desktop or mobile version should be used * Made mobile toolbar only appear when attached editor instance is focused * Fixed toolbar select desktop regression * Increased required threshold to detect orientation change * Fixed formatting toolbar not opening when using touch-capable device with external keyboard * Made mobile formatting toolbar center-aligned * Small fix * docs: name the mobile toolbar layouts + add layout toggle to the example (#2985) * docs: name the mobile toolbar layouts, add layout toggle to example Introduce "scrolling document" (default) vs "pinned scroll container" (opt-in) as the names for the two page layouts the mobile formatting toolbar supports, and restructure the docs section around them. - Docs: simple-first rewrite of the Mobile Formatting Toolbar section (default layout, then the opt-in layout with its two CSS rules). - Example: stop embedding it in the docs (`docs: false`) - its page-level CSS (html/body overflow, full-viewport fixed scroll host, `.prose` rules) leaks into the docs page since examples render inline. Link to the standalone playground example instead. - Example: add a nav-bar switch that toggles the pinned scroll container layout via a class on <html>, so both layouts can be compared. - Playground: `.mantine-AppShell-root` width 100vw -> 100%, which caused a horizontal scrollbar on any example taller than the viewport. - Align README, JSDoc and example comments with the new naming; regenerate examples.gen.tsx. * Implemented PR feedback --------- Co-authored-by: Matthew Lipski <matthewlipski@gmail.com> * Made `.bn-scroll-host` class name apply all necessary styles automatically * Added second editor to example * Cached `isTouchDevice` result * Cleaned up portal element context * Moved injected styles to stylesheet * Added `hide` middleware to `GenericPopover` * Minor fixes * Fixed popovers * Implemented PR feedback * Small fix * Fixed Android gap * Small fixes * Small docs change * Updated docs * Made formatting toolbar smooth in docs & playground * Added warning when `interactive-widget` is not set * Added `PortalContext` * Refactored `PortalContext` to `PortalTarget` * Refactored `PortalContext` to `PortalTarget` * Fixed build * Updated naming * Fixed build * Fixed naming omissions * Always pass `editorPortalElement` to `portalRoot` * refactor(react): one vocabulary and simpler plumbing for portal elements Reworks the portal consolidation from #3046, keeping its model — portal elements registered on the editor, themed roots for foreign targets — and changing how it is named and wired. Naming: one stem, `portalElement`, from `editor.registerPortalElement` and `mount(el, { portalElement })` through `resolvePortalElement`, `usePortalElement` and `PortalElementOverride` to the `portalElement` prop on every popover, menu and select. `portalRoot`, `portalTarget`, `portalContext` and `editorPortal` are gone. Where a forwarded prop and the ambient element coexist they are `portalElementProp` and `portalElement`. Theming: `ThemedRootProps` is replaced by a single `applyThemedRoot(element)` on `BlockNoteViewContext`, composed from the base classes and whatever the UI library adds. The mantine wrapper keeps base's `applyThemeVariables` ref for the editor container and passes the same function down for portal roots, so `BlockNoteTheme.ts` is untouched. Plumbing: the default portal element is derived in `usePortalElement` from the editor's own container, so `BlockNoteViewContainer` needs no state, no merged refs and no provider wrapper. `PortalElementOverride` creates its themed root directly and mounts it in a layout effect, replacing a `createPortal` of an empty div, a ref/state round-trip and a `closest(".bn-root")` read during render. * refactor(ui): keep the portalElement prop's plain name in the UI libraries Inside a component that receives a `portalElement` prop, `portalElement` now means that prop, and the surrounding default from `usePortalElement` takes the qualified name. Reads more directly, and keeps the props destructures on one line as they are on the base branch. * Added tests * refactor(core)!: drop the portal option from editor.mount BREAKING: `editor.mount(element, { portalTarget })` loses its options argument. The option decided where `editor.portalElement` was appended, and that element no longer exists; it had already been reduced to an alias for `registerPortalElement`, which callers can call directly and explicitly: editor.mount(element); editor.registerPortalElement(someContainer); Only needed when floating UI renders outside the editor's DOM tree — UI next to the contenteditable already counts as within the editor. * small fixes * refactor: require portalRoot and never fall back to the body `portalRoot` is now required on `Menu.Root`, `Popover.Root` and the toolbar select. The UI-library adapters never read the editor's context, so they cannot fall back to it themselves; making the prop required lets the compiler enforce what was a convention. Call sites pass the ambient element as-is — `null` before the editor has mounted — instead of coercing it to `undefined`. What each adapter does with `null` follows its library, so none of them portal to the document body: - shadcn passes it through: Base UI waits for a container. - ariakit toggles `portal` off: it renders inline until there is one (Ariakit appends a fresh div to the body for null and undefined alike). - mantine already rendered inline on a falsy value. The shadcn tooltips (`ToolbarButton`, `Badge`) keep reading the ambient element through `useEditorPortalElement`. That is the one documented exception: mantine and ariakit tooltips render inline and would ignore a passed element, so a prop would buy nothing there. * fix: pass the required portal element in the SettingsSelect examples `vp run build` typechecks example projects that lint doesn't cover: the three `SettingsSelect` copies render a `Toolbar.Select` without the now required prop. They sit inside `BlockNoteView`, so they read the ambient element and pass it. Also drops the last `?? undefined` on a portal value: the shadcn `Badge` tooltip passes the element as-is, like `ToolbarButton` already does, so nothing falls back to the body. * test(mantine): pin the render profile of BlockNoteView around portals Portal setups — a `portalElements` default, a per-element target, a controller's `portalElement` — must add no React commits or child renders at mount, and cost the same as the default setup on an unrelated parent re-render, with and without StrictMode. Counts are relative to the default setup measured in the same run, so unrelated editor render changes don't break them. Colocated browser-mode test rather than end-to-end: it needs a real browser (jsdom schedules commits differently) but nothing from the e2e harness, and renders with `createRoot` + `act` since `vitest-browser-react` only exists in the tests package. Red on the previous portal implementation (5 commits vs 3 at mount with an external default target), green here and on the base branch. * fix(mantine,ariakit): style portalled menus by their own class Menus now portal out of the toolbar, side menu and table handle that open them, so rules scoped on those ancestors (`.bn-toolbar .mantine-Menu-item`, `.bn-side-menu .mantine-Menu-dropdown`, ...) stopped applying: 14px items, a missing min-width, an ariakit gap on every popover. Scope on the dropdowns' own classes instead, and keep the ariakit gap for form popovers only. The e2e drag-handle menu selector no longer assumes nesting either. * feat(react): render a floating component's menus inside its wrapper Every menu, popover and form a floating component (toolbar, side menu, table handle, ...) opens now portals into a zero-size anchor next to that component, inside the wrapper floating-ui positions. So they share its stacking context and visibility (the ariakit colors submenu paints above the drag handle without a z-index override; ariakit and shadcn dropdowns hide with their toolbar instead of staying orphaned when it scrolls away), follow it when `portalElements` relocates it, and, for the mobile toolbar, sit outside its scroll strip, which iOS WebKit would otherwise not paint. `GenericPopover`'s closing snapshot must ignore the anchor's holder, or a popover whose children are already gone would snapshot an empty wrapper and vanish instead of fading out. The adapter-private contexts in ariakit and shadcn are renamed so they no longer share a name with the react package's context. * fix(mantine): drop the toolbar focus trap Mantine's `useFocusTrap`, armed once focus was within the toolbar, moved focus back into the toolbar a tick after a menu or form opened. With those now portalled next to the toolbar rather than inside it, that stole focus from the link form's URL field (0 ms clicks in e2e, and always for the link toolbar's Edit button). Tab now moves through the buttons and on, as in the other skins. The color menu's deferred `editor.focus()` existed only to work around the trap. * test(mantine): let commits settle before measuring the render profile Mounting schedules an update from an effect that can commit after `act` has returned. On Linux WebKit whether it does varies from mount to mount, so the setup measured second sometimes counted one commit fewer or more than the baseline and the comparison failed (CI's webkit shard, 1 of 3 runs locally in Docker). Wait until no commit has landed for 50 ms before reading the counts, at mount and after the parent re-render, so every mount is measured once things have settled. * fix(react): portal floating UI next to the editor element by default `usePortalElement` fell back to the editor's `bn-container`, which is the editor element's parent in the default layout but not when a layout renders `BlockNoteViewEditor` itself: there the container may also hold a sidebar, and the table's extend button, sized to the table's full width, escaped the editor's scrolling pane and painted over the sidebar while the table stayed clipped. Fall back to the editor element's parent, as `mount()` did before: floating UI clips and scrolls with the editor, and `portalElements` remains the way to escape. * docs(react): drop the null portal target from the doc comments `PortalElement` is `HTMLElement | string` and `PortalElementOverride` takes `HTMLElement | undefined`, but two doc comments still promised that `null` means `document.body`. Nothing accepts it any more. * chore(docs): allow only the private 172.16/12 range for dev origins Each `*` in `allowedDevOrigins` matches one address segment, so `172.*.*.*` admitted all of 172/8 rather than the private range the comment describes. Spell out the sixteen second segments of 172.16.0.0/12 instead. * feat(react): warn about the viewport meta tag from the view, in development only The check for `interactive-widget=resizes-content` moves out of `useVirtualKeyboard` into `BlockNoteViewEditor`'s mount effect, so it reaches developers on desktop and with a custom formatting toolbar alike. It lives in its own module (not re-exported from the package) and returns early when `process.env.NODE_ENV` is "production": the library build leaves that expression in place for the consumer's bundler, so production builds stay silent and a page without a bundler still gets the hint. * docs(examples): show the mobile toolbar example in the playground The playground already puts `bn-scroll-container` on its root, so the example's own container is nested inside it, and its "scrolling document" switch has to turn the page's container off. Both are CSS in the example: a nested container defers to the outer one, and a body class set by the switch neutralises any container on the page. The library stylesheet and the playground stay as they are. * fix(react): a nested BlockNoteView resolves its own portal element A view rendered inside another view's floating UI (the comments composer, an editor in a custom block's popover) inherited the outer view's portal anchor or override through context. That element is registered with the outer editor only, so the nested editor's own menus and popovers counted as outside it for isWithinEditor and the focus tracking built on it. BlockNoteViewContainer now resets the portal element for its subtree to the editor's own default. * docs(react): state the preventFocusOnOpen contract When set, the UI library must not move focus into the surface when it opens; an input inside that asks for focus itself still gets it. * test: link toolbar create and edit flows per skin The link form tests now run for mantine, ariakit and shadcn: each skin's popover decides on its own how the URL field takes focus. The per-skin screenshot tests also wait for the link toolbar before comparing, since a missing toolbar stays within the 2% screenshot tolerance. --------- Co-authored-by: Matthew Lipski <matthewlipski@gmail.com>
Summary
This PR attempts to consolidate and clean up how portalling is handled in UI elements.
The trigger for this comes from the mobile formatting toolbar. It requires dropdowns to be portalled outside of it as it has
overflow: auto. On desktop,position: fixedelements like the dropdowns are not clipped by an ancestoroverflow: autoelement, but this behaviour varies between platforms on mobile. Additionally, the mobile toolbar itself needs to be portalled todocument.bodyas it should float on top of everything else on the page.Originally, a proprietary
MobileToolbarPortalContextwould set a target element for the dropdowns to portal to. This was clunky as there is already theBlockNoteViewportalElementsprop which is supposed to handle portalling for editor UI elements. Alongside that there'seditor.portalElement, which is set byportalElements.defaultand is used byisWithinEditorto check whether something is in the editor UI. However, this was also broken as it didn't take into account the other portals set byportalElements. The other portals were also broken as they don't receive proper styling/theming, as that relies on a container element with various attributes (things like a.bn-rootclass,data-themeattribute, etc). Overall, I think it's quite clear that there's a benefit to consolidating all of these things into one and fix some existing bugs along the way.So this PR does so by introducing
PortalTargetandusePortalContext, which replacesMobileToolbarPortalContextand is also howportalElementscreates portals. It takes atargetand exposes it in aPortalContextso any descendants always know where an element should be portalled to, by consuming saidPortalContext. AdditionalPortalTargets can also be added anywhere in the render tree to override the target element (useful especially for the mobile formatting toolbar where it and descendant dropdowns portal todocument.body). The reason we need aPortalTargetcomponent instead of just using a barePortalContext, is because the portalled elements need to be inside a container element for styling - same issue that already exists withportalElements. Therefore,PortalTargetfirst checks if a.bn-rootcontainer element exists up the DOM tree, and creates it if necessary.Rationale
isWithinEditorandportalElementshave bugs which partially due to this.editor.portalElementhas a pretty convoluted implementation and is treated as a special case.Changes
PortalTargetandusePortalContext.reactpackage to always usePortalTarget&usePortalContainerto set/get portal elements.editor.portalElementwitheditor._portalRoots. This is now a map which contains all created portals, whichisWithinEditorreads to guarantee that all editor-related elements are checked.Impact
N/A
Testing
TODO
Screenshots/Video
N/A
Checklist
Additional Notes
N/A