Steps to reproduce:

1. Play a video in pip
2. Hover over Firefox icon in taskbar

Actual:
Picture-in-Picture has "missing icon" icon

Expected:
Picture-in-Picture has Firefox icon or some other icon

¡Hola!

Just noticed this on Nightly.

Updating flags accordingly FWIW.

I <3 PiP BTW.

¡Gracias!
Alex

To fix this, in theory from player.js we should be able to use this utility method: https://searchfox.org/mozilla-central/rev/8f97d6d022bd18d4c3138bf5f13f4144ac334d82/widget/nsIWindowsUIUtils.idl#18 .

Currently used from https://searchfox.org/mozilla-central/rev/8f97d6d022bd18d4c3138bf5f13f4144ac334d82/browser/components/ssb/WindowsSupport.jsm#129,155

Although tbh, if we just want to use the main application icon, I don't know why that doesn't work already...

I've learned a few things about this bug today and I want to note them down here.

1. I'm having a difficult time reproducing this bug because my taskbar previews don't actually have window icons (or titles) in them. I do still get them in one of my VM's which is running an older Windows 10 build, so I'm guessing that this is a change that was made to the Windows taskbar itself and I just didn't notice at the time. If I'm right, that's good news because it means the impact of this bug will gradually decrease over time as users update Windows, but there could also be some other variable I haven't thought of.

2. This bug is a regression from bug 1543027 (although I would argue that bug was worse than this one, so it seems a good trade-off). Our Windows widget code assigns the application icon to all windows that aren't dialogs, so turning the PIP window into a dialog took its icon away. But we can't fix this bug by removing the dialog feature, because that would probably bring back bug 1543027.

3. I think we could use comment 3's suggestion of nsIWindowsUIUtils::setWindowIcon(), but the difficulty is getting the icon data; the example from comment 3 doesn't help here any because the SSB code it refers to has been removed, and because it was apparently relying on getting icon links from out of the PWA manifest. I'm not sure how to get an imgIContainer to pass to setWindowIcon for the application icon, so it looks (for now) like the main challenge in this bug is figuring that out. Another idea I had is that we could use the site's favicon for PIP windows instead, so there's a way to differentiate them, but I don't know how to get that icon data either (and if it's harder then it's probably not worth it).

¡Hola Molly!

Thanks for looking into this bug.

I just wanted to comment on 1.

This PC is in Windows 10 Insider Preview 21296.1000 (rs_prerelease) and I still see both icons and titles on taskbar previews in both Firefox and Windows Explorer, please see the next two attachments.

Perhaps you modified some setting on your settings?

¡Gracias!
Alex

(In reply to Molly Howell (she/her) [:mhowell] from comment #5)

1. I'm having a difficult time reproducing this bug because my taskbar previews don't actually have window icons (or titles) in them. I do still get them in one of my VM's which is running an older Windows 10 build, so I'm guessing that this is a change that was made to the Windows taskbar itself and I just didn't notice at the time. If I'm right, that's good news because it means the impact of this bug will gradually decrease over time as users update Windows, but there could also be some other variable I haven't thought of.

Can you please check the STR #2 in bug 1684939 to see if that's the cause?
I think my settings are configured similarly to yours.

(In reply to Itiel from comment #9)

Can you please check the STR #2 in bug 1684939 to see if that's the cause?
I think my settings are configured similarly to yours.

Ah, you're both right, the taskbar grouping setting is what makes these "title bars" appear in the previews. I thought maybe I had missed something and I had. Thank you!

I came up with one relatively easy way I think we can get away with for fixing this. Note that this will require having a Windows native (that is, not artifact) build set up, because there is IDL and C++ involved.

As has been discussed already, we have a SetWindowIcon method that gets us part of the way there, but it requires you to have icon data to give it, and there's no easy way to get that. The thing is, that method just calls into the Win32 widget layer, and that layer is holding a reference to the default icon. So all we need is a way to tell it to use that instead of a supplied icon, and then to insert a call to that new method when the PIP window is created.

So, step by step:

1. Add new methods to the Win32 nsWindow class (it's a C++ class, so that means they need to also be declared in the header) based on the existing Set*Icon methods but using ::LoadIconW(::GetModuleHandleW(nullptr), gStockApplicationIcon) instead of a supplied HICON (try not to worry about all the Win32 shenanigans, but I'm happy to go over that if you want).
2. Add a new setDefaultWindowIcon method to the IDL file that doesn't take any icon parameters (it does still need to know which window to use, so it still needs that parameter). Here's some background on what these IDL files even are, but it's just background, you won't have to know all that to do this. TL;DR: it's a language that allows us to declare methods that can be written in C++ and then called from Javascript (among other things, but that's all we need here).
3. Implement that method in C++ using a modified version of the existing setWindowIcon implementation as a template, the difference being we need to call the methods we added in step 1 instead of the existing Set*Icon methods.
4. Insert a call to that new IDL method right after this call which creates the PIP window.

After all that, the PIP windows should start getting Firefox icons.

Molly, do you think there is a chance an Outreachy intern might help us with this?

I'm hesitant to ask to estimate this for MR1, but PiP looks on Windows would really benefit from having a proper FF icon instead.

I'll add it to the list, sure. I don't think this is a very large task.

Also, I'm removing the existing assignment because there's been no activity.

https://hg.mozilla.org/mozilla-central/rev/3074eed04b12

Reproduced the issue with Firefox 97.0a1 (20220109215539) on Windows 10x64.
The issue is verified fixed with Firefox 98.0b4 (20220213185901) on Windows 10x64. The Firefox icon is correctly displayed for the PiP window inside the taskbar.