1184938 - [Windows 10] Make the widget work for win10 XUL caption buttons depend on a window attribute

Status: RESOLVED WONTFIX

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

Description

[:Gijs (he/him)]

I'm kind of sad I didn't think about this sooner, but anyway: right now widget will assume we don't need caption button mouse processing if we're on win10, are using custom margins (ie drawing tabs in the titlebar) and so on. This will affect Thunderbird/SeaMonkey if they draw in the titlebar. We should probably make this behaviour opt-in in some way ("customcaptionbuttons=true" or something?) to avoid breaking non-Firefox things.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: WIP Patch

remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=36e3a2bf1b32

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1f747778f38

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 8636292 [details] [diff] [review]
Patch

>--- a/browser/base/content/browser.xul
>+++ b/browser/base/content/browser.xul
>@@ -45,16 +45,19 @@
> #ifdef CAN_DRAW_IN_TITLEBAR
> #ifdef XP_WIN
>         chromemargin="0,2,2,2"
> #else
>         chromemargin="0,-1,-1,-1"
> #endif
>         tabsintitlebar="true"
> #endif
>+#ifdef XP_WIN
>+        customcaptionbuttons="true"
>+#endif

Group this with chromemargin="0,2,2,2" in the existing XP_WIN ifdef. r+ on the browser bit with this change.

Comment 4

[Jim Mathies [:jimm]]

Comment on attachment 8636292 [details] [diff] [review]
Patch

Review of attachment 8636292 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsGkAtomList.h
@@ +193,5 @@
>  GK_ATOM(childList, "childList")
>  GK_ATOM(choose, "choose")
>  GK_ATOM(chromemargin, "chromemargin")
>  GK_ATOM(chromeOnlyContent, "chromeOnlyContent")
> +GK_ATOM(customcaptionbuttons, "customcaptionbuttons")

what does this actually do? Like if you were going to write the docs for it, what would you write?

::: widget/nsIWidget.h
@@ +2090,5 @@
> +     * the OS to draw its own window caption controls.
> +     *
> +     * @param aState Whether space for custom caption controls should be set aside.
> +     */
> +    virtual void SetCustomCaptionButtons(bool aState) = 0;

You need to rev the uuid of nsIWidget up top.

::: widget/windows/nsWindow.cpp
@@ +3728,1 @@
>      for (size_t i = 0; i < aThemeGeometries.Length(); i++) {

The added |} else {| here seems odd. For example we'll end up cutting out the caption hole on win10 if !mCustomNonClient, or if the window is maximized. Is this right?

Comment 5

[Jim Mathies [:jimm]]

Comment on attachment 8636292 [details] [diff] [review]
Patch

Jered is working on an update post a discussion on irc.

Comment 6

[Jim Mathies [:jimm]]

oops! sorry.. s/Jered/Jared

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.1

(In reply to Jim Mathies [:jimm] from comment #4)
> > +GK_ATOM(customcaptionbuttons, "customcaptionbuttons")
> 
> what does this actually do? Like if you were going to write the docs for it,
> what would you write?

When this attribute is set to "true", Gecko will not cut out the region of the window frame for Windows' window caption buttons. This only applies to Windows 10 and higher.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)

Kairo and :aceman, are you able to apply this patch and test out that the Windows caption buttons work good for both SeaMonkey and Thunderbird, respectively?

Comment 9

[Robert Kaiser]

Comment on attachment 8636796 [details] [diff] [review]
Patch v1.1

Sorry, I don't have a Windows build environment. That said, SeaMonkey isn't even building right now on trunk and I guess neither is Thunderbird.

Once it is, Philip, do you have an environment where you can test this?

Comment 10

[Richard Marti (:Paenglab)]

Comment on attachment 8636796 [details] [diff] [review]
Patch v1.1

Aceman has no Win10 environment. So I'm changing the feedback to me. I'll test the patch this evening.

What I can say is, locally I have now the caption buttons working on TB like FX without this patch.

Comment 11

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> Created attachment 8636796 [details] [diff] [review]
> Patch v1.1
> 
> (In reply to Jim Mathies [:jimm] from comment #4)
> > > +GK_ATOM(customcaptionbuttons, "customcaptionbuttons")
> > 
> > what does this actually do? Like if you were going to write the docs for it,
> > what would you write?
> 
> When this attribute is set to "true", Gecko will not cut out the region of
> the window frame for Windows' window caption buttons. This only applies to
> Windows 10 and higher.

Note that according to bug 1179283, the cutout is broken, so I'm not sure how useful it is to keep that path around for Windows 10.

Comment 12

[Philip Chee]

Comment on attachment 8636796 [details] [diff] [review]
Patch v1.1

Neil has a Windows 10 VM

Comment 13

[[e-mail]]

Aren't you already opting in by using the following CSS:

#titlebar-buttonbox {
  -moz-appearance: -moz-window-button-box;
}

Comment 14

[[e-mail]]

Sorry, I misread the patch - too many !s.

You use that CSS to opt-in to caption button processing, but you want to be able to opt back out again on win10 or later? Do we have a @media query we can use to opt out on win10 or later in CSS? (Since only Firefox is using that CSS to opt-in, I'm not quite sure I see what the opt-out achieves.)

Comment 15

[Richard Marti (:Paenglab)]

Comment on attachment 8636796 [details] [diff] [review]
Patch v1.1

I see no advantage of this patch for TB. The LW-theme cutout is too small and the styling of the buttons is still needed. Although the caption buttons are shown they are not working.

If the caption buttons would work natively like on Win 7 and 8 this would help until TB, SM and IB have their own styling like FX. But I think this would probably need some changes in the changes from bug 1173725

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Richard and Neil, is this something that we should continue pursuing? Or are you fine without this bug being fixed?

Comment 17

[Richard Marti (:Paenglab)]

I'm okay with WONTFIXing this bug. TB's next public version is ESR45 and we have enough time to implement it.

Comment 18

[[e-mail]]

SeaMonkey doesn't do anything silly like drawing in the title bar anyway.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

OK, let's just close this then.