Popover: Allow legitimate 0 positions to update popover position

[andrewserong]

What?

Fix an issue with the Popover component where 0 values for left (or top) would not update the position of the popover component, as the final value was treated as undefined instead of 0.

Why?

This resolves a tricky edge case that sometimes occurs with the list view drop indicator line under specific conditions (more info in the testing instructions). But the idea behind this PR is that we most likely don't want to be passing undefined for the final style of the popover, or we risk persisting stale positions, as occurs in the list view drop indicator.

How?

Ensure that 0 values are always applied to the style output of the popover component.

It looks like the issue was introduced in #46187 with some discussion occurring in https://github.com/WordPress/gutenberg/pull/46187/files#r1036857416. The fix here borrows from the idea in #45545 (comment) but doesn't include an undefined fallback, as Math.round() never returns null or undefined, as far I can tell.

Testing Instructions

These testing instructions are quire specific, as it's a tricky to reproduce issue:

To test, in a fresh post or page, add the following markup:

Some heavily nested block markup

<!-- wp:group {"layout":{"type":"constrained"}} -->
<div class="wp-block-group"><!-- wp:paragraph -->
<p>A paragraph in a group level 1</p>
<!-- /wp:paragraph -->

<!-- wp:group {"layout":{"type":"constrained"}} -->
<div class="wp-block-group"><!-- wp:paragraph -->
<p>A paragraph in a group level 2</p>
<!-- /wp:paragraph -->

<!-- wp:group {"layout":{"type":"constrained"}} -->
<div class="wp-block-group"><!-- wp:paragraph -->
<p>A paragraph in a group level 3</p>
<!-- /wp:paragraph -->

<!-- wp:group {"layout":{"type":"constrained"}} -->
<div class="wp-block-group"><!-- wp:paragraph -->
<p>A paragraph in a group level 4</p>
<!-- /wp:paragraph -->

<!-- wp:group {"layout":{"type":"constrained"}} -->
<div class="wp-block-group"><!-- wp:paragraph -->
<p>A paragraph in a group level 5</p>
<!-- /wp:paragraph -->

<!-- wp:group {"layout":{"type":"constrained"}} -->
<div class="wp-block-group"><!-- wp:paragraph {"anchor":""} -->
<p>A paragraph in a group level 6</p>
<!-- /wp:paragraph -->

<!-- wp:group {"layout":{"type":"constrained"}} -->
<div class="wp-block-group"><!-- wp:paragraph {"anchor":""} -->
<p>A paragraph in a group level 7</p>
<!-- /wp:paragraph -->

<!-- wp:group {"layout":{"type":"constrained"}} -->
<div class="wp-block-group"><!-- wp:paragraph {"anchor":""} -->
<p>A paragraph in a group level 8</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group --></div>
<!-- /wp:group --></div>
<!-- /wp:group --></div>
<!-- /wp:group --></div>
<!-- /wp:group --></div>
<!-- /wp:group --></div>
<!-- /wp:group --></div>
<!-- /wp:group -->

<!-- wp:heading -->
<h2 class="wp-block-heading">A heading</h2>
<!-- /wp:heading -->

<!-- wp:paragraph -->
<p>A paragraph</p>
<!-- /wp:paragraph -->

Prior to applying this PR, test the following (it might be easier to watch the steps in the screengrabs below):

• Open up the post or page with the above markup
• Open the list view
• Click on the "paragraph in group level 8" block in the editor canvas to expand it
• Click on the last paragraph in the editor canvas and press ENTER a number of times to add enough paragraph blocks to the list so that a vertical scrollbar is created in the list view — this action causes the horizontal scroll position of the list view to line up perfectly with the left edge of the list view items
• Without scrolling the list view horizontally, click and drag a list view item up to the position of the nested blocks, and then drag back down to the bottom-most blocks — notice that the list view drop indicator is unexpectedly indented to the right, matching the nested blocks instead of the root blocks. This is because the drop indicator is attempting to render at position 0 for the left position, but the drop indicator is retaining the previous x position styling.
• Apply this PR and retry the above steps.
• The drop indicator line should not be unexpectedly indented.

Note: if you are finding the issue difficult to reproduce, make sure that prior to adding the paragraph blocks, the list view is short enough that it does not yet have a vertical scrollbar. Changing the horizontal position of the list view is likely to make the list view items slightly off from flush with the left edge of the screen, so the issue won't appear.

Screenshots or screencast

Before

2023-06-08.16.45.35.mp4

After

2023-06-08.16.44.53.mp4

[andrewserong]

I see the snapshot tests need updating, but I'm not sure if it flags that this PR might introduce a regression? It seems that this change might result in transform: translateZ(0); being striped from the URLPopover? As in here:

Would love some feedback on that, as I'm not too sure what the desired behaviour is. My main goals for this PR is for us to allow 0 as a real value for the popover positioning if we can.

[andrewserong]

On second thoughts, perhaps this side of the ternary should be undefined as prior to this PR, the || meant that nullish or NaN values resulted in undefined... I can give that a try tomorrow, as that might potentially resolve the issue with the failing snapshot tests.

[ciampo]

To sum up, here are the different versions for this algorithm tried so far:

• this pr: c === null || Number.isNaN( c ) ? 0 : Math.round( c )
• trunk (which introduced the regression): Math.round( c ?? 0 ) || undefined
• old version: Number.isNaN( c ) ? 0 : c ?? undefined

I guess we need to understand exactly in which scenario we'd need to have undefined instead of 0. @corentin-gautier maybe can also help, since he worked on #46187 ?

[andrewserong]

Thanks for outlining each of the different options. To try to be more conservative about it, I've updated this logic in this PR to c === null || Number.isNaN( c ) ? undefined : Math.round( c ) so that we're only outputting a number when we have a real value. I think that might get us to a smaller potential change? Happy to revert if need be.

[ciampo]

That looks good to me. Looking at the snapshot changes, it's interesting that motion stops applying a transform on the Z axis — it must be some internal framer motion optimisation. From past experience, usually the reason for having translateZ(0) would be to promote an element to a separate composite layer, thus reducing the amount of times it needs to be re-painted to screen. But I can see that we're already applying the will-change: transform style, which also achieves the same goal, so we should also be fine under that point of view.

[ciampo]

Thank you for working on this! I pushed a couple of commits that refactor the logic to a separate function and add unit tests, so that we can use those tests to spec out exactly what behavior we'd like to achieve for this logic.

[andrewserong]

I pushed a couple of commits that refactor the logic to a separate function and add unit tests, so that we can use those tests to spec out exactly what behavior we'd like to achieve for this logic.

Excellent, thank you @ciampo, that makes things much neater! I've just pushed a tiny change and added a comment. I've updated the snapshot tests for URLPopover, too.

While manually testing URLPopover, I noticed that on trunk its position is incorrect when used in the MediaPlaceholder component. That seems unaffected by this PR, but I've opened up a separate PR to look at fixing that over in #51363.

[github-actions]

Flaky tests detected in 463041f.
Some 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.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5218630409
📝 Reported issues:

• [Flaky Test] should allow to return Global Styles root when example is clicked #50799 in /test/e2e/specs/site-editor/style-book.spec.js

[ciampo]

The code changes LGTM, although I've been struggling to repro the bug and therefore to test the fix specifically in the list view. It would be great if we'd get one final confirmation that the bug is indeed fixed the current set of changes from this PR before merging :)

[apeatling]

Great fix! This is working well for me now.

[andrewserong]

Wonderful, thanks for the collaboration @ciampo and for the review @apeatling! 🙇