Popover: fix inserter glitch

[ellatrix]

Description

Fixes #18953.

Since #18813, the popover content height it calculated with scrollHeight, but this doesn't work properly if the content itself contains another scroll container. As a result, if you open the sibling inserter, the content height will continuously change, causing the content to move up and down every half second.

The solution is to revert to using getBoundingClientRect without maxHeight set. This can be done right at the start, and for performance reasons at every interval after that because we have to remove maxHeight first to get the bounding client rect.

How has this been tested?

Screenshots

Types of changes

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR. .

[epiqueras]

As a result, if you open the sibling inserter, the content height will continuously change, causing the content to move up and down every half second.

I can't seem to reproduce this. Is this browser specific?

[ellatrix]

@epiqueras No, I think it's for all browsers. It just got reported here: #18953.

[youknowriad]

This fixes that particular issue but there's still other glitches, for example, if you open the "More Menu" (top right) in a screen with a small height and then scroll to the bottom of the menu, you'll notice that it will jump to the top again.

[ellatrix]

@youknowriad Ah, I see what you mean. It looks like we cannot measure the height after the first time, then, so I'll revert it back to the old behaviour of only measuring once at the start. This will introduce the problem with the "Block Settings" ellipsis menu again though. We have to figure out why the height is not measured correctly at the start.

[youknowriad]

yeah, hard problem. Maybe the content is not ready on first render for some reason for these popovers.

[ellatrix]

@youknowriad It should be fixed now. Looks like at the animation frame the height is good for both popovers.

[youknowriad]

This is magic

[ellatrix]

Ugh, this makes a number of jest tests fail that rely on having the popover visible immediately.

[ellatrix]

I'm not sure what to do about the tests here. For the tests, a later refresh fails them. I don't have a good understanding of the react DOM test utils and I'm not sure what they fail...

[youknowriad]

Maybe @gziolo can help here :)

[ellatrix]

The refresh on animation frame is somehow confusing the tests. If I use setTimeout it works fine.

[youknowriad]

@ellatrix there's some tricks here jestjs/jest#5147

[ellatrix]

It looks like a timeout (0) works just fine as well and leaves the tests working. Let's go for that then. I suspect the underlying height problem is the same as:

gutenberg/packages/components/src/popover/index.js

Lines 134 to 140 in c75761c

	/*
	* Without the setTimeout, the dom node is not being focused. Related:
	* https://stackoverflow.com/questions/35522220/react-ref-with-focus-doesnt-work-without-settimeout-my-example
	*
	* TODO: Treat the cause, not the symptom.
	*/
	const focusTimeout = setTimeout( () => {

Hopefully we find can figure out a proper solution in a follow up PR. Cc @aduth since you created #11430.

[epiqueras]

Nice work!

Interestingly, I was never able to replicate #18953 on my machine.

[youknowriad]

@epiqueras I did reproduce before this PR, i think it was related to viewport size somehow.

[epiqueras]

Ah, that could be it!