Always render the fallback Popover anchor within the Popover's parent element #53982
Conversation
youknowriad
added
the
[Type] Bug
There was a problem hiding this comment.
Great find, I didn't notice that the fallback anchor is being used and that it has moved to the portal.
There's one other little thing that's broken now in the Storybook. The popover used to inherit the button's styles, and looked like:
But now the popover element location is different, and the inherited styles are different, too:
I'm not sure if any real-life usages are affected, but it's worth being aware of.
| { ! hasAnchor && <span ref={ anchorRefFallback } /> } | ||
| { content } | ||
| </> | ||
| ); |
There was a problem hiding this comment.
Style nit: most of the time the anchor is specified, so let's avoid rendering the redundant fragment by writing it as:
if ( hasAnchor ) {
return content;
}
return <><span/>{ content }</>;
There was a problem hiding this comment.
Just to understand as I catch up with the changes happened to Popover:
previously, { content } was rendered as a child of the anchor span, but it's now rendered as a sibling. Is that on purpose for this PR ?
There was a problem hiding this comment.
Previously, the { content }, i.e., the popover element (used as floating by Floating UI), would be rendered as a child of the span, and it would be absolutely positioned, so it's not part of the layout flow. So, layout-wise, the <span> is empty, and Floating UI ensures that the popover is positioned correctly relative to the empty <span> anchor.
Now, the { content } is rendered in a portal element, at a completely different location. The <span> is empty not only layout-wise, but also DOM-markup-wise. Again, Floating UI is managing the position.
It's true that <span>{ content }</span> would also work, because content is a portal, it doesn't affect the DOM structure of the span at all. The only difference is whether the span React element receives the events dispatched withing the portal. But as the span has no listeners, it doesn't matter.
Yeah, that's actually one of the reasons for the removals of "incidental" inline popovers. Before the other PR, rendering the block editor without the popover slot was actually working but all the popovers were broken in terms of styles because they were inheriting styles that they shouldn't have been inheriting. I think it could be considered a breaking change for third-party usage but not in WordPress since the slot was always rendered there. |
jsnajdr
force-pushed
the
fix/popover-anchor-regression
branch
from
357efcb to
1b4667a
Compare
|
Size Change: +3 B (0%) Total Size: 1.51 MB
ℹ️ View Unchanged
|
|
I know that it has been partially discussed before (including a comment from @tyxla ), but the changes from both #53889 and this PR are IMO breaking changes from the point of view of consumers of the package:
While I understand the reason behind those changes, I think that we should at least add a CHANGELOG entry under "Breaking changes" and also add a dev note for the next 6.4 release. |
|
@ciampo That's fine by me, I can open a follow-up to update the changelog. |
youknowriad
added
the
Needs Dev Note
|
I added the "needs dev note" but I'm honestly not certain we need one because in WordPress we were using a slot before meaning the popovers were not inline by default and they won't be inline by default, while their position in the DOM will change slightly, the behavior is going to be exactly the same. |
That's true, to get an inline popover, you had to either:
So, the breaking change seems to be mainly the different DOM position. That's not negligible though: CSS cascade is affected, the accessibility tree and tab order... |
|
This is a breaking change, maybe not for core, but these packages are consumed elsewhere. Took me 2 hours to track this down to this PR. We render a slot, but previously all the styling was written taking advantage of the inline placement. Not a huge issue to fix but definitely breaking. And for reference we use it in custom WP plugin dash pages, so still within WP, but breaking none the less. And that Breaking change notice should be listed in both the package changelog in the main Gutenberg rollup release changelog imho. Let me clarify why it should be in Gutenberg plugin release changelog:
As such, even though I do read the package changelogs when we update them in our package.json deps list, the most reliable way to actually deduce the issue within the |
|
Hey @danieliser , thank you for your feedback
This change was definitely listed in the package's CHANGELOG (see #54022). Regarding the Gutenberg plugin CHANGELOGS, I'm sure sure who to reach out to — @youknowriad @bph do you happen to know? |
|
Thanks for the feedback @danieliser That said, for me the main target of the plugin changelog is clearly WordPress developers because it's a WordPress plugin. The changelog is published on make/core. I see how that changelog can serve as a quick way to see updates to multiple packages at the same time but the reality is that there are subtle differences between what happens in WordPress and the packages so I think it may create some confusion for WordPress developers if it becomes basically just a collection of all the changelogs of all the packages. |
|
@youknowriad Well in this case we are WP developers, developing for/within WP. We do not ever bundle the packages in our code, so the only versions we actually can test with are those in WP core and the Gutenberg plugin (early test for whats coming to core). As such the actual package changelogs are almost useless as they update independently of the gutenberg plugin and eventually WP core. So I'd argue as a WP dev not using the packages outside of WP environment, but rather the core/plugin scripts, the only reliable place to know what might be a |
|
Side effect of moving it, now trapping focus becomes a bit more difficult for some interface setups. Example we have a custom url picker field, the suggestions popover is only visible when the field's container is focused, clicking out of the focused field area results in the popover closing, but now the popover itself is outside that area. I'm sure I will work out a solution, likely tricking isFocus to also work if the popover is the current focus target, but wanted to list this side effect here as this was the cause. |
|
@danieliser Just noting that regardless of what you're trying to do, it's possible to have the exact behavior as before:
|
|
@youknowriad - All good, just had to change our CSS selectors and add some extra code to our onBlur state updates. That said, didn't know there was an My point was simply that this should have been easier to pinpoint, as well as leave notes for others who will see this problem soon enough as to what side effects could be lurking. That way when others find this issue based on some search terms, they can get confirmation their issue is the same as mine etc. |
ciampo
added
has dev note
|
Added dev note in the PR description |
Fix #53889 (comment)
Regression introduced in #53889
What?
We did a small refactor to how popover renders in #53889 but introduce a small subtle regression. Basically if the Popover doesn't define an "anchor", we used to render the anchor inline while after #53889 it has been mistakenly "portaled" to the popover container. This PR solves that by always rendering the popover anchor inline.
Testing Instructions
1- Run storybook
npm run storybook:dev2- The default popover story should work properly.
✍️ Dev Note
As part of a wider effort to streamline the developer experience of using Gutenberg as a platform/framework to build block editors, the
Popovercomponent has been tweaked so that it's not necessary anymore to manually specify aPopover.Slotcomponent (and aSlotFillProvider) somewhere in the React tree.The
Popovercomponent now works out of the box, and thePopover.Slotcomponent can be optionally used to tweak where the popover should render in the DOM tree.A side-effect of this change is that some instances of
Popovermay not render inline anymore when aPopover.Slotis not rendered on the page, affecting both the popover's DOM position and the styles inherited via the CSS cascade. To mitigate this use case, a newinlineprop has been added to thePopovercomponent, to force the component to render inline, thus preserving the legacy behavior.