docs(popover): fix appendTo where popovers are hidden

[MariaAga]

What: Closes #8689

[patternfly-build]

Preview: https://patternfly-react-pr-8802.surge.sh

A11y report: https://patternfly-react-pr-8802-a11y.surge.sh

[nicolethoen]

This does fix the problem - i'm just a little torn.
Ideally, we'd always want popovers to be appended inline, primarily for accessibility reason, so that the popover is as close to its trigger as we can get it in the DOM.
Popovers are supposed to be smart enough to recognize when there is not enough space for them to display themselves and reposition themselves. But I recognize that this is a weird case where the sidebar is overhanging a large margin and I don't know if the popover is smart enough to recognize that something is overhanging it.
I wonder if this is something that the popper has managed to solve and just are not leveraging correctly?

[MariaAga]

We can instead of change the appendTo change the z-index of the sidebar to be lower than the main section if that makes more sense? or if that makes more sense should we change thee z-index for page and side bar in its core css

[nicolethoen]

I think the side effects of lowering the z-index of the sidebar or increasing the z-index of the page section would be pretty great. I don't think we can do that. Likely if we cannot make the Popover smart enough to know that the sidebar is obscuring it and to reposition itself, then changing the appendTo prop is the better dirty fix.

Maybe we could change the z-index of the popover in this specific example?

[MariaAga]

Since the popover (before this pr) is appended to the main section it cant have a higher z-index than the side bar

[nicolethoen]

Curious if @thatblindgeye or @tlabaj have an opinion.
@kmcfaul do you think there is any way to do this by making the popper just a bit more aware?

[thatblindgeye]

Yeah this is tricky. Looking at it, another option might be updating these examples (probably all of them to be consistent) so that the style prop on the containing div is, style={{ margin: '50px', position: 'relative' }}. Downside there is it means we have to recommend wrapping the popover in such a div which may not be desired, but it does give the popover something closer to where it's appended in the DOM to be absolutely positioned to.

Adding position: relative to the pf-c-page__main element also seemed to work, but that may cause other issues with other components that aren't as easily visible immediately.

Main concern with having examples append to the document body is making sure we explain when it might be needed, and also explaining what else the consumer needs to do when doing so (probably only when they might also be utilizing the shouldOpen and shouldClose props as well). That would probably live in the a11y docs and would be needed anyway, though.

[kmcfaul]

I've looked at Popper code and it seems that it tries to get an 'offset parent' to calculate the x and y offsets from that. It might be a place we can adjust to account for other elements that aren't direct ancestors like the sidebar but I'm not 100% sure. My other thought since it is the auto-width prop that seems to cause this issue is that the position is getting calculated before the overflow styling is applied which shifts the popper.

I agree that adding the wrapping div would be a surefire way to get the styling to work and would give explicit boundaries for the popper, but we'd definitely need the extra documentation around usage. What also might work is to adjust the z-index so the popper sits on top of the sidebar, but that maybe isn't quite the desired behavior and may have collisions with other component z-index props.

[MariaAga]

Should the documentation for the div wrapper go under the Examples title, or above it as an alert?
As stated above, we probably shouldnt change the main section z-index for the popper to work

[nicolethoen]

I think the documentation should go below the Examples title - likely it should be placed above the first example that needs it, right under that example's title.

If it's needed for every example, we could place a note about our specific use of a wrapper for the examples at the top of the docs right after Example as you suggested.

[MariaAga]

I checked the position: 'relative' solution again and its making the popper and page jump on toggle, sometimes it would scroll the users page and sometimes the alert would show in the top of the page and then jump down after less then a second:

So Im guess were back to deciding between changing the page/side nav z-index or sometimes appending the popper to the body

[thatblindgeye]

I'm not able to reproduce that issue currently (tried Chrome and Firefox), but if it's an issue likely to occur then yeah we might be back to either tinkering with the z-index or appending to document body. @kmcfaul if the z-index of the popper can easily be adjusted, could that just be something we warn about, that the consumer may need to adjust z-indexes depending on their specific use-case?

If appending to the document body is the best solution for now, we'd just need the warning about that in a11y docs once they're written (would make sense to provide a warning somewhere on this examples page as well; maybe mentioning other alternatives that should be attempted first, like adjusting z-indexes, adding a wrapper div with position relative, etc).

[MariaAga]

Thanks for the input, I added some info in the docs hope its clear for users

[nicolethoen]

@MariaAga could you rebase this with the latest on v5? It will fix the styles in the generated preview here and make it easier to review.

[MariaAga]

rebased, test failure is not related to this pr

[nicolethoen]

you're right! a change just merged to fix that test.
If you rebase one more time, then it should all pass now