Repository navigation
Conversation
github-actions
Bot
added
[Package] Block editor
🤖 PR meta 🤖🎉 PropsIf you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. Updated as activity occurs, without notifying anyone named here. Add the 📦 Bundle sizeSize Change: -4.14 kB (-0.05%) Total Size: 9.05 MB 📦 View Changed
⚡ PerformanceShow the resultsClient side metrics exclude the server response time. front-end-block-theme
front-end-classic-theme
media-processing
media-upload
post-editor
site-editor
🏁 Flaky testsSome tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information. Regression: updating a specific option will only change its value and will not corrupt others in
|
|
Oh, love this simple solution! |
mirka
left a comment
mirka
left a comment
There was a problem hiding this comment.
Thanks for looking into this, I agree this approach is much simpler for our needs!
| * Bubbles selected native events from the iframe document onto the outer | ||
| * `<iframe>` element so parent-document listeners can observe them. | ||
| * | ||
| * `dragover` and `mousemove` keep drag-and-drop and pointer tracking working | ||
| * across the frame boundary. `pointerdown`, `mousedown`, and `click` let | ||
| * parent-document popups (menus, selects, etc.) treat a canvas press as an | ||
| * outside press. Keyboard shortcuts are bubbled separately from the React | ||
| * `onKeyDown` handler on the iframe element. |
There was a problem hiding this comment.
With this, a press in the canvas reaches Base UI as a press on the <iframe> element, not on what was actually pressed. That's fine for popups whose trigger is outside the canvas, but not for a control rendered inside it.
For example, put a Combobox in a block's edit UI. Its popup renders in the parent document through the overlay slot, while its input stays in the canvas. Type to open the list, then click inside the input to move the caret: the list closes, because Base UI sees a press on the <iframe>, which is outside both the input and the popup. I reproduced this with a browser test that bubbles presses the same way. Clicking elsewhere in the canvas closes it correctly.
Nothing in the repo does this yet, so I don't think it blocks this PR. We kind of discussed this the other day, and that we may be ok with just having a large chunk of wp-components/wp-ui be off limits in the editor iframe. We can figure out safeguards around this as a follow up.
For the scope of this PR, can we perhaps note the limitation here, so it isn't rediscovered as a bug?
There was a problem hiding this comment.
Type to open the list, then click inside the input to move the caret: the list closes, because Base UI sees a press on the
<iframe>, which is outside both the input and the popup.
How does the current iframe bridge solution solve this? There is a isInsideCurrentPopup check that bubbles only events that are outside the popup. There is an onOpenChange handler that stores the "trigger" element for the purposed of this checking.
Maybe we could implement a similar check, and filter the bubbled events.
There was a problem hiding this comment.
How does the current iframe bridge solution solve this?
It doesn't, and what we discovered in the other PR was that it can't (at least in a sensible way).
There was a problem hiding this comment.
The ui bridge also covered the site preview iframes (SitePreview in edit-site and boot), which don't bubble events. Should they get the same bubbling, or are we fine leaving that gap since they only render for classic themes on the home route?
There was a problem hiding this comment.
SitePreview is interesting. It's a plain <iframe src={homeUrl} />, no portals, no srcdoc. homeUrl is a same-origin URL, but not always. On WordPress.com Simple the admin can be example.wordpress.com/wp-admin while the site is example.blog, a mapped domain.
It's more like a cross-origin embed, which we can't handle anyway. Make we can sacrifice these frames, I don't know how big the practical issue is.
There was a problem hiding this comment.
We're removing the iframe tests in ui but add no coverage for the bubbling that replaces them. Should we add a minimal browser or e2e test where a non-modal popup in the parent closes on a canvas press, and the press still reaches its canvas target once?
There was a problem hiding this comment.
Good idea, I added an e2e test for closing the tags popup. Fits very nicely into the existing taxonomies.spec.js suites that tests that UI.
Which other parts of the Gutenberg UI can help us test the other variants? So that we cover the entire Autocomplete/Combobox/Popover/Menu spectrum?
There was a problem hiding this comment.
The tags input uses SearchableChipSelectControl, so the new test covers Combobox. For Menu, we could open the editor's Options menu and click the canvas. That already dismisses through useIframeDismissalBridge, so the test would protect the behavior we're preserving when we remove that workaround.
For Popover, the block inspector's style-state info popover (e.g. "More information about Hover" for a Button block) is a possible example, but it also dismisses on hover leave. We'd need to ensure the test actually depends on the canvas press. In human testing, a keyboard interaction (instead of mouse click) to open the infotip would work I think, but not sure if that will translate correctly to an automated test.
I haven't found a suitable @wordpress/ui Autocomplete popup in the editor. The workflow palette uses it inline inside a modal.
I think Tags plus Options would give us useful e2e coverage. Could we also add a focused browser test around the block-editor Iframe to assert that the canvas target receives the click exactly once? The tags test currently only checks dismissal.
1745d6d to
5b8235d
Compare
| timers: vi, | ||
| }; | ||
|
|
||
| // jsdom does not implement DragEvent. A stub is enough for code that only |
There was a problem hiding this comment.
I think my preference would be for tests that require draft events to just run in Vitest browser mode, any reason why you'd want to stick to JSDom instead?
There was a problem hiding this comment.
It's existing JSDOM tests starting to fail, without any change. The reason is that the iframe/index.tsx now references DragEvent statically:
const BRIDGED_EVENTS = [
{ native: 'dragover', react: 'onDragOver', ctor: DragEvent },
]Merely loading the file as a part of the import tree now fails, we don't need to use it.
Maybe we could extract the constructor from the prototype of the original event. And avoid the ctor field.
There was a problem hiding this comment.
Pushed a version that removes the need for ctor: DragEvent code and for the JSDOM polyfill. The constructor name is now extracted from the event itself.
8100acc to
e92200b
Compare
Alternative to #83778 and earlier changes in #83509 and #81952.
Removes completely the
useIframeDismissalBridgelogic added in these PRs. Theuipackage now doesn't have any awareness or handling of cross-iframe issues.Instead, the editor canvas iframe in
block-editornow forwards thepointerdown,mousedownandclickevents from the inner iframe document to the outer iframe element. There is an existing bubble logic there to support drag&drop and keyboard shortcuts, we are merely adding a few new events to it.I tested this on the tag combobox in the post inspector. Opening an autocomplete popup and then clicking in the canvas closes it, as expected. That didn't work before this PR.
Screen.Recording.2026-10-21.at.9.10.48.mov
Conceptually this is a much simpler solution, let's verify that it really works for all cases. For example, in #83509 PR description, @t-hamano has a screencast with a "Canvas popover" and "Sidebar popover". This fix works only when the "Sidebar popover" is open and we click into the canvas. The other way around, a "Canvas popover" open and clicking outside the canvas, it's not fixed. But do we have an actual usage of this in Gutenberg? I think that all popovers should be portaled from the canvas into the top-level window.