Skip to content

[iOS] Fix downscaled images not occupying correct space#17120

Merged
mattleibow merged 15 commits intonet8.0from
dev/jd/imageios
Oct 21, 2023
Merged

[iOS] Fix downscaled images not occupying correct space#17120
mattleibow merged 15 commits intonet8.0from
dev/jd/imageios

Conversation

@jknaudt21
Copy link
Copy Markdown
Contributor

@jknaudt21 jknaudt21 commented Aug 31, 2023

Problem

Large iOS images would not honor their layout's specified height/width nor would they display in a way that's consistent with Android and Windows.

The problem is caused by a limitation in iOS. When calling SizeThatFits for an ImageView, the function will always return the image's dimensions regardless of what the user constrains. This is by design from Apple

This also affected image buttons.

Solution

We create our own extension method that calculates "SizeThatFits" in a way that respects the View's given constraints. When the Aspect is equal to AspectFit, we calculate the how much we need to scale down the image based on the smallest dimension that fit the image. Otherwise, we just return the original constraints.

Keep in mind that this custom implementation of "SizeThatFits" is only used for calculating space needed by the view. It does not calculate the size of the image that's inside the view; we leave that to the native ContentMode (aka the Aspect settings).

Issue Before & After Screenshots (Catalyst & iOS)

I didn't include screenshots for every issue that was reported regarding this topic because there were too many as this was a pretty significant issue.

16120 (click me!)
    <ScrollView>
        <VerticalStackLayout>
            <Image Source="tree_big.jpg" x:Name="tree" Aspect="AspectFit" BackgroundColor="AliceBlue"/>
        </VerticalStackLayout>
    </ScrollView>
Before After
image image
image image
14139 (click me!)
    <ScrollView>
        <VerticalStackLayout Spacing="25" HorizontalOptions="FillAndExpand" VerticalOptions="FillAndExpand">
            <Grid>
                <Image
                    Source="image1.png"
                    Aspect="AspectFit"
                    HorizontalOptions="FillAndExpand"
                    VerticalOptions="Start"
                    BackgroundColor="Yellow"/>
                <Border HorizontalOptions="FillAndExpand" VerticalOptions="FillAndExpand" Stroke="Red" StrokeThickness="4" StrokeShape="RoundRectangle 10,10,10,10" BackgroundColor="Transparent" Margin="0" Padding="0" />
            </Grid>
        </VerticalStackLayout>
    </ScrollView>
Before After
image image
image image
16921 (click me!)

(See the sample given in #16921)

iOS and Mac ran the same but I'm only including the Mac recording. Note that the small blue "button" shows how the large image should actually behave. This is because the "button" is actually a non-downscaled image.
Before

Screen.Recording.2023-08-31.at.5.55.46.PM.mov

After

Screen.Recording.2023-08-31.at.5.50.20.PM.mov
14926 (click me!)
        <Grid ColumnDefinitions="Auto,*,Auto" Margin="10">
            <Button
                VerticalOptions="Center"
                Margin="15,0,0,0"
                Text="One" />
            <Image
                BackgroundColor="Green"
                Grid.Column="1"
                HeightRequest="76"
                HorizontalOptions="Center"
                Source="logo_dark.png" />
            <Button
                Margin="0,0,15,0"
                VerticalOptions="Center"
                Grid.Column="2"
                Text="Two" />
        </Grid>

Note The green box is supposed to "overflow" to fulfill the grid's * width requirement. It is not meant to overlap with other buttons, however.

Before After
image image
image image
13893 (click me!)

See customer sample in linked issue

There were many samples here, but they boiled down to similar problems, shown in the view below.

Before After
image image
image image

Issues Fixed

Fixes #16120
Fixes #14139
Fixes #16921
Fixes #14606
Fixes #13893
Fixes #14926
Fixes #10481
Fixes #12359

Work Remaining

  • Write tests
  • Include screenshots
  • Solve specific case for ImageButtons
  • Fix tests

@samhouts samhouts added this to the .NET 8 GA milestone Aug 31, 2023
@jknaudt21 jknaudt21 marked this pull request as ready for review September 2, 2023 00:38
@jknaudt21 jknaudt21 requested a review from a team as a code owner September 2, 2023 00:38
@jknaudt21 jknaudt21 added platform/macos macOS / Mac Catalyst platform/ios area-image Image loading, sources, caching labels Sep 5, 2023
@jknaudt21
Copy link
Copy Markdown
Contributor Author

Images.zip
To make testing this PR easier for reviewers I'm uploading the images folder

@jknaudt21 jknaudt21 requested a review from a team as a code owner October 18, 2023 23:18
@jknaudt21 jknaudt21 changed the base branch from main to net8.0 October 18, 2023 23:18
Copy link
Copy Markdown
Member

@tj-devel709 tj-devel709 left a comment

Choose a reason for hiding this comment

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

Sorry! I tested out all the attached scenarios and a few alterations locally previously and they all looked good to me!

Copy link
Copy Markdown
Member

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

Is it possible to add some image comparison tests? The current tests look like they just check for "is this color somewhere" and that will not catch much.

@Eilon Eilon removed the request for review from a team October 20, 2023 18:29
@mattleibow
Copy link
Copy Markdown
Member

For tests:

public void Issue15330Test()
{
this.IgnoreIfPlatforms(new TestDevice[] { TestDevice.iOS },
"Currently fails on iOS; see https://github.com/dotnet/maui/issues/17125");
App.WaitForElement("WaitForStubControl");
VerifyScreenshot();
}

public async Task FormattedStringSpanTextHasCorrectLayoutWhenAligned(TextAlignment alignment)
{
var formattedLabel = new Label
{
WidthRequest = 200,
HeightRequest = 50,
HorizontalTextAlignment = alignment,
FormattedText = new FormattedString
{
Spans =
{
new Span { Text = "short" },
new Span { Text = " long second string"}
}
},
};
var normalLabel = new Label
{
WidthRequest = 200,
HeightRequest = 50,
HorizontalTextAlignment = alignment,
Text = "short long second string"
};
await InvokeOnMainThreadAsync(async () =>
{
var formattedHandler = CreateHandler<LabelHandler>(formattedLabel);
var formattedBitmap = await formattedHandler.PlatformView.ToBitmap(MauiContext);
var normalHandler = CreateHandler<LabelHandler>(normalLabel);
var normalBitmap = await normalHandler.PlatformView.ToBitmap(MauiContext);
await normalBitmap.AssertEqualAsync(formattedBitmap);
});
}

Copy link
Copy Markdown
Member

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

I missed the image size tests, so this is OK for now

@mattleibow mattleibow merged commit b30d812 into net8.0 Oct 21, 2023
@mattleibow mattleibow deleted the dev/jd/imageios branch October 21, 2023 00:16
@mattleibow
Copy link
Copy Markdown
Member

/backport to release/8.0.1xx-rc2.1

@github-actions
Copy link
Copy Markdown
Contributor

Started backporting to release/8.0.1xx-rc2.1: https://github.com/dotnet/maui/actions/runs/6593944764

@github-actions github-actions bot locked and limited conversation to collaborators Dec 6, 2023
@Eilon Eilon added area-controls-image Image control and removed area-image Image loading, sources, caching labels May 10, 2024
@samhouts samhouts added the fixed-in-8.0.0-rc.2.9511 Look for this fix in 8.0.0-rc.2.9511 label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-controls-image Image control fixed-in-8.0.0-rc.2.9511 Look for this fix in 8.0.0-rc.2.9511 platform/ios platform/macos macOS / Mac Catalyst

Projects

None yet

7 participants