GradientPicker: fix popover positioning for control points

[ciampo]

What?

This PR aims at improving the positioning of the popover in the GradientPicker after #40740 introduced a regression by stopping to forward popoverProps correctly in the ColorPalette component (see change).

This PR also makes the popover for the gradient picker always shift when going out of the viewport (previously is shifted only when in the sidebar)

Why?

With this change, the GradientPicker component can override popoverProps again as it was previously intended.

How?

• Take into account the received popoverProps in the CustomColorPickerDropdown component
• Use the bottom-start placement instead of left-start as the position when in the sidebar (since the original position before the refactor in #40740 was bottom left)
• Bonus: Use placement also in the control point component instead of the legacy position prop

Testing Instructions

In Storybook, verify that the popover is always rendered inside the viewport (i.e. shifting behavior is enabled)

In the site editor, try to edit the available gradients palette and focus on the popover positioning for each color stops

Screenshots or screencast

GradientPicker in Storybook, trunk:

https://user-images.githubusercontent.com/1083581/182940241-1dfee265-c54c-4f59-87af-e77551dd60d9.mp4

GradientPicker in Storybook, this PR:

https://user-images.githubusercontent.com/1083581/182940361-509e22c8-4fd1-4fd3-8dbc-b8d904f908c4.mp4

Popover for color stops in gradient picker, site editor, trunk:

https://user-images.githubusercontent.com/1083581/182940472-da0d08ef-dd25-4cab-bd48-1df951fc29ef.mp4

Popover for color stops in gradient picker, site editor, this PR:

https://user-images.githubusercontent.com/1083581/182940489-92e137c8-8565-49c7-babc-59fdb0c3ad2c.mp4

(@jasmussen , welcome back! I may need some guidance here about what is the expected popover positioning when editing color stops in the site editor — see video above)

[github-actions[bot]]

Size Change: +134 B (0%)

Total Size: 1.27 MB

Filename	Size	Change
build/block-library/index.min.js	185 kB	+2 B (0%)
build/components/index.min.js	231 kB	+125 B (0%)
build/components/style-rtl.css	14 kB	+2 B (0%)
build/components/style.css	14 kB	+5 B (0%)

ℹ️ View Unchanged

Filename	Size
build/a11y/index.min.js	982 B
build/annotations/index.min.js	2.76 kB
build/api-fetch/index.min.js	2.26 kB
build/autop/index.min.js	2.14 kB
build/blob/index.min.js	475 B
build/block-directory/index.min.js	6.58 kB
build/block-directory/style-rtl.css	990 B
build/block-directory/style.css	991 B
build/block-editor/default-editor-styles-rtl.css	378 B
build/block-editor/default-editor-styles.css	378 B
build/block-editor/index.min.js	156 kB
build/block-editor/style-rtl.css	14.7 kB
build/block-editor/style.css	14.7 kB
build/block-library/blocks/archives/editor-rtl.css	61 B
build/block-library/blocks/archives/editor.css	60 B
build/block-library/blocks/archives/style-rtl.css	65 B
build/block-library/blocks/archives/style.css	65 B
build/block-library/blocks/audio/editor-rtl.css	150 B
build/block-library/blocks/audio/editor.css	150 B
build/block-library/blocks/audio/style-rtl.css	103 B
build/block-library/blocks/audio/style.css	103 B
build/block-library/blocks/audio/theme-rtl.css	110 B
build/block-library/blocks/audio/theme.css	110 B
build/block-library/blocks/avatar/editor-rtl.css	116 B
build/block-library/blocks/avatar/editor.css	116 B
build/block-library/blocks/avatar/style-rtl.css	59 B
build/block-library/blocks/avatar/style.css	59 B
build/block-library/blocks/block/editor-rtl.css	161 B
build/block-library/blocks/block/editor.css	161 B
build/block-library/blocks/button/editor-rtl.css	441 B
build/block-library/blocks/button/editor.css	441 B
build/block-library/blocks/button/style-rtl.css	539 B
build/block-library/blocks/button/style.css	539 B
build/block-library/blocks/buttons/editor-rtl.css	292 B
build/block-library/blocks/buttons/editor.css	292 B
build/block-library/blocks/buttons/style-rtl.css	275 B
build/block-library/blocks/buttons/style.css	275 B
build/block-library/blocks/calendar/style-rtl.css	207 B
build/block-library/blocks/calendar/style.css	207 B
build/block-library/blocks/categories/editor-rtl.css	84 B
build/block-library/blocks/categories/editor.css	83 B
build/block-library/blocks/categories/style-rtl.css	79 B
build/block-library/blocks/categories/style.css	79 B
build/block-library/blocks/code/style-rtl.css	103 B
build/block-library/blocks/code/style.css	103 B
build/block-library/blocks/code/theme-rtl.css	124 B
build/block-library/blocks/code/theme.css	124 B
build/block-library/blocks/columns/editor-rtl.css	108 B
build/block-library/blocks/columns/editor.css	108 B
build/block-library/blocks/columns/style-rtl.css	406 B
build/block-library/blocks/columns/style.css	406 B
build/block-library/blocks/comment-author-avatar/editor-rtl.css	125 B
build/block-library/blocks/comment-author-avatar/editor.css	125 B
build/block-library/blocks/comment-content/style-rtl.css	92 B
build/block-library/blocks/comment-content/style.css	92 B
build/block-library/blocks/comment-template/style-rtl.css	187 B
build/block-library/blocks/comment-template/style.css	185 B
build/block-library/blocks/comments-pagination-numbers/editor-rtl.css	123 B
build/block-library/blocks/comments-pagination-numbers/editor.css	121 B
build/block-library/blocks/comments-pagination/editor-rtl.css	222 B
build/block-library/blocks/comments-pagination/editor.css	209 B
build/block-library/blocks/comments-pagination/style-rtl.css	235 B
build/block-library/blocks/comments-pagination/style.css	231 B
build/block-library/blocks/comments-title/editor-rtl.css	75 B
build/block-library/blocks/comments-title/editor.css	75 B
build/block-library/blocks/comments/editor-rtl.css	834 B
build/block-library/blocks/comments/editor.css	832 B
build/block-library/blocks/comments/style-rtl.css	632 B
build/block-library/blocks/comments/style.css	630 B
build/block-library/blocks/cover/editor-rtl.css	615 B
build/block-library/blocks/cover/editor.css	616 B
build/block-library/blocks/cover/style-rtl.css	1.55 kB
build/block-library/blocks/cover/style.css	1.55 kB
build/block-library/blocks/embed/editor-rtl.css	293 B
build/block-library/blocks/embed/editor.css	293 B
build/block-library/blocks/embed/style-rtl.css	410 B
build/block-library/blocks/embed/style.css	410 B
build/block-library/blocks/embed/theme-rtl.css	110 B
build/block-library/blocks/embed/theme.css	110 B
build/block-library/blocks/file/editor-rtl.css	300 B
build/block-library/blocks/file/editor.css	300 B
build/block-library/blocks/file/style-rtl.css	253 B
build/block-library/blocks/file/style.css	254 B
build/block-library/blocks/file/view.min.js	346 B
build/block-library/blocks/freeform/editor-rtl.css	2.44 kB
build/block-library/blocks/freeform/editor.css	2.44 kB
build/block-library/blocks/gallery/editor-rtl.css	948 B
build/block-library/blocks/gallery/editor.css	950 B
build/block-library/blocks/gallery/style-rtl.css	1.53 kB
build/block-library/blocks/gallery/style.css	1.53 kB
build/block-library/blocks/gallery/theme-rtl.css	108 B
build/block-library/blocks/gallery/theme.css	108 B
build/block-library/blocks/group/editor-rtl.css	333 B
build/block-library/blocks/group/editor.css	333 B
build/block-library/blocks/group/style-rtl.css	57 B
build/block-library/blocks/group/style.css	57 B
build/block-library/blocks/group/theme-rtl.css	78 B
build/block-library/blocks/group/theme.css	78 B
build/block-library/blocks/heading/style-rtl.css	76 B
build/block-library/blocks/heading/style.css	76 B
build/block-library/blocks/html/editor-rtl.css	327 B
build/block-library/blocks/html/editor.css	329 B
build/block-library/blocks/image/editor-rtl.css	736 B
build/block-library/blocks/image/editor.css	737 B
build/block-library/blocks/image/style-rtl.css	627 B
build/block-library/blocks/image/style.css	630 B
build/block-library/blocks/image/theme-rtl.css	110 B
build/block-library/blocks/image/theme.css	110 B
build/block-library/blocks/latest-comments/style-rtl.css	284 B
build/block-library/blocks/latest-comments/style.css	284 B
build/block-library/blocks/latest-posts/editor-rtl.css	213 B
build/block-library/blocks/latest-posts/editor.css	212 B
build/block-library/blocks/latest-posts/style-rtl.css	463 B
build/block-library/blocks/latest-posts/style.css	462 B
build/block-library/blocks/list/style-rtl.css	88 B
build/block-library/blocks/list/style.css	88 B
build/block-library/blocks/media-text/editor-rtl.css	266 B
build/block-library/blocks/media-text/editor.css	263 B
build/block-library/blocks/media-text/style-rtl.css	493 B
build/block-library/blocks/media-text/style.css	490 B
build/block-library/blocks/more/editor-rtl.css	431 B
build/block-library/blocks/more/editor.css	431 B
build/block-library/blocks/navigation-link/editor-rtl.css	705 B
build/block-library/blocks/navigation-link/editor.css	703 B
build/block-library/blocks/navigation-link/style-rtl.css	115 B
build/block-library/blocks/navigation-link/style.css	115 B
build/block-library/blocks/navigation-submenu/editor-rtl.css	296 B
build/block-library/blocks/navigation-submenu/editor.css	295 B
build/block-library/blocks/navigation-submenu/view.min.js	423 B
build/block-library/blocks/navigation/editor-rtl.css	2.03 kB
build/block-library/blocks/navigation/editor.css	2.04 kB
build/block-library/blocks/navigation/style-rtl.css	1.98 kB
build/block-library/blocks/navigation/style.css	1.97 kB
build/block-library/blocks/navigation/view-modal.min.js	2.78 kB
build/block-library/blocks/navigation/view.min.js	443 B
build/block-library/blocks/nextpage/editor-rtl.css	395 B
build/block-library/blocks/nextpage/editor.css	395 B
build/block-library/blocks/page-list/editor-rtl.css	363 B
build/block-library/blocks/page-list/editor.css	363 B
build/block-library/blocks/page-list/style-rtl.css	175 B
build/block-library/blocks/page-list/style.css	175 B
build/block-library/blocks/paragraph/editor-rtl.css	157 B
build/block-library/blocks/paragraph/editor.css	157 B
build/block-library/blocks/paragraph/style-rtl.css	260 B
build/block-library/blocks/paragraph/style.css	260 B
build/block-library/blocks/post-author/style-rtl.css	175 B
build/block-library/blocks/post-author/style.css	176 B
build/block-library/blocks/post-comments-form/editor-rtl.css	96 B
build/block-library/blocks/post-comments-form/editor.css	96 B
build/block-library/blocks/post-comments-form/style-rtl.css	493 B
build/block-library/blocks/post-comments-form/style.css	493 B
build/block-library/blocks/post-excerpt/editor-rtl.css	73 B
build/block-library/blocks/post-excerpt/editor.css	73 B
build/block-library/blocks/post-excerpt/style-rtl.css	69 B
build/block-library/blocks/post-excerpt/style.css	69 B
build/block-library/blocks/post-featured-image/editor-rtl.css	605 B
build/block-library/blocks/post-featured-image/editor.css	605 B
build/block-library/blocks/post-featured-image/style-rtl.css	153 B
build/block-library/blocks/post-featured-image/style.css	153 B
build/block-library/blocks/post-template/editor-rtl.css	99 B
build/block-library/blocks/post-template/editor.css	98 B
build/block-library/blocks/post-template/style-rtl.css	282 B
build/block-library/blocks/post-template/style.css	282 B
build/block-library/blocks/post-terms/style-rtl.css	73 B
build/block-library/blocks/post-terms/style.css	73 B
build/block-library/blocks/post-title/style-rtl.css	80 B
build/block-library/blocks/post-title/style.css	80 B
build/block-library/blocks/preformatted/style-rtl.css	103 B
build/block-library/blocks/preformatted/style.css	103 B
build/block-library/blocks/pullquote/editor-rtl.css	198 B
build/block-library/blocks/pullquote/editor.css	198 B
build/block-library/blocks/pullquote/style-rtl.css	370 B
build/block-library/blocks/pullquote/style.css	370 B
build/block-library/blocks/pullquote/theme-rtl.css	167 B
build/block-library/blocks/pullquote/theme.css	167 B
build/block-library/blocks/query-pagination-numbers/editor-rtl.css	122 B
build/block-library/blocks/query-pagination-numbers/editor.css	121 B
build/block-library/blocks/query-pagination/editor-rtl.css	221 B
build/block-library/blocks/query-pagination/editor.css	211 B
build/block-library/blocks/query-pagination/style-rtl.css	282 B
build/block-library/blocks/query-pagination/style.css	278 B
build/block-library/blocks/query/editor-rtl.css	439 B
build/block-library/blocks/query/editor.css	439 B
build/block-library/blocks/quote/style-rtl.css	213 B
build/block-library/blocks/quote/style.css	213 B
build/block-library/blocks/quote/theme-rtl.css	223 B
build/block-library/blocks/quote/theme.css	226 B
build/block-library/blocks/read-more/style-rtl.css	132 B
build/block-library/blocks/read-more/style.css	132 B
build/block-library/blocks/rss/editor-rtl.css	202 B
build/block-library/blocks/rss/editor.css	204 B
build/block-library/blocks/rss/style-rtl.css	289 B
build/block-library/blocks/rss/style.css	288 B
build/block-library/blocks/search/editor-rtl.css	165 B
build/block-library/blocks/search/editor.css	165 B
build/block-library/blocks/search/style-rtl.css	396 B
build/block-library/blocks/search/style.css	393 B
build/block-library/blocks/search/theme-rtl.css	114 B
build/block-library/blocks/search/theme.css	114 B
build/block-library/blocks/separator/editor-rtl.css	146 B
build/block-library/blocks/separator/editor.css	146 B
build/block-library/blocks/separator/style-rtl.css	233 B
build/block-library/blocks/separator/style.css	233 B
build/block-library/blocks/separator/theme-rtl.css	194 B
build/block-library/blocks/separator/theme.css	194 B
build/block-library/blocks/shortcode/editor-rtl.css	464 B
build/block-library/blocks/shortcode/editor.css	464 B
build/block-library/blocks/site-logo/editor-rtl.css	708 B
build/block-library/blocks/site-logo/editor.css	708 B
build/block-library/blocks/site-logo/style-rtl.css	192 B
build/block-library/blocks/site-logo/style.css	192 B
build/block-library/blocks/site-tagline/editor-rtl.css	86 B
build/block-library/blocks/site-tagline/editor.css	86 B
build/block-library/blocks/site-title/editor-rtl.css	84 B
build/block-library/blocks/site-title/editor.css	84 B
build/block-library/blocks/social-link/editor-rtl.css	184 B
build/block-library/blocks/social-link/editor.css	184 B
build/block-library/blocks/social-links/editor-rtl.css	674 B
build/block-library/blocks/social-links/editor.css	673 B
build/block-library/blocks/social-links/style-rtl.css	1.39 kB
build/block-library/blocks/social-links/style.css	1.38 kB
build/block-library/blocks/spacer/editor-rtl.css	322 B
build/block-library/blocks/spacer/editor.css	322 B
build/block-library/blocks/spacer/style-rtl.css	48 B
build/block-library/blocks/spacer/style.css	48 B
build/block-library/blocks/table/editor-rtl.css	494 B
build/block-library/blocks/table/editor.css	494 B
build/block-library/blocks/table/style-rtl.css	611 B
build/block-library/blocks/table/style.css	609 B
build/block-library/blocks/table/theme-rtl.css	175 B
build/block-library/blocks/table/theme.css	175 B
build/block-library/blocks/tag-cloud/style-rtl.css	239 B
build/block-library/blocks/tag-cloud/style.css	239 B
build/block-library/blocks/template-part/editor-rtl.css	235 B
build/block-library/blocks/template-part/editor.css	235 B
build/block-library/blocks/template-part/theme-rtl.css	101 B
build/block-library/blocks/template-part/theme.css	101 B
build/block-library/blocks/text-columns/editor-rtl.css	95 B
build/block-library/blocks/text-columns/editor.css	95 B
build/block-library/blocks/text-columns/style-rtl.css	166 B
build/block-library/blocks/text-columns/style.css	166 B
build/block-library/blocks/verse/style-rtl.css	87 B
build/block-library/blocks/verse/style.css	87 B
build/block-library/blocks/video/editor-rtl.css	561 B
build/block-library/blocks/video/editor.css	563 B
build/block-library/blocks/video/style-rtl.css	159 B
build/block-library/blocks/video/style.css	159 B
build/block-library/blocks/video/theme-rtl.css	110 B
build/block-library/blocks/video/theme.css	110 B
build/block-library/common-rtl.css	1.01 kB
build/block-library/common.css	1 kB
build/block-library/editor-elements-rtl.css	75 B
build/block-library/editor-elements.css	75 B
build/block-library/editor-rtl.css	10.9 kB
build/block-library/editor.css	10.9 kB
build/block-library/elements-rtl.css	54 B
build/block-library/elements.css	54 B
build/block-library/reset-rtl.css	478 B
build/block-library/reset.css	478 B
build/block-library/style-rtl.css	11.9 kB
build/block-library/style.css	11.9 kB
build/block-library/theme-rtl.css	695 B
build/block-library/theme.css	700 B
build/block-serialization-default-parser/index.min.js	1.11 kB
build/block-serialization-spec-parser/index.min.js	2.83 kB
build/blocks/index.min.js	47.3 kB
build/compose/index.min.js	12 kB
build/core-data/index.min.js	15.2 kB
build/customize-widgets/index.min.js	11.3 kB
build/customize-widgets/style-rtl.css	1.4 kB
build/customize-widgets/style.css	1.4 kB
build/data-controls/index.min.js	653 B
build/data/index.min.js	8.03 kB
build/date/index.min.js	32 kB
build/deprecated/index.min.js	507 B
build/dom-ready/index.min.js	324 B
build/dom/index.min.js	4.69 kB
build/edit-navigation/index.min.js	16 kB
build/edit-navigation/style-rtl.css	4.02 kB
build/edit-navigation/style.css	4.03 kB
build/edit-post/classic-rtl.css	546 B
build/edit-post/classic.css	547 B
build/edit-post/index.min.js	30.5 kB
build/edit-post/style-rtl.css	6.94 kB
build/edit-post/style.css	6.94 kB
build/edit-site/index.min.js	56.9 kB
build/edit-site/style-rtl.css	8.23 kB
build/edit-site/style.css	8.22 kB
build/edit-widgets/index.min.js	16.5 kB
build/edit-widgets/style-rtl.css	4.35 kB
build/edit-widgets/style.css	4.35 kB
build/editor/index.min.js	41.4 kB
build/editor/style-rtl.css	3.66 kB
build/editor/style.css	3.65 kB
build/element/index.min.js	4.68 kB
build/escape-html/index.min.js	537 B
build/format-library/index.min.js	6.75 kB
build/format-library/style-rtl.css	571 B
build/format-library/style.css	571 B
build/hooks/index.min.js	1.64 kB
build/html-entities/index.min.js	448 B
build/i18n/index.min.js	3.77 kB
build/is-shallow-equal/index.min.js	527 B
build/keyboard-shortcuts/index.min.js	1.78 kB
build/keycodes/index.min.js	1.79 kB
build/list-reusable-blocks/index.min.js	1.74 kB
build/list-reusable-blocks/style-rtl.css	835 B
build/list-reusable-blocks/style.css	835 B
build/media-utils/index.min.js	2.93 kB
build/notices/index.min.js	953 B
build/nux/index.min.js	2.05 kB
build/nux/style-rtl.css	732 B
build/nux/style.css	728 B
build/plugins/index.min.js	1.94 kB
build/preferences-persistence/index.min.js	2.22 kB
build/preferences/index.min.js	1.3 kB
build/primitives/index.min.js	933 B
build/priority-queue/index.min.js	612 B
build/react-i18n/index.min.js	696 B
build/react-refresh-entry/index.min.js	8.44 kB
build/react-refresh-runtime/index.min.js	7.31 kB
build/redux-routine/index.min.js	2.74 kB
build/reusable-blocks/index.min.js	2.21 kB
build/reusable-blocks/style-rtl.css	256 B
build/reusable-blocks/style.css	256 B
build/rich-text/index.min.js	11.1 kB
build/server-side-render/index.min.js	1.61 kB
build/shortcode/index.min.js	1.53 kB
build/token-list/index.min.js	644 B
build/url/index.min.js	3.61 kB
build/vendors/react-dom.min.js	38.5 kB
build/vendors/react.min.js	4.34 kB
build/viewport/index.min.js	1.08 kB
build/warning/index.min.js	268 B
build/widgets/index.min.js	7.19 kB
build/widgets/style-rtl.css	1.16 kB
build/widgets/style.css	1.16 kB
build/wordcount/index.min.js	1.06 kB

_{compressed-size-action}

[talldan]

BTW - I notice in WordPress core, the custom color popover is always to the left of the gradient popover (it seems anchored to the popover contents, judging by how it sits just below the top padding, and I think it would be placement left-start):

Maybe that's the desired behavior? Hopefully Joen can confirm.

[jasmussen]

Thank you for the welcome! This is a tricky one. Here's a GIF showing the desired test behavior:

The color picker opens downward and to the right from the opening swatch (cropped by the GIF).

This is in comparison to trunk, where the picker seems arbitrarily positioned:

The desired behavior in the past was for each popover to open in a stacking behavior leftwards from the ItemGroup button — swatch popover first, then the color popover left of that. A bit like this inspector mockup where I hacked the CSS:

My opinion: that hasn't been entirely successful.

• The color popover has a unique borderless and frameless design that, when stacked, looks a little different
• The color context is confused, relying on an active state for the gradient
• Povers opening from popovers feels like too much

An older mockup, is to enable the swatch panel to slide rightwards to the color picker:

It solves the popover in popover problem, but it's still not clear that would be a better user experience, moreover, it's likely going to be a lot of work. So outside of a larger effort, we can either land this PR and drop the stacking behavior, or see if we can restore it to its original behavior (the inspector mockup above).

Question: is there a substantial code quality improvement in dropping the stacking behavior? I imagine there might be, and if we do go that way, I'd prefer prefer the popover open down and center, rather than down and right. Like so:

I'm keen on other opinions.

[ciampo]

Hey @jasmussen , I made some changes, and the popover should now open below (and centred) under each control point in the gradient bar, if I understood this piece of feedback correctly:

I'd prefer prefer the popover open down and center, rather than down and right. Like so:

Otherwise, I'm also happy to try and replicate what you called the "original behavior":

The desired behavior in the past was for each popover to open in a stacking behavior leftwards from the ItemGroup button — swatch popover first, then the color popover left of that. A bit like this inspector mockup where I hacked the CSS:

With respect to a deeper refactor, I personally never liked having "nested" popovers, and so I'd be in favour of having something similar to the older mockup that you shared, where the swatch panel slides rightwards to the color picker (although, as you guessed, that would definitely be more than a simple refactor).

[jasmussen]

Thank you for the fast and impressive work, and thank you Dan for the very helpful pointers towards the changes beyond just gradients.

For the gradients specifically, they do feel more contextual with the popover:

I failed to think about the other places where this component is used, and per Dan's pointers, they aren't optimal:

(Yes Duotone should probably always open control points downwards)

Specifically this one, clicking the main color swatch to customize it, feels confusing. That does make me feel like we should seek back to the original behavior. Sorry about the detour.

On a separate track with less urgency, yes, we should still think about how we can improve this flow overall. Whether that's the sliding interface or something else — this PR has helped put into clarity what needs to be considered here. Thanks again.

[ciampo]

Alright, I've applied some of the suggested changes: in particular, with the latest changes in this PR all dropdowns opening within a GradientPicker or a ColorPalette currently open below the clicked anchor, regardless of whether they are rendered in the sidebar or not.

This is the thing: with the new desired behavior, the __experimentalIsInSidebar prop is not useful anymore to determine which configuration (placement/offset...) we should apply to the popover — for example, the scenario described by @jasmussen above also happens in the sidebar:

Specifically this one, clicking the main color swatch to customize it, feels confusing. That does make me feel like we should seek back to the original behavior. Sorry about the detour.

This also links well with @talldan 's comment above. From what I gathered, in order to achieve the new desired behavior we'd need to:

• remove the __experimentalIsInSidebar prop from the ColorPalette, CustomGradientBar, CustomGradientPicker, GradientPicker, PaletteEdit components, and from all of the consumers of these components (since it's not useful anymore)
• expose a new prop on these components that will allow us to customize the position of the popover — the most flexible way to achieve that is to literally accept a childPopoverProps or colorPickerPopoverProps prop, where we can set any desired popover behavior

This is definitely a much larger job than the current size of the PR, and therefore I'll wait for everyone's feedback on the proposed solution before working on it.

It would be great if we could avoid the many layers of prop drilling the currently are needed to forward the __experimentalIsInSidebar prop all the way down to the low-level components, and so if anyone has a better alternative to adding a new prop, that would be great.

[jasmussen]

Just to take stock of the current status. Here's what trunk looks like:

Here's what this branch looks like:

As discussed on the thread, there's a lot to fix in terms of popover appearances and positions, though I did note a slight preference for opening like trunk does at least for colors. However as is also clear, the bug to be fixed is the one for color stops in the gradient.

In order to fix that, is it fine that we open down/center from the opening color stops across the board? Probably, yes. Especially so we can land a fix for 6.1. I'd love a quick input from @javierarce or @jameskoster if they have alternative ideas, though.

One thing we'd need to fix if we go with this approach, though, is that the animation indicates a top left origin, and it should indicate a top center origin. It's hard to see with the fast animation, but if you look for it, it's there. Nice work.

[jameskoster]

I've always found it a bit confusing that we combine presets and custom values in the same panel. We could eliminate the double popovers altogether if we did something like:

I would probably combine preset solids and gradients too, but that's another story.

Anyways...

In order to fix that, is it fine that we open down/center from the opening color stops across the board

The overlapping popovers feels a bit awkward for solid colors, but not necessarily a regression. It feels like a fair trade-off to fix the original bug.

[ciampo]

I will also try to summarize the current situation, in case we can get to a simpler solution for the purpose of this PR (which originally aimed at being a "quick" bug fix):

• Currently on trunk, when we display a ColorPicker inside a Popover, we have two "configurations" that are chosen based on whether the popover is rendered in the sidebar or not:
  • If it's rendered in the sidebar, the popover's placement is supposed to be left-start (i.e. to the left, and with the top edge of the popover aligned with the top edge of the anchor) with respect to its anchor
  • If it's rendered outside of the sidebar, the popover is supposed to render above its anchor
  • The anchor depends on each "widget" and can also change based on the isRenderedInSidebar check: sometimes the anchor is the individual "circle" (single color entries in the color palette, or control points in a gradient picker), sometimes it's the larger "rectangle" (the whole clickable color indicator, or the whole gradient bar)
• The limit with this system is that, as explained above, we are currently able to only pick between 2 "configurations" based on the isRenderedInSidebar check. And so, if we want the popover to be configured the same way as when in the sidebar also when it's used by the Duotone tools outside of the sidebar, it means that:
  • We either have to use the same "configuration" for all color picker popovers
  • We need to spend some time refactoring the code to allow for a finer grain control of the popover configuration that is more flexible than a simple isRenderedInSidebar check

Currently, the behaviour of the color picker popover in trunk can also be a bit weird and inconsistent because of the regressions introduced by #40740 (which we're working on fixing). For the sake of keeping this PR focused on fixing the initial bug, we could also find a solution that still relies on having two configurations that we switch based on the isRenderedInSidebar check. We could aim at restoring the original intended behavior, ie.:

• if the popover is in the sidebar, we do what @jasmussen referred to as the "original desired behavior": each popover to open in a stacking behavior leftwards
• if the popover is not in the sidebar (including duotone), we pick a placement for the popover (which used to be on top of the anchor, if there's enough space)

[talldan]

The limit with this system is that, as explained above, we are currently able to only pick between 2 "configurations" based on the isRenderedInSidebar check.

I think there's the possibility of three configurations:

• The GradientColorPickerDropdown version of CustomColorPickerDropdown can have its own behavior by overriding the propoverProps. Currently it seems like the discussion has leaned towards always opening the popover below the control points.
• Standard versions of CustomColorPickerDropdown that don't have an override of popoverProps can have two behaviors depending on isRenderedInSidebar. The discussion has suggested opening to the left of the parent popover when isRenderedInSidebar is true, but I'm not sure how it should appear when isRenderedInSidebar is false. Below?

[ciampo]

@talldan's proposed solution makes sense to me — I went ahead and tried to implement it with my latest changes.

Here's what the "color picker in a popover" behaves like across the editor:

https://user-images.githubusercontent.com/1083581/184196225-983e37aa-19d8-4258-bb97-4b85b26ecc80.mp4

What do folks think?

[jasmussen]

Thanks for your effort. If the code isn't too painful, I think that's an excellent path forward, even if we know we want to revisit this in the future.

If you can, I'd love to put 12px of space between the color picker and the swatch palette, though — just like the space between the ItemGroup item and the swatch palette. And the color picker still opens from top left, as opposed to top center, for the gradient stops.

Nice work!

[ciampo]

Hey @jasmussen , I've updated the offset between the color picker and the swatch palette:

And the color picker still opens from top left, as opposed to top center, for the gradient stops.

I will work on improving the popover opening animations in a follow-up PR.

I believe this PR is ready for a final review and hopefully we can go ahead and merge it!

[jasmussen]

Screenshot looks good to me (maybe give or take some border math, is it slightly larger than the space between palette and itemgroup?) but nothing to block it.

I tried compiling the branch but for whatever reason I'm still seeing the old spacing. Probably something on my side. But in any case, screenshot looks good!

[ciampo]

(maybe give or take some border math, is it slightly larger than the space between palette and itemgroup?) but nothing to block it.

It could be — nothing that we can't tweak in a follow-up if necessary (in case you wanted to test it on your machine, these would be the lines of code responsible for positioning that popover)

Do you think this PR is good enough to get an approval and merge?

[ciampo]

I will work on improving the popover opening animations in a follow-up PR.

Opened https://github.com/WordPress/gutenberg/pull/43186 (I had already started work on this https://github.com/WordPress/gutenberg/pull/42531 and so it was quite quick to put the PR together)