Change in Popup Size measurement method#1520
Change in Popup Size measurement method#1520cat0363 wants to merge 13 commits intoCommunityToolkit:mainfrom
Conversation
|
The code for verification is below. |
|
Modify some conditions. It is currently being verified. |
|
Adding a condition when the target's Parent is ReorderItemView solves the CollectionView display problem. Android.Emulator.-.pixel_2_-_api_30_5554.2023-11-17.10-08-18.mp4Issue #1532 should be resolved with this fix. |
|
Hey @cat0363, thanks for that... Basically you need to have access to the Maui world, but the code that you put inside the shared layer is just for android, and that doesn't make too much sense, using the shared to control to call the handler/platform control. What you can is to create a |
|
@pictos , Thank you for the advice. I had made a big mistake. I haven't fully investigated the details of how the Measure method works on other platforms. Only Android has a MeasureSpecMode specification, and if you don't specify anything and just pass the width and height to the Measure method, it will behave as MeasureSpecMode.Unspecified, so this is the cause of the problem in Android. At this point, we do not know if there are any issues with platforms other than Android. |
|
In Windows, there were cases where there was a problem when the Popup size was not specified. |
|
I found a solution to the problem where Popup is displayed at an unintended size when you do not specify an explicit size on Windows. |
|
If the Popup size is not specified and the Label Text is dynamically changed, it will not be redrawn on iOS. iPhone.14.iOS.16.4.2023-11-24.12-09-47.mp4I can't think of any other way because I can't detect changes in the size of child elements on the iOS side. |
cat0363
changed the title
|
Addressed popup size issues on Android, iOS, and Windows. |
|
I need to consider the StrokeThickness and Padding of the Border control, so we will reconsider it. |
| namespace CommunityToolkit.Maui.Views; | ||
|
|
||
| public partial class Popup | ||
| public partial class Popup : Element |
There was a problem hiding this comment.
why this's using the shared layer instead of the Handler and the MapSize?
There was a problem hiding this comment.
@pictos , Since it was unclear how to detect the size change event of the child element of Popup's Content, I implemented it in the shared layer.
If it is possible to detect changes in the size of child elements as the text changes, there is no need to implement it in the shared layer.
Is there any better way?
At least if I didn't implement it like this, No. 4 wouldn't work.
In the case where the Popup's size is unspecified and it resizes according to the dynamic resizing of Content's child elements.
There was a problem hiding this comment.
@pictos Just curious - what's the downside to making Popup inherit from Element.
Looking at the .NET MAUI repo, every control that appears on the screen inherits from VisualElement (e.g. Button, Label, etc, all inherit from VisualElement).
For Popup, I'd say it may make even more sense to have it inherit from VisualElement to better align with how .NET MAUI creates controls.
There was a problem hiding this comment.
@brminnick answering your questions...
@pictos Just curious - what's the downside to making Popup inherit from Element.
All the downside possibles, hehe... The .NET MAUI team gave more love to VisualElement instead of Element, the good part from the community is that Shane made a lot of improvements on Element because of poups, and now everyone in the community will have a happy place where they need to implement controls that inherit from Element.
For Popup, I'd say it may make even more sense to have it inherit from VisualElement to better align with how .NET MAUI creates controls.
Would be great if we could do it, but we are blocked on that because of the current .NET MAUI restrictions on what can be a VisualElement. So, every control that inherits from VisualElement its native part will inherit from ViewHandler, Basically just controls that are inherited from the platform View can be a VisualElement. Our popup implementation follows what each platform defines what's a popup, For android it's a Dialog and for iOS a UIViewController is used, and none of those inherit from platform View, with that, we can't make the PopupHandler a ViewHandler and because of that the Popup can't be a VisualElement.
But again, @PureWeen made a lot of improvements to make Element a first-class citizen on .net Maui custom controls. We basically need to make usage of the new APIs, like this one, as you can see on description, Shane used the Popup as showcase
There was a problem hiding this comment.
Got it! Thanks for the details!!
There was a problem hiding this comment.
Can't we assign it to the Content.SizeChanged and call the MapSize from there? MY plan here is to make this control aligned with the .NET MAUI way to do things, that way if this control is promoted to be in the Maui framework will be easier to port it.
There was a problem hiding this comment.
@pictos , Thank you for suggesting the amendment.
Fixed to call SetSize method in Content.SizeChanged.
| public static void MapSize(PopupHandler handler, IPopup view) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(handler.Container); | ||
|
|
There was a problem hiding this comment.
do we need this method after removing the code?
There was a problem hiding this comment.
The MapSize method is not needed as a similar implementation is done within the OnPropertyChanged method.
[src\CommunityToolkit.Maui\HandlerImplementation\Popup\Popup.android.cs]
void OnPropertyChanged(object? sender, PropertyChangedEventArgs e)
{
if (e.PropertyName == nameof(Size))
{
if (Handler?.MauiContext is IMauiContext mauiContext)
{
var platformPopup = this.ToHandler(mauiContext);
if (platformPopup.PlatformView is Dialog dialog &&
platformPopup.VirtualView is IPopup pPopup &&
pPopup.Content?.ToPlatform(mauiContext) is AView container)
{
SetSize(dialog, this, pPopup, container);
CommunityToolkit.Maui.Core.Views.PopupExtensions.SetAnchor(dialog, pPopup);
}
}
}
}
There was a problem hiding this comment.
However, since the MapSize method was being called when the VerticalOptions and HorizontalOptions properties were changed, it is necessary to correct it as shown below.
[src\CommunityToolkit.Maui\HandlerImplementation\Popup\Popup.android.cs]
void OnPropertyChanged(object? sender, PropertyChangedEventArgs e)
{
if (e.PropertyName == nameof(Size) ||
e.PropertyName == nameof(HorizontalOptions) ||
e.PropertyName == nameof(VerticalOptions))
{
if (Handler?.MauiContext is IMauiContext mauiContext)
{
var platformPopup = this.ToHandler(mauiContext);
if (platformPopup.PlatformView is Dialog dialog &&
platformPopup.VirtualView is IPopup pPopup &&
pPopup.Content?.ToPlatform(mauiContext) is AView container)
{
SetSize(dialog, this, pPopup, container);
CommunityToolkit.Maui.Core.Views.PopupExtensions.SetAnchor(dialog, pPopup);
}
}
}
}
[src\CommunityToolkit.Maui\HandlerImplementation\Popup\Popup.windows.cs]
void OnPropertyChanged(object? sender, PropertyChangedEventArgs e)
{
if (e.PropertyName == nameof(Size) ||
e.PropertyName == nameof(HorizontalOptions) ||
e.PropertyName == nameof(VerticalOptions))
{
if (Handler?.MauiContext is IMauiContext mauiContext)
{
var platformPopup = this.ToHandler(mauiContext);
if (platformPopup.PlatformView is Microsoft.UI.Xaml.Controls.Primitives.Popup dialog &&
platformPopup.VirtualView is IPopup pPopup &&
pPopup.Content?.ToPlatform(mauiContext) is Microsoft.UI.Xaml.FrameworkElement container)
{
SetSize(dialog, this, pPopup, mauiContext, container);
CommunityToolkit.Maui.Core.Views.PopupExtensions.SetLayout(dialog, pPopup, mauiContext);
}
}
}
}
[src\CommunityToolkit.Maui.Core\Handlers\Popup\PopupHandler.shared.cs]
public static IPropertyMapper<IPopup, PopupHandler> PopUpMapper = new PropertyMapper<IPopup, PopupHandler>(ElementMapper)
{
[nameof(IPopup.Anchor)] = MapAnchor,
[nameof(IPopup.Color)] = MapColor,
#if IOS || MACCATALYST || TIZEN
[nameof(IPopup.Size)] = MapSize,
[nameof(IPopup.VerticalOptions)] = MapSize,
[nameof(IPopup.HorizontalOptions)] = MapSize,
#endif
[nameof(IPopup.CanBeDismissedByTappingOutsideOfPopup)] = MapCanBeDismissedByTappingOutsideOfPopup
};
Therefore, the MapSize method is not required on Windows and Android.
The following must be removed.
[src\CommunityToolkit.Maui.Core\Handlers\Popup\PopUpHandler.android.cs]
public static void MapSize(PopupHandler handler, IPopup view)
{
ArgumentNullException.ThrowIfNull(handler.Container);
}
[src\CommunityToolkit.Maui.Core\Handlers\Popup\PopUpHandler.windows.cs]
public static void MapSize(PopupHandler handler, IPopup view)
{
}
I will update the fix after verifying operation.
| #if ANDROID || WINDOWS | ||
| AddHandlerChanged(); | ||
| AddPropertyChanged(); | ||
| #elif IOS |
There was a problem hiding this comment.
@VladislavAntonyuk , As you pointed out, MacCatalyst is required.
|
Modify the implementation on the Windows side to reduce the number of changes. |
ghost
added
stale
|
As Shane mentions in Issue 1664, this PR's approach is wrong because calling the Measure method outside of the layout cycle produces unintended results on Android. Therefore, this PR should not be applied. I am very sorry that I have come to this conclusion after receiving various points. At the moment, I can't think of a way to get the exact size of a Popup without calling the Measure method outside of the layout cycle. |
|
Thanks for following up @cat0363!
Bummer! No worries. I'll close this PR. |
|
@brminnick , Thank you for your reply. |
|
I'm currently testing it, but I've found a new solution. Regarding No.7, this solution does not solve the problem. |
|
@cat0363 Just a note of support - thank you for continuing to work on this. |
|
@Kas-code , In addition to the Popup size issue, I am currently verifying a solution to the display position issue after screen rotation when specifying Anchor for the Popup. |
|
Now that I have completed the verification, I will create a PR starting tomorrow. |
|
I just uploaded a PR. Thank you for allowing me to take on the challenge again. |
5 participants
This PR resolves an issue where the TextAlignment of a button label placed in a StackLayout is displayed in an unintended position.
It also resolves the issue where the TextAlignment of buttons placed in StackLayout and Grid appears in unintended positions when the device is rotated.
Description of Change
The cause of this problem is that the platform's Measure method is called without considering Maui's LayoutOptions to determine the Popup size. When you call the platform's Measure method, the size of controls placed in StackLayout, VerticalStackLayout, HorizontalStackLayout, and Grid is not measured to the intended size.
In order to call the Measure method taking LayoutOptions into account, the SetSize method in the following locations has been abolished.
[src\CommunityToolkit.Maui.Core\Views\Popup\PopupExtensions.android.cs]
Also, I have removed calls to the SetSize and SetAnor methods that are called in the following places:
[src\CommunityToolkit.Maui.Core\Handlers\Popup\PopUpHandler.android.cs]
Instead, I changed it to call the SetSize SetAnchor method at the location below.
[src\CommunityToolkit.Maui\Views\Popup\Popup.shared.cs]
[src\CommunityToolkit.Maui\Views\Popup\PopupExtensions.android.cs]
I defined a new SetSize method in the location below.
[src\CommunityToolkit.Maui\Views\Popup\PopupExtensions.android.cs]
By defining the SetSize method in the above location, Maui's LayoutOptions can be referenced and exact size calculations can be performed. See implementation for details.
I changed the way the Measure method is called for each of StackLayout, VerticalStackLayout, HorizontalStackLayout, and Grid.
If Padding is set for Popup's Content, Button's TextAlignment will be misaligned, so we take Padding into consideration.
Linked Issues
PR Checklist
approved(bug) orChampioned(feature/proposal)mainat time of PRAdditional information
Below are the verification results.
[No.1]
[No.2]
[No.3]
[No.4] (Test to dynamically change the label in Popup's Content)
[No.5] (Test to set a long string to the label text in Popup's Content)
[No.6] Test for rounding popup corners
[No.7]
The result is similar to the .NET MAUI side. This behavior cannot be changed. An explicit size must be specified.
[No.8] (This is a test for Issue #1489.)
Android.Emulator.-.pixel_2_-_api_30_5554.2023-11-15.17-14-07.mp4
Below is the execution result after rotating the device.
[No.9] (Testing VerticalStackLayout.)
[No.10] (Testing HorizontalStackLayout.)
[No.11] (Testing Grid.)
[No.12]
You can see that the Popup is displayed as intended in both layouts.
Note:
This PR takes padding into consideration, so please check if the padding in CommunityToolkit's PopupLayout is what you intended.
Due to insufficient consideration of PR #1456, Popups were not displayed as per the layout on the Android side for a long time.
This is mainly a Popup sample.
This PR makes major changes to how controls within Popups are measured. If you do not accept this change, you may want to reconsider reverting the code in PR #1456. Display issues on Android are caused by insufficient consideration of PR #1456.