Refactor PopupBox placement to respect configurable elevation

[nicolaihenriksen]

Fixes #3129

I set out to simply add ElevationAssist.Elevation support for the PopupBox, but I ended up opening "a can of worms".

UPDATE: After discussing this, we agreed not to use ElevationAssist.Elevation, but rather create a dedicated PopupBox.Elevation.

When applying a drop shadow to the Card contained in the PopupBox, the Popup that is displayed, needs to reserve space for said drop shadow; it cannot draw outside its bounds. To solve this problem, a Margin matching the drop shadow size is applied to the Card and this is then compensated for in the placement of the Popup.

The PopupBox also supports custom offsets of the Popup set by the calling code. Previously these were directly applied to the Popup.HorizontalOffset/Popup.VerticalOffset properties, and were then needed to be taken into consideration when calculating the custom popup placement. This made the calculations a bit funky; in fact I don't believe they were correct in all cases. It definitely did not support DPI scaling correctly, and I also believe it had issues with some alignments in FlowDirection=RightToLeft.

This PR refactors this such that the Popup.HorizontalOffset/Popup.VerticalOffset are only used to compensate for the drop shadow size. The custom offsets that can be applied (PopupBox.PopupHorizontalOffset and PopupBox.PopupVerticalOffset) are now included in the calculations in the PopupBox.GetPopupPlacement() callback instead. This callback is also refactored to adjust for DPI scaling and respect FlowDirection for all alignment cases.

I added a dedicated page in the demo app for the PopupBox to allow for manual testing of the various scenarios.

[nicolaihenriksen]

@Keboo I have left a few comments above. 2 of them surprised me a bit and may be something that we need to look into.

The last one is simply an elaboration on a mitigation of a drop shadow clipping issue to explain why it is like it is. This scenario was also present before my changes, but I struggle to see how we can "fix" it such that the user does not need to worry about it. One option is to always "reserve space" using the largest possible drop-shadow radius, but in case the users content does not have drop shadows at all, we will be covering a lot of UI real-estate with a (transparent background) Popup which is hit test visible and thus will "block" interaction with elements in the parent window which are located "nearby". So I chose not to do anything about it.

Have a look at it, and give the branch a spin when you have some time. Looking forward to hear what your thoughts are about it.

[Keboo]

One thing that seems odd, is the Elevation property is applied inconsistently to either the toggle button or the popup content depending on the style selected.

A couple options.

1. We have two properties, one for the toggle button, and one for the popup.
2. We adjust the samples so that they are at least consistent.

[nicolaihenriksen]

One thing that seems odd, is the Elevation property is applied inconsistently to either the toggle button or the popup content depending on the style selected.

I agree that this is a bit odd. However, it was like that even before my changes. The ElevationAssist.Elevation only affected the ToggleButton in the MaterialDesignMultiFloatingActionPopupBox derived styles, and had no effect in the default style. With my changes, it now also has an effect in the default style, albeit a "different" effect.

We can of course "fix" the samples so they are at least consistent. However, I don't think that fixes the oddity that the two styles behave differently. I mean both styles contain a "clickable" element which opens a Popup, but only one of the clickable elements is affected by the ElevationAssist.Elevation.

In short, I am not really sure what the correct approach is. ElevationAssist.Elevation is a "general purpose" attached property which perhaps should only affect the "clickable" element and not the Popup? But currently it does nothing in the default style. And if we go this route, we probably need a dedicated dependency property on PopupBox which can control the elevation of the Card in the default style; this is also a bit weird since the DP only applies to certain styles of the control. Alternatively we could introduce a CardAssist.Elevation (inherits) AP which can be applied to the PopupBox (and then inherited by the Card in the Popup; if that even works, since it is not really a child in the visual tree, but an independent "window").

Thoughts?

[Keboo]

I agree that this is a bit odd. However, it was like that even before my changes. The ElevationAssist.Elevation only affected the ToggleButton in the MaterialDesignMultiFloatingActionPopupBox derived styles, and had no effect in the default style. With my changes, it now also has an effect in the default style, albeit a "different" effect.

That is a good point. Since it is not a regression we can ignore it for now.

We can of course "fix" the samples so they are at least consistent. However, I don't think that fixes the oddity that the two styles behave differently. I mean both styles contain a "clickable" element which opens a Popup, but only one of the clickable elements is affected by the ElevationAssist.Elevation.

Given that it is not a regression this is probably not worth it right now. I took a look at the open issues that relate to the popup box and it doesn't appear that this is an issue that is really affecting people.

In short, I am not really sure what the correct approach is. ElevationAssist.Elevation is a "general purpose" attached property which perhaps should only affect the "clickable" element and not the Popup? But currently it does nothing in the default style. And if we go this route, we probably need a dedicated dependency property on PopupBox which can control the elevation of the Card in the default style; this is also a bit weird since the DP only applies to certain styles of the control. Alternatively we could introduce a CardAssist.Elevation (inherits) AP which can be applied to the PopupBox (and then inherited by the Card in the Popup; if that even works, since it is not really a child in the visual tree, but an independent "window").

I am wondering if perhaps splitting the control would make more sense. I do agree that a dedicated DP would make the most sense, but that is awkward if we only have a single control. Splitting the control would then allow for adding the DP only to the one where it makes sense. If we decide to go this route it might be nice to do it with the 5.0.0 release since it would be a breaking change for people (albeit a simple one to address).

Though again, the more I think on it, I think this PR is probably good enough that we can merge it and consider future improvements down the road.

[nicolaihenriksen]

Though again, the more I think on it, I think this PR is probably good enough that we can merge it and consider future improvements down the road.

I agree it probably makes sense to split into 2 different controls going forward.

I just took another quick look at the Popup template. We already have a dedicated DP (PopupUniformCornerRadius) which is only used in the card-based Popup. So perhaps we should just stick with that asymmetric approach and create a dedicated PopupElevation DP which is also only used in the card-based style?

The ElevationAssist.Elevation is after all not created as an Inherits attached property, so it is a bit misleading that you need to use that to affect a "child" element; the fact that it currently does not affect the button that opens the Popup in the default style is a different issue that could be handled separately.

If you agree with the Popup.PopupElevation DP approach, I can make those changes, rebase and clean up the branch to make it ready for review?

What are your thoughts on the dedicated PopupBox demo page? It is different from all the others, and more along the lines of what you've talked about in your streams; being able to manipulate the properties of the "displayed" control. We could leave it out, but then use it for inspiration for a future more generic approach; the current one is really tied to the Popup control.

[Keboo]

What are your thoughts on the dedicated PopupBox demo page? It is different from all the others, and more along the lines of what you've talked about in your streams; being able to manipulate the properties of the "displayed" control. We could leave it out, but then use it for inspiration for a future more generic approach; the current one is really tied to the Popup control.

Yea I sort of like it. I say we keep it.

Go ahead with the Popup.PopupElevation DP approach. I am good with adding it.