-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[macOS] Fix GesturePlatformManager.CalculatePosition
when asked for a relative position
#19371
[macOS] Fix GesturePlatformManager.CalculatePosition
when asked for a relative position
#19371
Conversation
Hey there @MartyIX! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
GesturePlatformManager.CalculatePosition
when asked for a relative position
Btw: The change this PR reverts was introduced here e84098b#diff-ad1a05553c13fe18565124521da4efa63ec857d8d9b6b3e248c1c4ea4497fed5R172 (#12108). edit: Hm, so #12108 fixed #8330 so that is probably the thing that should be tested. |
Can you add a test? Example of one here #19283 |
I can try but it would be great to know if the fix looks ok to you or not. |
919e470
to
ee002f2
Compare
I need to understand a bit more about why the new code doesn't work, and if the fix here will break other cases. The view that it's currently using is a container view that sometimes gets created around the platform view. In some cases that container view will have visible parts. For example, if you add a background to a Label that background will be a view around the label that displays the background . So, I think this PR will cause some scenarios like that to become incorrect. My guess at what is happening here is that the ContainerView is expanding to fill the entire horizontal space and then the image is just occupying the center. I'm curious if that's intentional or if there's a bug with ContainerView and it needs to be sized more correctly. |
@PureWeen I don't know the code in depth to add any meaningful comment right now. However, I believe it's worth taking a step back and trying to first establish what platform is actually affected by a bug. For that purpose I have modified #19329 (comment) and added animations of the behavior on Windows and macOs for the same MAUI application. The fact that the same source code leads to different outcomes on both platforms leads me to believe it's a bug. However, given your previous response, I'm not actually sure if Windows or macOS behaves differently than as expected. Could you take a look at those two animations please? |
Nevermind :-) I see what's going on here now. We just made a bad refactor and your revert makes sense |
src/Controls/src/Core/Platform/GestureManager/GesturePlatformManager.iOS.cs
Outdated
Show resolved
Hide resolved
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
src/Controls/samples/Controls.Sample.UITests/Issues/Issue19329.xaml
Outdated
Show resolved
Hide resolved
d152abd
to
35dd0f7
Compare
35dd0f7
to
c0b1672
Compare
c0b1672
to
ee64161
Compare
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
src/Controls/samples/Controls.Sample.UITests/Issues/Issue19329.xaml.cs
Outdated
Show resolved
Hide resolved
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
OpenQA.Selenium.WebDriverException : An unknown server-side error occurred while processing the command. Original error: Error Domain=io.appium.WebDriverAgentMac Code=1 "Only pointer type 'mouse' is supported. 'touch' is given instead for action with id '0164df41-a79d-42c1-9f78-c4742f81238c'" UserInfo={NSLocalizedDescription=Only pointer type 'mouse' is supported. 'touch' is given instead for action with id '0164df41-a79d-42c1-9f78-c4742f81238c'} I'll work on this one tomorrow we're close |
src/Controls/samples/Controls.Sample.UITests/Issues/Issue19329.xaml
Outdated
Show resolved
Hide resolved
….xaml Co-authored-by: MartyIX <203266+MartyIX@users.noreply.github.com>
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
Co-authored-by: MartyIX <203266+MartyIX@users.noreply.github.com>
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
Description of Change
This PR follows what I wrote here #19327 (comment). I'm not sure if the fix is correct or not. I just figured that creating a PR is an easy way to get some feedback.
Issues Fixed
Fixes #19329
Fixes #19327