fix: prevent dropdown menus from overflowing the editor root - #562
Conversation
XCFramework BuildThis PR's XCFramework is available for testing. Add the following to your .package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "pr-build/562")Built from 0c74e65 |
b28c1e4 to
686e739
Compare
Block toolbar popovers (e.g. the "Options" menu) render into a fallback container appended to the body and are positioned relative to the viewport. When anchored near the right edge—such as after scrolling the block toolbar—they extend past the viewport, growing the scrollable width of the root element. The canvas could then be swiped horizontally, revealing the empty background behind the editor styles wrapper. Apply `contain: paint` to the body, which fills the viewport, so off-screen popovers are clipped there instead of expanding the document's scrollable width. `overflow-x` on the body does not clip them (the value propagates to the viewport rather than clipping out-of-flow descendants), and clipping on `#root` misses them since the fallback container is a sibling of `#root`, not a descendant. Because `contain: paint` on the body would otherwise clip content taller than the viewport instead of letting it scroll, give `.gutenberg-kit-root` its own vertical scroll so overflowing editor content scrolls there rather than relying on the (now paint-contained) document. Clip at the body rather than moving popovers into a `Popover.Slot` within the editor: popovers must stay in the default fallback container—a sibling of `#root`—so the modalize helper can mark the rest of the editor inert while keeping the active popover accessible when the block inspector or inserter is open (see #145). Document this constraint inline. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`modalize` hid only the body's children, which assumed the modal element was itself a body child. That holds for popovers in Gutenberg's fallback container, but not for popovers rendered into a slot nested within a body-level container. Walk from each modal element up to the body instead, hiding the other children of every ancestor along the way. Ancestors of a modal element are collected first so they are never hidden, which would hide the modal element with them. Also accept multiple modal elements, as popovers may render into more than one container. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Block toolbar popovers—e.g. the "Options" menu—rendered into Gutenberg's fallback container, a body child positioned relative to the viewport. When anchored near the right edge, such as after scrolling the block toolbar, they extended past the viewport and grew the document's scrollable width. The canvas could then be swiped horizontally, revealing the empty background beside the editor styles wrapper. Render popovers into slots within body-level containers instead, split by whether the popover needs clipping: - Dropdown popovers use the default slot, in a fixed, `overflow: hidden` container. Being out of flow, it clips the overflow without contributing to the document's scrollable size, so the document remains the scroll container. - The full-screen block settings menu opts into its own slot in an unclipped container via `__unstableSlotName`. WebKit renders a `position: fixed` element semi-transparent when an ancestor clips its overflow, and the menu fills the viewport and never overflows, so it does not need clipping. Both containers are body children, so `modalize` keeps the active popover accessible while marking the rest of the document inert. This replaces `contain: paint` on the body, which clipped the overflow but required moving vertical scroll onto `.gutenberg-kit-root`. That broke iOS scroll-to-top, which only drives the web view's main scroll view—the document scroller. Wrap the layout in a `SlotFillProvider` so the slots and the editor's popovers share one registry. `BlockEditorProvider` otherwise creates its own, leaving the slots unreachable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Color and typography controls open dropdowns—the color picker, the tools-panel options menus, the font-size unit selector—that render into the default slot in the clip container. The block settings menu renders into the overlay container, which stacked above the clip container, so those dropdowns painted behind the opaque menu and appeared to do nothing. Stack the clip container one above the overlay container. The dropdowns are anchored on-screen within the menu, so clipping them to the viewport is harmless, and painting them above the menu makes them visible. These dropdowns cannot instead be routed into the overlay slot: they arrive through the `InspectorControls` slot-fill, which renders them in the fill's own React context—outside any slot-name provider wrapped around `BlockInspector`—so a context override does not reach them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`unmodalize` popped the most recent batch off a stack, assuming modals close in the reverse of the order they opened. They do not: the block inserter and the block settings menu can be open at once, and React runs effect cleanups in hook-declaration order, not reverse-open order. When two modals overlapped, closing the first popped the second's batch, revealing elements while a modal was still open and stranding others `aria-hidden` after everything closed. This was masked only because both call sites pass the same containers, so the second `modalize` produces an empty batch. Any caller passing a distinct `elementRef`—which the signature supports—would hit it. `modalize` now returns a handle for the batch it hid, and `unmodalize` removes that batch by identity, keeping an element hidden while any other open modal still hides it. The batch is a `Set`, so overlapping ancestor chains do not record an element twice. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Restore pointer events on the clip container's slot content rather than only on `.components-popover` roots, so a fill that portals in non-popover markup is not silently click-through. - Drop the create-on-demand fallback in `getContainer`. It appended to the global `document`, not the target one, so it never served the multi-document case its comment described—and no such case exists: the containers are declared in `index.html`, and enabling the editor iframe later would not move them, as popovers render into the parent document. - Note that the block inserter is a fixed, full-screen popover that renders into the clipped container without the compositing artifact that the settings menu avoids, so the trigger is narrower than any clipped fixed element. - Correct the "rendered before the editor" comment: slot registration order is not load-bearing, as Gutenberg re-parents a fill that mounts before its slot. - Explain why `#popover-fallback-container` is kept. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The header credited the file as a port of Gutenberg's `aria-helper`, but it now diverges: `modalize` walks the ancestor path and `unmodalize` reverses by handle. Record both so the file is not mistaken for a faithful copy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
5da0617 to
49dd690
Compare
There was a problem hiding this comment.
These styles and the corresponding Popover.Slots are the crux of the solution to the overflow problem. All other changes are adapting existing logic for compatibility with the new DOM structure.
Essentially, we now have two separate "slots" in which popovers are rendered: one for popovers that need clipping to avoid horizontal body overflow receive (#popover-clip-container); another for all others that do not and operate the same as before (#popover-overlay-container).
The problem is unique to GutenbergKit, in comparison to web editor, in that the former pursues relying upon body element scrolling to preserve iOS' tap-status-bar-to-scroll-to-top behavior. The web does not pursue this—instead relying upon children element scrolling—and can easily forcibly clip body overflow without concern for breaking the ability scrolling post content. Tapping the status bar when using the web editor does not scroll to the top.
|
@dcalhoun I asked Claude for a review and it found three potential issues. |
|
@dcalhoun I verified the fix works in the iOS simulator so I'll approve it. I'm not sure if any of Claude's findings are valid so feel free to merge after reviewing them. |
Three related corrections to the modalize a11y helper, prompted by review: - `modalize` now records a sibling in its batch even when an earlier batch already hid it, and `unmodalize` clears `aria-hidden` only once every batch that hid the element has been reversed. Previously the second of two overlapping modals produced an empty batch (every candidate was already hidden), so closing the first modal revealed elements while the second was still open. An element hidden by anything other than a batch (e.g. authored markup) is still left untouched. - Remove the unused `elementRef` parameter from `useModalize`. No caller passed it, so its branch never ran, and its one live effect was to mask the bug above. The hook always modalizes the two popover containers. - Make `unmodalize`'s `handle` required. Its default of "the most recent batch" reinstated the very close-order assumption the handle exists to avoid, and the backward-compatibility rationale was moot—the package is private and re-exports nothing. Add unit tests covering overlap, close order, authored `aria-hidden`, and live regions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed the shared findings in 0c74e65, then re-tested. |
What?
Opening a block toolbar dropdown menu (e.g. the "Options" more menu) can add horizontal overflow to the editor's root element, letting the canvas be swiped sideways to reveal the empty background behind it.
Why?
Ref CMM-997.
The editor is not iframed on mobile, so the whole editor lives in a single document. Block toolbar popovers render into a fallback container appended to
<body>and are positioned relative to the viewport. When a popover is anchored near the right edge—such as after scrolling the block toolbar so the toggle sits at the far right—it extends past the viewport. Because nothing clips horizontal overflow on<html>/<body>/#root, this grows the scrollable width of the root element, and the canvas can be swiped horizontally to reveal the empty background behind the editor styles wrapper.Measured with the menu open on a 375px-wide viewport: the popover's right edge lands at 419px (44px past the viewport),
html.scrollWidthgrows to 419 whileclientWidthstays 375, and only<html>becomes horizontally scrollable.How?
Render popovers into slots within body-level containers, split by whether the popover needs clipping:
position: fixed; inset: 0; overflow: hiddencontainer. Being out of flow, it clips the overflow without contributing to the document's scrollable size, so the document remains the scroll container. The container spans the viewport, so it setspointer-events: noneand its slot content restorespointer-events: auto.__unstableSlotName. It fills the viewport and never overflows, so it does not need clipping. It is also kept clear of a clipping ancestor as a precaution: with an earlier clipping approach the menu rendered semi-transparent in WebKit (editor content bleeding through). The exact trigger is narrower than "anyposition: fixedelement under a clipped ancestor"—the block inserter is also fixed and full-screen and renders into the clipped container with no such artifact (verified on device)—but the menu gains nothing from clipping, so keeping it in an unclipped slot sidesteps the risk entirely.Both containers are body children, so
modalizecan keep the active popover accessible while marking the rest of the document inert. The clip container is stacked one above the overlay container (z-index). Dropdowns opened from within the settings menu—color and typography controls, etc.—render into the default (clipped) slot; stacking the clip container above the opaque menu keeps them visible. They are anchored on-screen within the menu, so clipping them to the viewport is harmless. Routing them into the overlay slot instead is not possible: they arrive through theInspectorControlsslot-fill, which renders them in the fill's own React context—outside any slot-name provider wrapped aroundBlockInspector—so a context override does not reach them.A few supporting changes:
modalizenow walks the ancestor path. It previously hid onlydocument.body's children, which assumed the modal element was itself a body child—true for the fallback container, not for a popover nested in a slot. It now walks from each modal element up to the body, hiding the other children of every ancestor, collecting ancestors first so they are never hidden. It accepts multiple modal elements, and returns a handle sounmodalizereverses that specific batch by identity rather than assuming modals close in reverse-open order—the inserter and settings menu can be open at once, and React runs effect cleanups in hook-declaration order.SlotFillProviderso the slots and the editor's popovers share one registry.BlockEditorProviderrenders<SlotFillProvider passthrough>, which creates its own registry when no provider exists above it, leaving the slots unreachable and silently sending popovers back to the fallback container.Copying additional context from #562 (comment) for posterity:
Alternatives considered
contain: painton the body (the previous approach in this PR) clips the overflow, but it also stops the document from scrolling content taller than the viewport. Restoring vertical scroll on.gutenberg-kit-rootmoves the scroll off the document, which breaks iOS status-bar-tap scroll-to-top—that gesture only drives the web view's main scroll view. Both rules are reverted here, so the document is the scroll container again.overflow-x: hidden/cliponhtml/bodydoes not clip out-of-flow popovers at all (the value propagates to the viewport), and clipping on#rootmisses them entirely since the fallback container is a sibling of#root, not a descendant.Testing Instructions
trunk, the canvas shifts left and reveals the root element's background.)Please also confirm the following, as they cover the constraints this approach had to satisfy:
Accessibility Testing Instructions
Popovers now render into slots rather than Gutenberg's fallback container, and
modalizewas refactored to match, so the inert behaviour from #145 is worth re-checking with VoiceOver:Verified in the DOM: with a popover open,
#rootisaria-hidden="true"while both popover containers, the popover's ancestor chain, and thearia-liveregions are untouched; dismissing removes every attribute that was added.Screenshots or screencast
before.mov
after.mov