Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Popover: Introduce overflowPadding prop to support adding an offset #69555

Open
wants to merge 6 commits into
base: trunk
Choose a base branch
from

Conversation

yogeshbhutkar
Copy link
Contributor

@yogeshbhutkar yogeshbhutkar commented Mar 13, 2025

What?

Closes #69553

This PR introduces a new overflowPadding prop for the Popover component, allowing precise control over the offset between the popover container and the viewport edge.

Why?

Previously, there was no way to adjust the offset when the popover container reached the viewport edge, often cutting it off or partially obscuring near screen boundaries.

How?

  • A new prop overflowPadding was introduced.
  • The value of this prop is passed down to the padding property within size middleware from floating-ui.

Testing Instructions

  1. Navigate to Appearance -> Editor -> Templates.
  2. Click on View Options.
  3. Shrink the height of the Browser Window.
  4. Confirm, the popover container doesn't overflow over the edit site page content.

Testing Instructions for Keyboard

Same.

Screenshots

Before

before

After

after

@yogeshbhutkar yogeshbhutkar changed the title Popover: Introduce overflowOffset prop to control maximum height calculation Popover: Introduce overflowOffset prop to support adding an offset between popover and viewport edge Mar 13, 2025
@yogeshbhutkar yogeshbhutkar changed the title Popover: Introduce overflowOffset prop to support adding an offset between popover and viewport edge Popover: Introduce overflowOffset prop to support adding an offset Mar 13, 2025
@yogeshbhutkar yogeshbhutkar force-pushed the enhance-69553/overflow-offset-popover branch from b6f0322 to 15888d8 Compare March 13, 2025 06:23
@yogeshbhutkar yogeshbhutkar marked this pull request as ready for review March 13, 2025 06:51
Copy link

github-actions bot commented Mar 13, 2025

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: yogeshbhutkar <[email protected]>
Co-authored-by: mirka <[email protected]>
Co-authored-by: ciampo <[email protected]>
Co-authored-by: okmttdhr <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@t-hamano t-hamano added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Mar 13, 2025
@ciampo ciampo requested review from a team March 13, 2025 10:35
Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this problem not affect the X axis? The current prop naming and descriptions feel a bit misleading on first glance because it only works on the Y axis.

I'm also wondering if these "padding" values (as Ariakit names it) should actually be set globally in some way, at least at the app level, if not at the wp-components level. I haven't assessed all the use cases, but it seems like this padded behavior is something all popovers would want by default?

@yogeshbhutkar yogeshbhutkar changed the title Popover: Introduce overflowOffset prop to support adding an offset Popover: Introduce overflowOffsetY prop to support adding an offset Mar 14, 2025
@yogeshbhutkar yogeshbhutkar force-pushed the enhance-69553/overflow-offset-popover branch from 06d0b01 to 18f4007 Compare March 14, 2025 04:58
@yogeshbhutkar
Copy link
Contributor Author

yogeshbhutkar commented Mar 14, 2025

Does this problem not affect the X axis?

I couldn’t find any instances where the popover overflows to the viewport edge along the X-axis. This is likely because, in most cases, the popover does not extend beyond the associated toggle button in that direction.

Precisely, this:
dataviews

Ref. (from issue) "Although the dropdown is scrollable and remains functional, it is not visually clear that content is accessible via scrolling. Users may mistakenly believe that the menu is truncated instead of fully available."

Additionally, popovers rarely become scrollable along the X-axis, and when they do, it may not provide the best user experience. I'm interested in also knowing your thoughts on this.

The current prop naming and descriptions feel a bit misleading on first glance because it only works on the Y axis.

I've fixed this in 18f4007

It seems like this padded behavior is something all popovers would want by default?

Since most popovers don’t extend to the viewport edge due to their typically limited content, it might be best to handle these scenarios on a case-by-case basis. That said, applying a global padding style or setting the default overflowOffsetY to 8px should effectively address this. I’d love to hear your thoughts on this as well.

@ciampo
Copy link
Contributor

ciampo commented Mar 14, 2025

Some quick thoughts:

  • I can't think of a case in Gutenberg (or in general) in which the popover would resize along the X axis — it usually flips, instead of resizing; At the same time, I don't think it would hurt to set the same padding for both axis?
  • Any chance we'd want the same "padding" functionality for flipping, too? That's what ariakit does

@@ -233,7 +234,9 @@ const UnforwardedPopover = (

// Reduce the height of the popover to the available space.
Object.assign( firstElementChild.style, {
maxHeight: `${ sizeProps.availableHeight }px`,
maxHeight: `${
sizeProps.availableHeight - overflowOffsetY
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the size middleware accepts a padding prop which we could use instead of manually subtracting the overflowOffset.

I would also take a look at what Ariakit does and take any inspiration — for example, we could apply the overflow offset to both axis?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion, @ciampo!

Using the padding prop within the size middleware is the best approach. As documented here, it accepts either a single number (applying uniform virtual padding) or an object for specifying padding per side. This is how Ariakit implements it.

I've incorporated this in commit 5d9491b and renamed the internal prop to overflowPadding for consistency with Ariakit.

With this change, we can now apply virtual padding across all sides.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mirka also wanted to make sure you agree with keeping one overflowPadding prop (instead of having a dedicated one for the Y axis)

@yogeshbhutkar yogeshbhutkar changed the title Popover: Introduce overflowOffsetY prop to support adding an offset Popover: Introduce overflowPadding prop to support adding an offset Mar 18, 2025
@yogeshbhutkar yogeshbhutkar requested a review from ciampo March 18, 2025 06:34
@yogeshbhutkar yogeshbhutkar force-pushed the enhance-69553/overflow-offset-popover branch 2 times, most recently from fc04545 to 784fb4d Compare March 18, 2025 10:58
@@ -142,6 +142,7 @@ const UnforwardedPopover = (
inline = false,
variant,
style: contentStyle,
overflowPadding = 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mirka I'm torn between a default of 0 and 8, what are your thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The offset feature may be useful for certain things, but I just realized it doesn't really help in solving the actual UX issue that prompted this 🙈 #69553 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have a point! But specifically for the default value, I'm learning towards 0 as it wouldn't introduce any changes to existing Popover usages. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's purely for padding and not for addressing the original UX issue, then I think the default would have to be 0, yes.

I can't say without doing an audit, but I notice that there are a lot of popovers that become fullscreen (completely filling the viewport with no padding) on mobile widths. So we wouldn't want to mess up those instances, though I'm not immediately sure how those are implemented.

@yogeshbhutkar yogeshbhutkar force-pushed the enhance-69553/overflow-offset-popover branch from 784fb4d to 0cd2051 Compare March 19, 2025 04:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Popover component: Improve Visibility When Hitting Viewport Edge
4 participants