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

RESOLVED WONTFIX

Status

()

Core
Widget: Win32
P2
normal
RESOLVED WONTFIX
3 years ago
3 years ago

People

(Reporter: Gijs, Assigned: jaws)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 affected)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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.
Created attachment 8636251 [details] [diff] [review]
WIP Patch

remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=36e3a2bf1b32
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Created attachment 8636292 [details] [diff] [review]
Patch

remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1f747778f38
Attachment #8636251 - Attachment is obsolete: true
Attachment #8636292 - Flags: review?(jmathies)
Attachment #8636292 - Flags: review?(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.
Attachment #8636292 - Flags: review?(dao) → review+

Comment 4

3 years ago
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?
Priority: -- → P2

Comment 5

3 years ago
Comment on attachment 8636292 [details] [diff] [review]
Patch

Jered is working on an update post a discussion on irc.
Attachment #8636292 - Flags: review?(jmathies) → review-

Comment 6

3 years ago
oops! sorry.. s/Jered/Jared
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.
Attachment #8636292 - Attachment is obsolete: true
Attachment #8636796 - Flags: review?(jmathies)
Attachment #8636796 - Flags: feedback?(kairo)
Attachment #8636796 - Flags: feedback?(acelists)
(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

3 years ago
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?
Attachment #8636796 - Flags: feedback?(kairo) → feedback?(philip.chee)
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.
Attachment #8636796 - Flags: feedback?(acelists) → feedback?(richard.marti)
(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

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

Neil has a Windows 10 VM
Attachment #8636796 - Flags: feedback?(philip.chee) → feedback?(neil)
Aren't you already opting in by using the following CSS:

#titlebar-buttonbox {
  -moz-appearance: -moz-window-button-box;
}
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 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
Attachment #8636796 - Flags: feedback?(richard.marti) → feedback-
Richard and Neil, is this something that we should continue pursuing? Or are you fine without this bug being fixed?
Flags: needinfo?(richard.marti)
Flags: needinfo?(neil)
I'm okay with WONTFIXing this bug. TB's next public version is ESR45 and we have enough time to implement it.
Flags: needinfo?(richard.marti)
SeaMonkey doesn't do anything silly like drawing in the title bar anyway.
Flags: needinfo?(neil)
OK, let's just close this then.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX

Updated

3 years ago
Attachment #8636796 - Flags: review?(jmathies)
Attachment #8636796 - Flags: feedback?(neil)
You need to log in before you can comment on or make changes to this bug.