- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.9k
          [Windows] Fix ImageHandler Vertical&Horizontal Options with AspectFit
          #30936
        
          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
Conversation
| Hey there @@morning4coffe-dev! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. | 
| /azp run | 
| Azure Pipelines successfully started running 3 pipeline(s). | 
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.
Pull Request Overview
This PR fixes a bug where images on WinUI did not respect VerticalOptions and HorizontalOptions when using the AspectFit property. The fix ensures that images with AspectFit are displayed at their source image size and can be properly positioned according to layout options.
- Overrides GetDesiredSizemethod inImageHandler.Windows.csto handle AspectFit layout constraints
- Adds helper method to retrieve actual image dimensions from BitmapSource
- Includes regression test to validate the fix
Reviewed Changes
Copilot reviewed 4 out of 15 changed files in this pull request and generated 3 comments.
| File | Description | 
|---|---|
| src/Core/src/Handlers/Image/ImageHandler.Windows.cs | Implements the core fix by overriding GetDesiredSize and adding image size detection | 
| src/Core/src/PublicAPI/net-windows/PublicAPI.Unshipped.txt | Documents the new public API surface for the GetDesiredSize override | 
| src/Controls/tests/TestCases.HostApp/Issues/Issue30403.cs | Creates UI test page demonstrating the image positioning behavior | 
| src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue30403.cs | Implements automated test to verify the fix works correctly | 
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.
The images you've modified are all being modified to closer match the other platforms which is awesome!
Can you rebase this one to net10.0 ? Now that we're getting closer to NET10.0 GA changes like this are better to have their so the cheese isn't moving in net9 at this point.
ced56e1    to
    b797ce8      
    Compare
  
    | /rebase | 
bf7a1db    to
    77ea7a5      
    Compare
  
    | /azp run | 
| Azure Pipelines successfully started running 3 pipeline(s). | 
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, @jsuarezruiz! As discussed during our 1:1, I reuploaded the screenshots | 
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.
With the latest modifications, the fix looks good to me. Just requested to expand the added test to cover edge cases where potentially the changes could have an impact.
| { | ||
| Title = "Issue 30403"; | ||
|  | ||
| Content = new Grid | 
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.
Could modify a little bit this test? Could you include a StackLayout with 2-3 images?
Leave the current one, but could also validate:
- Image with extreme aspect ratio
- Really small image
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.
Created a separate issue to create the tests #31686

Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Description of Change
This pull request addresses a bug where images on WinUI did not respect
VerticalOptionsandHorizontalOptionswhen using theAspectFitproperty. The changes include adding a regression test, updating theImageHandlerto handle layout constraints properly.This change in behavior streamlines it with other targets, where images with those properties are displayed at the size of the source image. It can then be resized to a smaller size after the source size is met with the size of the panel/window.
Current behavior
Issues Fixed
Fixes #30403