-
Notifications
You must be signed in to change notification settings - Fork 381
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 Popup size on Android and iOS #1361
Conversation
When not specifying the size, does Popup respects the LayoutOptions (Center, Fill, Start, End)? |
@pictos , I'll fix it again because the PR I created doesn't respect the LayoutOptions when the Popup size is unspecified. |
Below are additional validation results.
Below are the verification results on Android. [When VerticalOptions is Start]
[When VerticalOptions is Center]
[When VerticalOptions is End]
[When VerticalOptions is Fill]
Below are the verification results on iOS. [When VerticalOptions is Start]
[When VerticalOptions is Center]
[When VerticalOptions is End]
[When VerticalOptions is Fill]
You can see that the VerticalOptions are respected because the height is unspecified. |
@pictos , Windows test results are posted below. |
Following the comments below, I have made similar fixes to this PR as well. |
src/CommunityToolkit.Maui.Core/Views/Popup/PopupExtensions.android.cs
Outdated
Show resolved
Hide resolved
@pictos , @VladislavAntonyuk |
I'm sorry for the change after approval. Below are the corrections. [src\CommunityToolkit.Maui.Core\Handlers\Popup\PopupHandler.macios.cs]
This fix is taken from PR #1369. [src\CommunityToolkit.Maui.Core\Views\Popup\PopupExtensions.macios.cs]
The above is a correction of the base position of the second and subsequent Popups. The verification results before and after modification are shown below. The 1st Popup has HorizontalOptions="End" and VerticalOptions="End" set.
Before the modification, when the third Popup was closed, the second Popup was also closed, but after the modification, only the third Popup was closed. Before the correction, the next Popup was displayed starting from the position of the previous Popup, but after the correction, the Popup is displayed starting from the View that is the display source of the Popup. With the overlay fix in PR #1369, the second and subsequent Popups will no longer be transparent. |
@brminnick , Currently, the following warning is output and the iOS side does not function as before,
I would like to include the fix for Issue #1416 in this PR. Below are the changes. [src\CommunityToolkit.Maui.Core\Views\Popup\MauiPopup.macios.cs]
If I can find a better solution, I would like to create a new PR then. Thank you. |
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 @cat0363! FYI - I added the sample you created in your reproduction to the official sample app. Great work!
This is really really close! I confirmed that everything works as expected on iOS + Android.
But on macOS, I'm seeing strange behavior in the CustomSizeAndPositionPopupPage
:
Seems like the issue in calculating the offset. Popup is moved half of its size to the right |
@brminnick , Thank you for verifying with MacCatalyst. systemMinimumLayoutMargins may be related, but I'm not sure ... |
I will reconsider including the rounding process when the Popup size exceeds the screen size. |
It seems that MacCatalyst does not allow Popup to completely cover the caller's screen as it does on iOS. |
In the case of MacCatalyst, there is a margin that is always taken on the left side, so modify it to take that |
For MacCatalyst, I modified it so that a margin of 18 is taken around the Popup by default.
You may need to look into the MacCatalyst specifications to find out why a minimum margin of 18 is required. |
I have uploaded the fix for Issue #1423.
Below is a video of the verification results. Android.Emulator.-.pixel_2_-_api_30_5554.2023-09-27.17-02-54.mp4Added squares to the four corners of Popup in the reproduction code of Issue #1423. I tried rotating the screen and verifying it.
|
Below is a verification video using MacCatalyst. [Width = 100, Height = 100] 2023-09-29.17.11.02.mov[Width = 1000, Height = 100] 2023-09-29.17.22.18.mov[Width = 100, Height = 1000] 2023-09-29.17.23.41.mov[Width = 1000, Height = 1000] 2023-09-29.17.20.19.mov |
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.
Awesome!! Thank you so much Kenji for your always amazing work 💯
This PR corrects the Popup size to be calculated correctly when the Popup size is not explicitly specified.
Description of Change
The process for getting the window size has been made into a method.
[src\CommunityToolkit.Maui.Core\Views\Popup\PopupExtensions.android.cs]
Then I modified the CalculateSizes method as follows:
[src\CommunityToolkit.Maui.Core\Views\Popup\PopupExtensions.android.cs]
Changed to recalculate the size within the size of the Window when the Popup's Content size is not specified.
And, Changed to limit Popup size to Window size in SetSize method.
[src\CommunityToolkit.Maui.Core\Views\Popup\PopupExtensions.android.cs]
With these changes, the Popup's size is now correctly calculated when no explicit size is specified for the Popup.
Linked Issues
PR Checklist
approved
(bug) orChampioned
(feature/proposal)main
at time of PRAdditional information
Below are the verification results for Issue #1353.
You can see that the label placed inside the Popup wraps as intended.
Even if you specify an explicit size for the Popups Size when the Popups content is only a Label, the label
text was not wrapped before the fix, but after the fix, the label text is wrapped.
I layout the Popup as shown below and verified it.
Below are the verification results.
[When VerticalOptions is Start]
I changed the HorizontalOptions to:
[When VerticalOptions is Center]
I changed the HorizontalOptions to:
[When VerticalOptions is End]
I changed the HorizontalOptions to:
[When VerticalOptions is Fill]
I changed the HorizontalOptions to:
Even if you explicitly specify the size of the Popup, you can see that the label inside the Popup is wrapped.