Popover: polish component after recent refactor

[ciampo]

The recent refactor of the Popover component to use floating ui has caused a few regressions, and still feels partially incomplete.

Known regressions (either issues, or PRs containing a fix):

• [x] https://github.com/WordPress/gutenberg/pull/41312
• [x] https://github.com/WordPress/gutenberg/issues/41353
• [x] https://github.com/WordPress/gutenberg/issues/41894
• [x] https://github.com/WordPress/gutenberg/pull/41268
• [x] https://github.com/WordPress/gutenberg/issues/42003
• [x] https://github.com/WordPress/gutenberg/pull/42076
• [x] https://github.com/WordPress/gutenberg/issues/42631
• [x] https://github.com/WordPress/gutenberg/issues/42820
• [x] https://github.com/WordPress/gutenberg/pull/42886
• [x] https://github.com/WordPress/gutenberg/pull/42971
• [x] https://github.com/WordPress/gutenberg/pull/42989
• [x] https://github.com/WordPress/gutenberg/pull/43172
• [x] https://github.com/WordPress/gutenberg/pull/43186
• [x] https://github.com/WordPress/gutenberg/pull/43267
• [x] https://github.com/WordPress/gutenberg/pull/43329
• [x] https://github.com/WordPress/gutenberg/pull/43332
• [x] https://github.com/WordPress/gutenberg/pull/43377
• [x] https://github.com/WordPress/gutenberg/pull/43297
• [x] https://github.com/WordPress/gutenberg/issues/41575 : related PRs:
  • [x] https://github.com/WordPress/gutenberg/pull/42417
  • [x] https://github.com/WordPress/gutenberg/pull/42887
  • [x] https://github.com/WordPress/gutenberg/pull/43162
  • [x] https://github.com/WordPress/gutenberg/pull/44323
• [x] https://github.com/WordPress/gutenberg/issues/42725
  • [x] https://github.com/WordPress/gutenberg/pull/43544
  • [x] https://github.com/WordPress/gutenberg/pull/43617
• [x] https://github.com/WordPress/gutenberg/issues/43541
  • [x] https://github.com/WordPress/gutenberg/pull/43546
  • [x] https://github.com/WordPress/gutenberg/pull/43548
• [x] https://github.com/WordPress/gutenberg/pull/43335
• [x] https://github.com/WordPress/gutenberg/issues/43063
• [x] https://github.com/WordPress/gutenberg/issues/41823
  • [x] https://github.com/WordPress/gutenberg/pull/43971
• [x] https://github.com/WordPress/gutenberg/issues/41739
  • [x] Fix: https://github.com/WordPress/gutenberg/pull/44282
• [x] https://github.com/WordPress/gutenberg/issues/44220
  • [x] https://github.com/WordPress/gutenberg/pull/44301
• [x] https://github.com/WordPress/gutenberg/pull/42950
• [ ] https://github.com/WordPress/gutenberg/issues/44281
• [ ] https://github.com/WordPress/gutenberg/issues/43678

Improvements wish list:

• [x] Improve code readability by adding more inline comments (#42944)
• [x] Improve Storybook examples (#42903)
• [x] #43191
  • [x] https://github.com/WordPress/gutenberg/pull/43546
  • [x] https://github.com/WordPress/gutenberg/pull/43845
• [x] There are currently many ways of passing an anchor, we should settle on a few, refactor all components and mark the other legacy approaches as deprecated
  • [x] #43691
• [ ] Legacy position prop:
  • [ ] #44339
    • [ ] #44377
  • [ ] #44401
  • [ ] #44386
• [ ] We should untangle the different onClose-like callbacks and unify them to just one callback (link)
• [ ] We should refactor all usages of the Popover using CSS margins and try to refactor them to use the offset prop instead
• [ ] Similarly, we should move away from the two other __unstable props:
  • [x] __unstableObserveElement (included in https://github.com/WordPress/gutenberg/pull/43617)
  • [ ] __unstableSlotName
• [ ] We should replace isAlternate with a more generic variant prop (see convo)
• [ ] Find a better way to make widget editor popovers use position: fixed. (see https://github.com/WordPress/gutenberg/pull/44282 for context)
• [ ] In the context of all these changes, we could:
  • [ ] rewrite the component to TypeScript and update the README
    • [x] #43823
    • [ ] #44373
  • [ ] rewrite the component's styles to use Emotion
  • [ ] rewrite the current unit tests with modern testing library features, and add more unit tests

[renatho]

Hey! I imagine it's probably related to what I'm facing with the Popover arrow.

Using WP 6.0 with Gutenberg plugin deactivated (working):

With Gutenberg (trunk) plugin activated (not working):

[ciampo]

Hey! I imagine it's probably related to what I'm facing with the Popover arrow.

Hey @renatho , it could be related — could you open a separate issue for this regression (where you can add more details and we can discuss further), so that we keep this issue as a master tracking issue? Thank you!

[renatho]

Hey! I imagine it's probably related to what I'm facing with the Popover arrow.

Hey @renatho , it could be related — could you open a separate issue for this regression (where you can add more details and we can discuss further), so that we keep this issue as a master tracking issue? Thank you!

Sure! Here is the issue: https://github.com/WordPress/gutenberg/issues/42820

[talldan]

I've added https://github.com/WordPress/gutenberg/issues/42725 to the list. Hope that's ok!

[ciampo]

I've added #42725 to the list. Hope that's ok!

Of course it is, thank you!

I must flag that I currently don't really have the capacity to work on this. I can try to squeeze some time and help, but we definitely need to understand who is going to work on these fixes

[talldan]

The popover's offset doesn't update when the block/pattern inserter is closed (https://github.com/WordPress/gutenberg/pull/42417#issuecomment-1198048684) (needs to be confirmed if the regression is caused by the refactor to floating UI)

I think I know what's happening here. There's no 'resize' listener for frameOffset, so the value isn't updated. It looks like it works ok when opening the sidebar, but this is actually the 'shift' middleware being triggered by the block's position changing.

The bug seems to specifically happen when inserting a new block and closing the sidebar, and I think that happens because the inserted block's toolbar is newly mounted, and a fresh frameOffset is calculated while the sidebar is open. Then, when closing the sidebar the frameOffset still retains the incorrect value.

I can work on a fix for this (edit: fix in #43172).

Interestingly, I also just noticed this in the floating-ui docs: https://floating-ui.com/docs/react-dom#variables-inside-middleware-functions

It suggests that any variable accessed through a closure by middleware functions needs to be a ref. That would also apply to frameOffset.

[tellthemachines]

👋 I just created #43541 for what seems to be a related issue; may be related to the recently merged #42887.

[talldan]

There are a couple of other issues here in the widgets customizer - Popover component overlay not fully visible in Customizer