dotnet / winforms

Windows Forms is a .NET UI framework for building Windows desktop applications.
MIT License
4.42k stars 984 forks source link

TabControl ImageList adds a white background in .NET8 #10462

Closed Monotek18 closed 9 months ago

Monotek18 commented 11 months ago

.NET version

.NET8

Did it work in .NET Framework?

Not tested/verified

Did it work in any of the earlier releases of .NET Core or .NET 5+?

Yes, .NET7

Issue description

When upgrading my Winforms project from .NET7 to .NET8 (while changing nothing to the code) I noticed that the images dynamically added to the tab through an ImageList add a blank background.

.NET7:

net7

.NET8: net8

Steps to reproduce

The following code is used for both of the screenshots above:

ImageList iconsList = new()
{
    TransparentColor = Color.Transparent,
    ColorDepth = ColorDepth.Depth32Bit,
    ImageSize = new Size(16, 16)
};
iconsList.Images.Add(Resources.GambitTabControl_0);
iconsList.Images.Add(Resources.GambitTabControl_1);
iconsList.Images.Add(Resources.GambitTabControl_2);
iconsList.Images.Add(Resources.GambitTabControl_3);
iconsList.Images.Add(Resources.GambitTabControl_4);
iconsList.Images.Add(Resources.GambitTabControl_5);
TBC_Gambit.ImageList = iconsList;

(Note: I am not sure if I need to update the code/UI or if this an issue with Winforms since .NET8, but I did not see anything regarding changes to TabControl)

elachlan commented 11 months ago

We don't have test coverage in the WinformsControlsTest test project. All the uses of ImageList use a white background.

I managed to reproduce with the treelist test by setting the backcolor to lightgray: image

@Monotek18 what image type are you using?

Monotek18 commented 11 months ago

Hello @elachlan,

I use PNG images. (with transparent background if that matter)

JeremyKuhne commented 11 months ago

@Monotek18 would you mind sharing an image you're actually using so we can make sure we correctly address this?

Monotek18 commented 11 months ago

@JeremyKuhne here are the images I used above: GambitTabControl.zip

Zheng-Li01 commented 11 months ago

Repro the issue on 8.0 & 9.0 with the images provided by @Monotek18, can't reproduce it on 6.0 & 7.0. after compared between 6.0/7.0 and 8.0/9.0. just noticed that the value of "Image Bit Depth" changed from Depth8Bit to Depth32Bit, not sure if this is the root cause. image

This changes, that value of "Image Bit Depth" changed from Depth8Bit to Depth32Bit, which comes form https://github.com/dotnet/winforms/issues/7021 & https://github.com/dotnet/winforms/pull/8366

elachlan commented 11 months ago

@Zheng-Li01 in .net 7 if you change the bit depth to 32bit, does it repro?

Zheng-Li01 commented 11 months ago

@elachlan For .net 7, the issue can't reproduce when changing the value of "Image Bit Depth" from Depth8Bit to Depth32Bit, For .net 8, the issue still reproduce when changing the value of "Image Bit Depth" from Depth32Bit to Depth8Bit, so maybe this is not the root cause...
image

3am-solu commented 11 months ago

I have exactly the same issue - identical in fact. Transparent 16 x 16 PNG files in an ImageList, suddenly showing white backgrounds in TabControls under .NET 8. Other control types not affected, but using the same ImageList and PNG files.

We skipped .NET 7, but this was not an issue in .NET 6 using identical code. Is there a workaround or are we stuck with this for now?

3am-solu commented 11 months ago

Did it work in .NET Framework?

Yes it did. .NET Framework 4.0, 4.8

elachlan commented 11 months ago

@3am-solu if you have the time. You could try and Bi-sect the regression.

mdonatas commented 10 months ago

@elachlan I've run the bisect and found this PR https://github.com/dotnet/winforms/pull/7866 to be the cause of this regression.

mdonatas commented 10 months ago

So here's the root cause. Before the https://github.com/dotnet/winforms/pull/7866 when calling ImageList_SetBkColor in ComCtl32 a value of -1 (ComCtl32.CLR.NONE) was passed. image

While after this PR We see that during HandleCreate a conversion difference is introduced image

And a value of 16777215 (#FFFFFF) is passed instead image

Investigating this has actually revealed a rather easy workaround of calling ImageList_SetBkColor with a -1 which was used before.

[DllImport("COMCTL32.dll", EntryPoint = "ImageList_SetBkColor")]
public static extern int SetBkColor(IntPtr himl, int clrBk);

private void Form1_Load(object sender, EventArgs e)
{
    var imgList = new ImageList
    {
        ColorDepth = ColorDepth.Depth32Bit,
        ImageSize = new Size(16, 16),
        Images = { Images.FileTree }
    };

    SetBkColor(imgList.Handle, -1); // Workaround

    tabControl1.ImageList = imgList;
    tabPage1.ImageIndex = 0;
}

p.s. Huge kudos 🙌 to the team that made building and debugging WinForms repo a breeze. I enjoyed bisecting and investigating this issue.

elachlan commented 10 months ago

@mdonatas did you want to submit a PR to fix this?

mdonatas commented 10 months ago

@mdonatas did you want to submit a PR to fix this?

No, I see that with this change a usage of COLORREF has been introduced and by looking at the System.Drawing.ColorTranslator code and some StackOverflow posts I get an impression that these two are not meant to be used together if opacity is needed. I don't have enough knowledge or context to understand what a correct fix should be. I'm glad I could help with the investigation :)

elachlan commented 10 months ago

@JeremyKuhne Can you sanity check this for me?

The headers have:

#define CLR_NONE                0xFFFFFFFFL

https://github.com/microsoft/win32metadata/blob/12d9e5a76731733a48dcc580a68f4a5aca2d4170/generation/WinSDK/RecompiledIdlHeaders/um/CommCtrl.h#L495-L496

L being long.

Metadata is defining it as: public const int CLR_NONE = -1;,

I think this is a metadata issue which CLR_NONE should be uint not int.

@riverar What do you think?

JeremyKuhne commented 10 months ago

@elachlan because it is L that says it is a long in C, not unsigned long, so int is appropriate in C#.

riverar commented 10 months ago

This becomes an unsigned long in MSVC, so does look like a metadata bug.

elachlan commented 10 months ago

I passed 0xFFFFFFFF to COLORREF directly instead of using the CLR_NONE, but it didn't fix the treeview. More investigation will be needed.

riverar commented 10 months ago

I can help debug this, just trying to get all the pieces line up.

Counter opinion: Working with this repository is a huge pain due to its reliance on bleeding edge .NET.

riverar commented 10 months ago

So comctl32!ImageList_SetBkColor is actually called twice.

  1. The first is in ImageList.CreateHandle (https://github.com/dotnet/winforms/blob/main/src/System.Windows.Forms/src/System/Windows/Forms/Controls/ImageList/ImageList.cs)

    COLORREF has a implicit conversion for Color that drops high-order byte data on the floor; it loses that byte because it runs the Color through ColorTranslator.ToWin32 which only works with RGB values.

    Creating the COLORREF instance directly (e.g. new COLORREF(unchecked((uint)PInvoke.CLR_NONE))) is one (not ideal) approach.

  2. The second is indirectly by way of the deserialized ImageList stream running through ImageList_Read. I ran out of time debugging that tonight but peeking under the hood, the ImageList header does include a clrBk member. So perhaps there's an issue with image loading in the designer or in code handling resource files.

Hope that helps a bit.

riverar commented 10 months ago

Looking at the decompressed persisted ImageList on disk, I see clrBk lost a byte:

49 4C 01 01 06 00 38 00 38 00 10 00 10 00 FF FF FF 00 21 00 . . .
│   │ │   │                               │         │      
┼───┼ ┼───┼                               ┼─────────┼      
"IL"  ver                                 clrBk            
riverar commented 9 months ago

image

Not seeing this as fixed in latest master, using the sample I provided https://github.com/dotnet/winforms/pull/10696#issuecomment-1899386924

@JeremyKuhne are you seeing otherwise with my sample?

JeremyKuhne commented 9 months ago

@riverar the designer needs to reserialize the resx data, but it should be fine after that is regenerated. I'll double check after we get a daily SDK with the change in it.

Olina-Zhang commented 9 months ago

Tested this issue in the latest .Net 9.0 SDK build: 9.0.100-preview.2.24074.16, it was fixed: image

riverar commented 9 months ago

That's great, thanks @Olina-Zhang!

@elachlan If you're familiar with setting up a debug environment, would love to compare notes. It seems my local testing w/ start-vs.cmd failed here for some reason.

elachlan commented 9 months ago

I am unsure. I am fairly sure start-vs should install all the required features to visual studio.

What error do you get when running it?

Philip-Wang01 commented 9 months ago

Verified this issue with .NET 9 Preview 1 test pass build, it was fixed. Test result is same as above.

Nora-Zhou01 commented 9 months ago

Verified issue on .NET 8.0.300 with dlls built from Release/8.0 branch of Winforms repo, the issue was fixed, the test results are as follows. image

Philip-Wang01 commented 8 months ago

Verified with .NET SDK 8.0.3 test pass build, this issue was fixed. Test result is same as above.

Zheng-Li01 commented 8 months ago

Verified with .NET SDK 9.0.100-preview.2.24129.7 test pass build, this issue was fixed. Test result is same as above.