Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Windows] Fix FontImageSource resize behavior #21212

Merged
merged 5 commits into from
Mar 19, 2024
Merged

[Windows] Fix FontImageSource resize behavior #21212

merged 5 commits into from
Mar 19, 2024

Conversation

Foda
Copy link
Member

@Foda Foda commented Mar 14, 2024 •
edited by PureWeen
Loading

Description of Change

Images should always set their size to the size of FontImageSource/CanvasImageSource as this is established behavior for Maui/Xamarin.

Image + Button Resize Rules

  • Images should always set their size to the size of FontImageSource/CanvasImageSource regardless of if they're in a button or not
  • If you don't set the size of the button then the button should grow to fit the image/content size
  • If you set the size of the button the image should stretch to fit the available space, unless it's a FontImageSource/CanvasImageSource
  • If you set the size of the button and the image then no resizing should happen
  • If you set the size of the image, it should not resize

Issues Fixed

Fixes #21202
Fixes #20648

@Foda Foda added platform/windows 🪟 area-controls-button Button, ImageButton i/regression This issue described a confirmed regression on a currently supported version labels Mar 14, 2024
@Foda Foda requested a review from a team as a code owner March 14, 2024 18:12
mattleibow
mattleibow reviewed Mar 15, 2024
View reviewed changes
src/Core/src/Platform/Windows/ButtonExtensions.cs Outdated
Comment on lines 169 to 173
if (nativeImage.Source is CanvasImageSource canvas)
{
nativeImage.Width = canvas.Size.Width;
nativeImage.Height = canvas.Size.Height;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like I reviewed a PR weeks ago that removed all the measuring code here - from normal images and things.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit: 7dff6eb#diff-e34feff8430d43e27f52abff099ba347b41628f6bde6a68e12f8b618722c59c9

Should this rather have been implemented differently instead of just being removed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you did 🥲. I misunderstood the behavior of how FontImageSource scales-to-fit the available space, which differs from how images in buttons scale-to-fit.

src/Controls/samples/Controls.Sample.UITests/Issues/Issue18242.xaml
@@ -3,36 +3,82 @@
xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
x:Class="Maui.Controls.Sample.Issues.Issue18242"
Title="Issue 18242">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should be a new page? I just finished up the Android fixes and if there are 2 columns this page will be too narrow and cramped for a good test on android/ios

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's probably a good idea.

src/Controls/samples/Controls.Sample.UITests/Controls.Sample.UITests.csproj Outdated
@@ -48,6 +48,10 @@
<MauiSplashScreen Include="Resources\Splash\splash.svg" Color="#FFFFFF" BaseSize="168,208" />
</ItemGroup>

<ItemGroup>
<EmbeddedResource Include="Resources\Fonts\OpenSans-Regular.ttf" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should just be <MauiFont> ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-packaged app fonts won't work if they're set to MauiFont

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be fixed now - we merged the win2d updates.

But it is fine if we want this to run on older branches.

@Foda Foda mentioned this pull request Mar 15, 2024
Merged
@PureWeen PureWeen requested a review from mattleibow March 15, 2024 20:09
mattleibow
mattleibow previously approved these changes Mar 15, 2024
View reviewed changes
@PureWeen
Copy link
Member

/rebase

@github-actions github-actions bot force-pushed the foda/FontImageButtonStretch branch from 66bbbe4 to a44ad41 Compare March 19, 2024 02:05
@PureWeen
Copy link
Member

UI Tests all passed.
Failing Device tests are unrelated

@PureWeen PureWeen merged commit d03a2c6 into main Mar 19, 2024
39 of 47 checks passed
@PureWeen PureWeen deleted the foda/FontImageButtonStretch branch March 19, 2024 03:57
@Foda Foda mentioned this pull request Mar 26, 2024
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@PureWeen PureWeen PureWeen approved these changes

@mattleibow mattleibow mattleibow left review comments

@StephaneDelcroix StephaneDelcroix Awaiting requested review from StephaneDelcroix StephaneDelcroix is a code owner automatically assigned from dotnet/dotnet-maui-reviewers

@jsuarezruiz jsuarezruiz Awaiting requested review from jsuarezruiz jsuarezruiz is a code owner automatically assigned from dotnet/dotnet-maui-reviewers

Assignees
No one assigned
Labels
area-controls-button Button, ImageButton fixed-in-8.0.20 fixed-in-9.0.0-preview.3.10457 i/regression This issue described a confirmed regression on a currently supported version platform/windows 🪟
Projects
None yet
Milestone
No milestone
4 participants
@Foda @PureWeen @mattleibow @samhouts