1947324 - Taskbar windows (thumbnails/titles) lack context menu

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P3)

Description

[jscher2000]

Starting in Firefox 135, right-clicking a window thumbnail or window title displayed by hovering the button on the Windows Taskbar does not generate a context menu. Instead, the right-click functions like a left click.

According to a mozregression by SUMO user JC, the regression was caused by bug 1934040 (removes WS_SYSMENU for Mica titlebar).

https://support.mozilla.org/en-US/questions/1491135#answer-1711172

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1934040

:emilio, since you are the author of the regressor, bug 1934040, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 2

[Emilio Cobos Álvarez [:emilio]]

So this is indeed a regression from that bug. The good thing is that there's an easy workaround for that menu, which is either:

• Using Alt+Space on the keyboard.
• Enabling the native titlebar (right click -> customize toolbar -> titlebar checkbox).

The bad news is that I don't see a great way of making that work without regressing bug 1934040: When I trigger the right click, I don't see any message sent to our window to potentially handle it (unlike the Alt+Space case which gives us a SYSCOMMAND message).

Maybe there's another way to convince DWM not to paint the caption button? https://learn.microsoft.com/en-us/windows/windows-app-sdk/api/winrt/microsoft.ui.windowing.titlebarheightoption?view=windows-app-sdk-1.6 has a "Collapsed" variant. But I have no idea how that is implemented under the hood...

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Attachment: WIP: Bug 1947324 - Try to use WinRT rather than WS_SYSMENU to collapse titlebar.

This insta-crashes on the first call to the Interop... function.

Comment 7

[Greg Stoll :gstoll]

I'm going to take another look at this. After chatting with :handyman, I think our best path forward is to:

• restore the WS_SYSMENU flag on the window (this fixes the context menu problem)
• set AppWindowTitlebar.PreferredHeightOption to Tall and let Windows draw the titlebar buttons.

The second point will be tricky, although we do use some WinRT functionality already (an example)

Comment 8

[Emilio Cobos Álvarez [:emilio]]

It would be better / less invasive / simpler to use the undocumented "collapsed" height. That seemed to work in my testing with a local application.

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Oh, it is mentioned (but not described) in https://learn.microsoft.com/en-us/windows/windows-app-sdk/api/winrt/microsoft.ui.windowing.titlebarheightoption?view=windows-app-sdk-1.7

Comment 10

[Greg Stoll :gstoll]

Either way, we will need to call MddBootstrapInitialize() before doing any of this stuff. I've been working on this, and it's not going great. (about to go on PTO so leaving some notes for myself here)

I've been trying to pull in this folder on GitHub which has the definition of MddBootstrapInitialize(). It depends on wil and is generally just a lot of code that I haven't gotten to even compile yet.

Perhaps instead of trying to include enough stuff to make these files compile, I should try to pull out the core function FirstTimeInitialization() and trim out everything except the essentials.

Comment 11

[David Parks [:handyman]]

I certainly don't know the limitations of this but Microsoft.WindowsAppRuntime.Bootstrap.dll should be available -- I've got it at "C:\Windows\SystemApps\Microsoft.WindowsAppRuntime.CBS_8wekyb3d8bbwe\Microsoft.WindowsAppRuntime.Bootstrap.dll". dumpbin shows that it has the expected MddBootstrapInitialize* methods.

Comment 12

[Greg Stoll :gstoll]

OK, I have this DLL too - the good news is that I am able to ::LoadLibrary() on it and ::LoadProcAddress() on MddBootstrapInitialize2(). In order for the call to succeed, though, I had to install a "real" Windows App SDK from here. I was missing the "Microsoft.WinAppRuntime.DDLM" package, among others, and this is the Dynamic Dependency Lifetime Manager which apparently keeps the package from being modified while an unpackaged app (Firefox in this case) is using it.

So I think to be able to use this we would have to package a Windows App SDK installer in the Firefox installer. Microsoft has instructions on how to do this but I'm not sure if this would be acceptable for us.

Another possibility is to make Firefox a self-contained app which would just put the dependencies next to firefox.exe on disk. This might be more acceptable.

Anyway, for now I'm going to press onward and see if I can get the functionality we want with the titlebar to actually work.

Comment 13

[Greg Stoll :gstoll]

OK, I'm able to call these methods by including all those WinRT headers. If I used the "collapsed" height then the minimize/maximize/close buttons are not drawn at all - I assume there's some change I need to make to make Firefox draw these, :emilio?

Comment 14

[Emilio Cobos Álvarez [:emilio]]

Can you elaborate / attach a screenshot? Firefox draws its own titlebar buttons by default, so unless you've made any CSS change it should keep drawing its own.

Comment 15

[Greg Stoll :gstoll]

Oh, thanks, I had picked up a CSS change I forgot about, and reverting that made it work. Sorry!

Comment 16

[Greg Stoll :gstoll]

I'm going to explore the self-contained route more. If we do this we don't even have to call MddBootstrapInitialize() - the point of that is to make sure the package doesn't get updated while we're using it, but that isn't an issue if we're using our own version of it.

Comment 17

[Greg Stoll :gstoll]

I have this almost entirely working. The one part that doesn't is that if browser.tabs.inTitlebar is 0 (so we get the "standard" Windows titlebar with no tabs in it), the minimize/maximize/close buttons don't show up. I'm setting AppWindowTitleBar.ExtendsContentIntoTitleBar to false in this case, but setting PreferredHeightOption to Standard returns an error (since, as the documentation notes, that property only applies if ExtendsContentIntoTitleBar is true.

I assume that we are suppressing these buttons some other way - do you happen to know what that is, and if I should be doing something else in nsWindow::SetCustomTitlebar()?

(let me know if it would help to share my change; it's in a stack of ~25 patches now but I can squash it down)

Comment 18

[Emilio Cobos Álvarez [:emilio]]

Do they not show up only in the first window open? Or in all of them?

Sure, sharing the patch would help though most likely you just need to make sure that the WS_SYSMENU is set (undoing bits of the regressing bug). In particular the skeleton UI need to recover that bit as well.

If it's not that, please send me a GitHub / try / something else link and I can take a look?

Comment 19

[Greg Stoll :gstoll]

Hmm, it’s definitely on the first window, I’m not sure I tried others.

Here’s a non-squished patch, I’ll get you a better one when I’m at my computer in around 2 hours: https://treeherder.mozilla.org/jobs?repo=try&revision=f5f330d6c759fee32abb78d4c15c8049cae04e54

Comment 20

[Greg Stoll :gstoll]

OK, here's a squished version: https://treeherder.mozilla.org/jobs?repo=try&revision=e6077d6b9c2c07fdf7a2b69c3824d6137256dd84

After some more testing, indeed the buttons don't show up only on the first window open; subsequent windows are fine. (even if they're open when I toggle the pref) So I think you're right that this is a problem with the skeleton UI, but my change has WS_SYSMENU added to kPreXULSkeletonUIWindowStyle, which I thought would be sufficient. I'll play around with that some more.

Comment 21

[Greg Stoll :gstoll]

Weird, Spy++ shows that the window does have WS_SYSMENU. (and in fact has the exact same styles as a new window, which does have the titlebar correctly) And this problem only happens if I launch with browser.tabs.inTitlebar turned on and then turn it off.

Comment 22

[Greg Stoll :gstoll]

If I disable the skeleton UI the problem goes away entirely. So the problem is definitely related to the skeleton UI, but it doesn't seem related to WS_SYSMENU.

Comment 23

[Greg Stoll :gstoll]

I figured it out - the problem was this line which doesn't seem to be true anymore (we do seem to use the skeleton UI in both cases), and in any event doesn't seem necessary.

Comment 24

[Greg Stoll :gstoll]

Attachment: Bug 1947324 part 1 - "build" Windows App SDK on toolchain tasks r=glandium!

This downloads the installers for the various flavors of Windows App SDK
and extracts the needed DLLs from them.

Comment 25

[Greg Stoll :gstoll]

Attachment: Bug 1947324 part 2 - make Windows Firefox builds depend on the Windows App SDK r=glandium!

This also copies the needed DLLs to the dist/bin directory so Firefox
will be able to load them at runtime.

Comment 26

[Greg Stoll :gstoll]

Attachment: Bug 1947324 part 3 - use WinRT/Windows App SDK instead of WS_SYSMENU to collapse titlebar r=emilio!

This essentially reverts bug 1934040, as that attempted to deal with
the problem of the wrong titlebar height with Mica on by not setting the
WS_SYSMENU style. However, this caused problems like this bug, where
taskbar windows couldn't be right-clicked on.

Now we accomplish the same thing by calling into the Windows App SDK to
set ExtendsContentIntoTitleBar and PreferredHeightOption appropriately.
This is done in WindowsUIUtils::SetIsTitlebarCollapsed(); the rest of this
change is almost entirely the revert of bug 1934040.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/37b6a1f956e6
https://hg.mozilla.org/integration/autoland/rev/9f8f7adb239f
part 1 - "build" Windows App SDK on toolchain tasks r=glandium
https://github.com/mozilla-firefox/firefox/commit/1ebdc110f24d
https://hg.mozilla.org/integration/autoland/rev/b21e12d7591d
part 2 - make Windows Firefox builds depend on the Windows App SDK r=glandium
https://github.com/mozilla-firefox/firefox/commit/032488ff117d
https://hg.mozilla.org/integration/autoland/rev/8aa8ba6921ef
part 3 - use WinRT/Windows App SDK instead of WS_SYSMENU to collapse titlebar r=emilio

Comment 28

[Greg Stoll :gstoll]

Attachment: Bug 1947324 part 4 - fix build errors with shippable builds

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/4a2a50478740
https://hg.mozilla.org/integration/autoland/rev/2a5a2c2d19c3
Revert "Bug 1947324 part 3 - use WinRT/Windows App SDK instead of WS_SYSMENU to collapse titlebar r=emilio" for causing multiple failures.

Comment 30

[Iulian Moraru]

Revert for causing multiple failures.

• Push with failures

• Failure log for build bustages

• Failure log for reftest failures

• Backout link

Comment 31

[amarc]

There's also this talos failure -> https://treeherder.mozilla.org/logviewer?job_id=527826775&repo=autoland&task=JW04Rn9qT_Wqa5uOkznkAA.0&lineNumber=1141

Comment 32

[Florin Bilt]

(In reply to Iulian Moraru from comment #30)

Revert for causing multiple failures.

• Push with failures

• Failure log for build bustages

• Failure log for reftest failures

• Backout link

Perfherder has detected a build_metrics performance change from push cb3327fb6325e42c645d4c5349b8e5d6bd2dc406.

If you have any questions, please reach out to a performance sheriff. Alternatively, you can find help on Slack by joining #perf-help, and on Matrix you can find help by joining #perftest.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
2%	installer size	windows2012-64-shippable	nightly	129,503,459.92 -> 127,334,899.42

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a performance sheriff to do that for you.

You can run all of these tests on try with ./mach try perf --alert 46873

The following documentation link provides more information about this command.

Comment 33

[Greg Stoll :gstoll]

Attachment: Bug 1947324 part 4 - add a few DLLs to xperf allowlists r=#perftest-reviewers

As a result of using the Windows App SDK we load a few more DLLs now.
Note that the "::$ea" syntax refers to a file's extended attributes.

Comment 34

[Greg Stoll :gstoll]

Attachment: Bug 1947324 part 5 - remove assert for unstacked window procs on Windows r=#win-reviewers

Now that we're using the Windows App SDK to remove the minimize/maximize/close
buttons from the toolbar, the Windows App SDK seems to associate its own
window proc with the window, so this assertion was failing. Unstacking it
in this way doesn't seem to cause any issues.

Comment 35

[Greg Stoll :gstoll]

Attachment: Bug 1947324 part 6 - avoid using Windows App SDK on opt-talos-xperf runs r=bobowen!,#win-reviewers

On certain test runs (most notably opt-talos-xperf), Firefox runs
without local login privileges. This causes the Windows App SDK to
fail calling DCompositionCreateDevice3() with ERROR_ACCESS_DENIED,
and the code assumes it is going to succeed so it proceeds to crash
deferencing null.

So detect this case and don't use the Windows App SDK. I don't expect
any users will run into this, so it's unfortunate that we have to check,
but we just do it once.

Comment 36

[Greg Stoll :gstoll]

Green try run: https://treeherder.mozilla.org/jobs?repo=try&revision=85da456a4a7db0d78698594ef1c62de644b9d084

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/d5f33314b3aa
https://hg.mozilla.org/integration/autoland/rev/8413a6fa9a77
part 1 - "build" Windows App SDK on toolchain tasks r=glandium
https://github.com/mozilla-firefox/firefox/commit/af6ce89b6983
https://hg.mozilla.org/integration/autoland/rev/e488b02f821c
part 2 - make Windows Firefox builds depend on the Windows App SDK r=glandium
https://github.com/mozilla-firefox/firefox/commit/2fe4ae073010
https://hg.mozilla.org/integration/autoland/rev/5f22dfcea973
part 3 - use WinRT/Windows App SDK instead of WS_SYSMENU to collapse titlebar r=emilio
https://github.com/mozilla-firefox/firefox/commit/af539d638d26
https://hg.mozilla.org/integration/autoland/rev/0c055e29e718
part 4 - add a few DLLs to xperf allowlists r=perftest-reviewers,kshampur
https://github.com/mozilla-firefox/firefox/commit/796486a95674
https://hg.mozilla.org/integration/autoland/rev/26ae4fd252cd
part 5 - remove assert for unstacked window procs on Windows r=win-reviewers,handyman
https://github.com/mozilla-firefox/firefox/commit/2374da507a9b
https://hg.mozilla.org/integration/autoland/rev/53aa44de71d6
part 6 - avoid using Windows App SDK on opt-talos-xperf runs r=bobowen,perftest-reviewers,kshampur

Comment 38

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/8413a6fa9a77
https://hg.mozilla.org/mozilla-central/rev/e488b02f821c
https://hg.mozilla.org/mozilla-central/rev/5f22dfcea973
https://hg.mozilla.org/mozilla-central/rev/0c055e29e718
https://hg.mozilla.org/mozilla-central/rev/26ae4fd252cd
https://hg.mozilla.org/mozilla-central/rev/53aa44de71d6