-
Notifications
You must be signed in to change notification settings - Fork 399
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix an issue with Popup size and position on Android #1683
Conversation
I have tested this with our samples and they all appear to behave as expected. Do we need to add more samples to cover the issues this is solving? I am happy to get involved and add those. |
@bijington , I agree with your opinion. Having sample code makes it easier to verify operation. |
I just tested it using |
@cat0363 I've been testing it with all the previews for the last few weeks -- the latest I tested it with is As I mentioned in my previous comments, the size and positioning issues seem to have been fixed now but there's another issue -- and I think related one -- still remains -- see it at #1621 (comment) Originally, I thought label controls didn't display text when placed inside a popup but if you see the screen capture I did, you'll see that once I scroll up and down, text starts to display. I'm not sure if this issue is related to this one here but I think it may be. |
@imsam67 , Thank you for your reply. I don't know if this PR is included in the preview you listed, but can I obtain the preview package for this PR by following the steps below? https://github.com/CommunityToolkit/Maui/wiki/Preview-Packages |
Hi @cat0363 I have tried to follow the steps to get the preview package, when I click on the details link shown below, to go to the Azure Pipeline where the PR was built I get: "401 - Uh-oh, you do not have access" If you can provide me with a .nupkg file I will test and add my results here, maybe attach the .nupkg file to this thread would be easiest? |
@cat0363 The issue I'm experiencing is indeed the same issue as reported in #1532 I also went through the process of installing the package generated for #1683 and it certainly fixes the issue! How can we get your fix into, at least, the latest preview package? I really appreciate your tackling these issues that seem somewhat unpopular. Thank you for your efforts :-) |
when i removed this code: Label in Popup.Border(with padding).Grid can finally handle the center HorizontalTextAlignment . Maui/src/CommunityToolkit.Maui.Core/Views/Popup/PopupExtensions.android.cs Lines 253 to 262 in 3e45f88
|
Just following up - since this has a breaking change to the API, and we're supper close to merging TouchBehavior, I'll be merging this PR after we merge TouchBehavior and then we'll publish v8.0.0 of |
happy to hear that, it's been so long since the last release.
|
Thanks for the update!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Kenji!!
Thanks Kenji for fixing the issue and thanks Brandon for getting this all wrapped up! |
Just installed |
@imsam67 @brminnick Please could you let me know how I can get hold of 99.0.0-preview1654 ? |
@Kas-code The information is under Wiki > Usage section: https://github.com/CommunityToolkit/Maui/wiki/Preview-Packages Just add the URL for the "Latest" feed to NuGet sources in Visual Studio > Tools > Options > NuGet Package Manager > Package Sources. BTW, they have a new version of preview now. HTH. |
After updating to the latest preview, I can also confirm that the issue is fixed! Thanks @cat0363 , @brminnick , @bijington and everyone else who contributed! |
This PR will solve the following problems.
Description of Change
The solution to the first problem is shown below.
I made a mistake in PR #1520 by calling the Measure method on all child elements of the Popup's Content if you do not explicitly specify the Popup's size. There was also a mistake in the way PR #1456 addressed the label wrapping issue.
Calling the Measure method carelessly can produce unintended results in Android.
I modified it to call the Measure method only if the Popup size is not explicitly specified.
I also modified it to not call the Measure method of the Popup's Content if there is no change in the Popup's size or screen orientation.
If the Popup size is not explicitly specified, LayoutOptions is other than Fill, and the result of calling the Measure method does not exceed the screen size, the argument of the SetLayout method remains WrapContent, so in that case, MeasuredWidth, MeasuredHeight should be set. It will be retained as the previous value.
Therefore, I have made changes to retain the previous values of Popup and screen size.
[src\CommunityToolkit.Maui.Core\Handlers\Popup\PopUpHandler.android.cs]
[src\CommunityToolkit.Maui.Core\Views\Popup\PopupExtensions.android.cs]
The solution to the second problem is shown below.
In the source code before modification, when Anchor is specified, the Popup is displayed near the center of Anchor, but it is not exactly in the center.
This is because the height of the StatusBar and NavigationBar is not taken into account when calculating the popup display position.
Therefore, we have modified it to take into account the height of the StatusBar and NavigationBar when calculating the popup display position.
This time, I have newly added GetStatusBarHeight and GetNavigationBarHeight methods.
Additionally, I changed the definition location of the GetWindowSize method.
If the Popup size is explicitly specified, the Popup content size is specified, or the Popup size is obtained by the Measure method, I used these to calculate the Popup display position.
[src\CommunityToolkit.Maui.Core\Views\Popup\PopupExtensions.android.cs]
Linked Issues
PR Checklist
approved
(bug) orChampioned
(feature/proposal)main
at time of PRAdditional information
For verification, I used the following Nightly Build of .NET MAUI.
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.2024-02-08.13-37-08.mp4
Below is the execution result after rotating the device.
[No.9] (Testing VerticalStackLayout.)
[No.10] (Testing HorizontalStackLayout.)
[No.11] (Testing Grid.)
[No.12] (Issue #1603)
Android.Emulator.-.pixel_2_-_api_30_5554.2024-02-08.10-58-14.mp4
You can see that the Popup is displayed as intended in both layouts.
Below are the verification results for each issue.
[Issue #1532]
Android.Emulator.-.pixel_2_-_api_30_5554.2024-02-08.11-30-03.mp4
[Issue #1575]
Android.Emulator.-.pixel_2_-_api_30_5554.2024-02-08.12-28-41.mp4
[Issue #1489]
Android.Emulator.-.pixel_2_-_api_30_5554.2024-02-08.12-45-24.mp4
[Issue #1592]
Android.Emulator.-.pixel_2_-_api_30_5554.2024-02-08.12-54-45.mp4
You can see that each issue has been resolved by this PR.
Finally, the verification results when specifying Anchor for Popup are shown below.
For verification, we used a modified sample program.
I used Border because it was difficult to see the exact center of the anchor with the cross-shaped text on the label.
[Anchor Layout]
[Popup Layout]
You can see that a red circle Popup is displayed in the center of the blue circle anchor.
Android.Emulator.-.pixel_2_-_api_30_5554.2024-02-08.13-11-11.mp4
Below are the verification results before and after screen rotation.
When refactoring, it is best to do so with caution. The impact of the changes is huge.
I'm sorry, but I can't spend any more time on this issue.
Even with this PR, Issue #1585 is not resolved. There is a high possibility that this is a problem on the MAUI side.