Custom popup placement callback

[maxkatz6]

What does the pull request do?

Implements Placement.Custom for Popup and Popup-based controls - Flyout, Tooltip, ContextMenu.

Differences from WPF implementation:

1. It's not possible to support multiple custom placement options like in WPF due to Wayland. But our positioning is generally flexible with different PopupAnchor/PopupGravity/PopupPositionerConstraintAdjustment combinations, that allows implicit adjustments.
2. In WPF, CustomPopupPlacement has only Point position and primary axis params. In Avalonia it's more consistent to the current API.
3. In WPF, CustomPopupPlacementCallback provides Size targetSize argument. I don't see any limitations to provide full rect here though.

New APIs:

+public delegate void CustomPopupPlacementCallback(CustomPopupPlacement parameters);
+public record struct CustomPopupPlacement
+{
+  public Size PopupSize { get; }
+  public Visual Target { get; }
+  public Rect AnchorRectangle { get; set; }
+  public PopupAnchor Anchor { get; set; }
+  public PopupGravity Gravity  { get; set; }
+  public PopupPositionerConstraintAdjustment ConstraintAdjustment { get; set; }
+  public Point Offset { get; set; }
+}

public class Popup
{
+ public static readonly StyledProperty<CustomPopupPlacementCallback?> CustomPopupPlacementCallbackProperty;
+ public CustomPopupPlacementCallback? CustomPopupPlacementCallback { get; set; }
}

public class PopupFlyoutBase
{
+ public static readonly StyledProperty<CustomPopupPlacementCallback?> CustomPopupPlacementCallbackProperty;
+ public CustomPopupPlacementCallback? CustomPopupPlacementCallback { get; set; }
}

public class ContextMenu
{
+ public static readonly StyledProperty<CustomPopupPlacementCallback?> CustomPopupPlacementCallbackProperty;
+ public CustomPopupPlacementCallback? CustomPopupPlacementCallback { get; set; }
}

public class ToolTip
{
+ public static readonly AttachedProperty<CustomPopupPlacementCallback?> CustomPopupPlacementCallbackProperty;
+ public static CustomPopupPlacementCallback? GetCustomPopupPlacementCallback(Control element);
+ public static void SetCustomPopupPlacementCallback(Control element, CustomPopupPlacementCallback? value);
}

Obsolete APIs changes (no binary breaking changes, except NotClientImplementable interface):

[NotClientImplementable]
+[Unstable(ObsoletionMessages.MayBeRemovedInAvalonia12)]
public interface IPopupHost
{
-void ConfigurePosition(Visual target, PlacementMode placement...);
+void ConfigurePosition(PopupPositionRequest positionRequest);
}

+[PrivateApi]
+[Unstable(ObsoletionMessages.MayBeRemovedInAvalonia12)]
+public class PopupPositionRequest
+{
+internal PopupPositionRequest(Visual target, PlacementMode placement)
+internal PopupPositionRequest(Visual target, PlacementMode placement, Point offset, PopupAnchor anchor, PopupGravity gravity, PopupPositionerConstraintAdjustment constraintAdjustment, Rect? anchorRect, CustomPopupPlacementCallback? placementCallback);
+}

public class OverlayPopupHost
{
+[Unstable(ObsoletionMessages.MayBeRemovedInAvalonia12)]
public void ConfigurePosition(Visual target, PlacementMode placement);
}
public class PopupRoot
{
+[Unstable(ObsoletionMessages.MayBeRemovedInAvalonia12)]
public void ConfigurePosition(Visual target, PlacementMode placement);
}

Checklist

• Added unit tests (if possible)? Note: unit testing here is a bit limited, so it's not 100% covered.
• Added XML documentation to any related classes?
• Consider submitting a PR to https://github.com/AvaloniaUI/avalonia-docs with user documentation

Breaking changes

None.

Obsoletions / Deprecations

See API changes.

Fixed issues

Fixes #15233

[robloo]

I think this PR is a good example of an old discussion where we need to start organizing controls by feature/control rather than putting them all together. In this case I think Popup needs its own directory.

• Move PopupPositioning contents into the new Popup directory (get rid of that directory entirely)
• Move all Popup related types into that directory too
• Separate out some of the new types into their own files and place in that directory as well

Small projects in the MVVM world organize by type, I get that. But inevitably with large projects you end up organizing by project/namespace and then by feature. All large-scale projects I've worked on evolve to organize by feature.

[robloo]

I would also break this out into a new file.

Additionally, if it was me, I would make is a class so it could actually be extended if ever needed. This is too complex of a type to lock-down to a struct in my opinion.

[maxkatz6]

I can see PopupPositionRequest being complex (which is why it's a class already), but why CustomPopupPlacement? Essentially, it's just a three enums and a Point property.

[robloo]

In my mind struct is for primitives types (int, size, etc.) this is more complex. The reason to make it a class is to not limit yourself in the future (keep the door open for future extension by developers). There also doesn't appear to be a performance reason for this to be a struct.

So my criteria would be

1. Everything is a class by default in an object-oriented language
2. Is it a primitive type? then make it a struct
3. Is it performance critical? then make it a struct.
4. Is it a special-case?

It fails both of these struct tests and isn't a special case so my convention would be to make it a class. By default types should be a class in an object oriented language like C# -- not a struct. I do think Avalonia went a little too far with struct usage elsewhere.

[billhenn]

I haven't tried the current prototype implementation yet, but my original thought after digging in when writing the issue was that we could improve over WPF's implementation by still allowing default positioning logic to run but also give the callback, if specified, a way to further adjust it.

After doing a quick review of the code changes for this PR, this is a summary of the public API changes:

• PlacementMode.Custom value was added.
• public delegate CustomPopupPlacement CustomPopupPlacementCallback(Size popupSize, Rect targetRect, Point offset); was added.
• struct CustomPopupPlacement was added for a result of the CustomPopupPlacementCallback.
• Popup, PopupFlyoutBase, ContextMenu, and ToolTip added CustomPopupPlacementCallback properties.
• PopupPositionerExtensions.BuildParameters is where the meat of the logic is for positioning everything.

My thoughts on the updates are:

• I don't love how WPF and the changes above pass multiple arguments to CustomPopupPlacementCallback. It would be cleaner if it could pass a single object that had the data in it. Such as a pre-initialized CustomPopupPlacement. This would also allow for possible future options to be passed in without breaking changes.
• Instead of PopupPositionerExtensions.BuildParameters having a separate code path for PlacementMode.Custom, I'd suggest removing the PlacementMode.Custom value altogether.
• In PopupPositionerExtensions.BuildParameters, create a CustomPopupPlacement with all the values from whatever Placement is set instead of setting those values on positionerParameters right away.
• Then always invoke the CustomPopupPlacementCallback if one is specified and pass it the CustomPopupPlacement. This allows the callback to use the pre-initialized values directly if it wants.
• Some properties on CustomPopupPlacement could be read-only if appropriate (like PopupSize and AnchorRectangle) but allow others like Anchor, Gravity, ConstraintAdjustment, and Offset to be get/set.
• CustomPopupPlacement should probably be a class instead of struct.
• An updated delegate could be: public delegate void CustomPopupPlacementCallback(CustomPopupPlacement placement);
• After the possible call to CustomPopupPlacementCallback, PopupPositionerExtensions.BuildParameters would update positionerParameters with the get/set property values from CustomPopupPlacement. This should all be done before the FlowDirection logic kicks in that could possibly mirror things.
• You may want to rename the CustomPopupPlacement type and CustomPopupPlacementCallback delegate to something else if the above API changes are made since the behavior would be different than before.

The main reasons that I suggest the changes above are that it keeps the APIs simple and open for possible future expansion. But more importantly, the callback would receive all the pre-initialized anchor/gravity/constraint/offset values and the callback could leave them as-is or adjust them. Consider a case where we set Placement=PlacementMode.Bottom. If we have callbacks like above, it would already have the anchor/gravity set appropriately and the callback could do something like bump the offset down another few pixels.

It's very open ended with the suggested changes without requiring the developer to do too much either.

I'm happy to discuss further.

[robloo]

But more importantly, the callback would receive all the pre-initialized anchor/gravity/constraint/offset values and the callback could leave them as-is or adjust them.

This is a very good idea!

[maxkatz6]

I might end up wrong here, but positioning API wasn't changed in couple of years, without any need or plans to change it either. Especially when current positioning API is mirroring wayland as a common denominator.

I also not sure about reusing the same class (CustomPopupPlacement) for callback input and output. Since input will always include additional parameters - anchor rect and popup size. While these values are never returned from the callback. Marking them readonly helps a little, but is it really worth it?

PlacementMode.Custom was added not only to mirror WPF API, but also mirror current Avalonia PlacementMode.AnchorAndGravityAPI - which enables other two optional positioning parameters.

I am going to leave this PR as is for now, as I am currently on vacation. More opinions are welcomed here.

[avaloniaui-bot]

You can test this PR using the following package version. 11.2.999-cibuild0048338-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[avaloniaui-bot]

You can test this PR using the following package version. 11.2.999-cibuild0050711-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[avaloniaui-bot]

You can test this PR using the following package version. 11.2.999-cibuild0050717-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[billhenn]

Thank you for updating the PR for testing. The related issue (#15233) was created so that we can display ScreenTips (which inherit ToolTip and use its display mechanisms) for various ribbon controls like buttons underneath our upcoming Ribbon control. In most other cases, we want to keep the default placement mode as Pointer-based, which is the default for ToolTip. ScreenTip has an enhanced template over a standard ToolTip.

What we do in WPF is that any ScreenTip that is displayed, we look at the target element it's opening for and see its location. If it's within the Ribbon, we activate custom placement logic to move the ScreenTip under the Ribbon. Otherwise, we leave the default Pointer placement mode. We can easily do this for all ScreenTips in WPF since all the placement-related options are set on the ScreenTip itself and the placement target control is available (ToolTip.PlacementTarget), so we can adjust the ScreenTip positioning dynamically.

Avalonia has a very different design for ToolTip, where all placement-related properties are attached properties set on the target control. While this design is fine, the biggest problem is that we have no way to get the placement target control in the CustomPopupPlacementCallback. In WPF, since the CustomPopupPlacementCallback was set on the ToolTip itself, we could look at the ToolTip.PlacementTarget to get the control. This is not possible in Avalonia. And without the ability to examine the placement target, we can't put any meaningful logic in the CustomPopupPlacementCallback we make.

This could be somewhat improved if the CustomPopupPlacementCallback would provide the Popup being positioned (which we could look at for the Popup.PlacementTarget), or perhaps the placement target control itself. Here is a prime example where the CustomPopupPlacementCallback needs additional information and if you stick with the ugly WPF design where it passes several fixed parameters, there is no possibility of extensibility in the future without major breaking changes. That's why I think it's better to pass some data transfer object with all the appropriate information as a single parameter to CustomPopupPlacementCallback. This data transfer object could be updated in the future without breaking the callback.

Another thing I noticed is that in my hacked tests of the new logic, CustomPopupPlacementCallback provides no way to alter the target bounds on the placement target. In our case, I need to show the ScreenTip below the Ribbon but at the same X-location of the target control (e.g. button in ribbon). While CustomPopupPlacementCallback lets me set an Offset, what I really want to do is adjust the TargetRect used instead to cover the vertical area of the Ribbon control. That would allow me to use ConstraintAdjustment to properly flip placement around my adjusted TargetRect. If I use Offset only to move where the ideal display is, any adjusted display location per ConstraintAdjustment is completely wrong since it always uses the original TargetRect relative to the Offset.

Here's a screenshot showing that constraint adjustment issue. The left side shows where the ScreenTip for the Home tab should appear. But if there isn't enough vertical space on screen and PopupPositionerConstraintAdjustment.FlipY is set, you get this:

You can see it displays the ScreenTip above the Y location of the vertical Offset subtracted by the original TargetRect height (Home tab height). This is not good. What I'd really like to do is not use Offset at all and change the TargetRect height instead so the constraint adjusted flip moves the ScreenTip above the Ribbon instead.

I'm available for any continued discussion on this. Thanks!

[maxkatz6]

While this design is fine, the biggest problem is that we have no way to get the placement target control in the CustomPopupPlacementCallback

That's a good point I missed. I was under assumption there is a way to get placement target outside of this API already.
I agree it's important to add this information for custom placement flexibility.

This could be somewhat improved if the CustomPopupPlacementCallback would provide the Popup being positioned (which we could look at for the Popup.PlacementTarget)

Popup would be fine, if this callback was only used for popups. But it's also used for tooltips and flyouts. We don't really expose Popup in these controls via other APIs, and we probably shouldn't begin. We can add Visual target instead.

CustomPopupPlacementCallback provides no way to alter the target bounds on the placement target

From my understanding how Wayland works, this shouldn't be an issue to add.

Will push a new commit with some changes. And will update API diff in the first message.

[maxkatz6]

Pushed a new commit.

The biggest change is the callback itself.
Instead of

public CustomPopupPlacement CustomPopupPlacementCallback(Size popupSize, Rect targetRect, Point offset)

it is now

public void CustomPopupPlacementCallback(CustomPopupPlacement placement)

Where CustomPopupPlacement has some get-only parameters (Visual target, Popup size), and the rest are mutable (anchor, offset...). Each mutable field has an actual on a Popup value before the callback.
This way if we add any new parameter in the future, popups won't be broken due to new parameters being uninitialized in the returned object.
I still don't see if we would change this API at any time, but better to be wrong now than figuring it out later.

[avaloniaui-bot]

You can test this PR using the following package version. 11.2.999-cibuild0050947-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[billhenn]

Thank you for the work on this @maxkatz6. The latest updates are working great for our needs, and we were able to accomplish what we wanted to do with them. From our point of view, feel free to merge this PR.

[grokys]

Why use this pattern rather than an attached event? i.e. why not an attached Popup.RequestCustomPlacement event?

[grokys]

Oh wow, ok looks like this comes from WPF. Seems a bit strange not to used an attached event, but looks like there is precedent.