Closed
Bug 1184938
Opened 9 years ago
Closed 9 years ago
[Windows 10] Make the widget work for win10 XUL caption buttons depend on a window attribute
Categories
(Core :: Widget: Win32, defect, P2)
Core
Widget: Win32
Tracking
()
RESOLVED
WONTFIX
Tracking | Status | |
---|---|---|
firefox42 | --- | affected |
People
(Reporter: Gijs, Assigned: jaws)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
Paenglab
:
feedback-
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
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
Assignee | ||
Comment 2•9 years ago
|
||
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 3•9 years ago
|
||
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•9 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?
Updated•9 years ago
|
Priority: -- → P2
Comment 5•9 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•9 years ago
|
||
oops! sorry.. s/Jered/Jared
Assignee | ||
Comment 7•9 years ago
|
||
(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)
Assignee | ||
Comment 8•9 years ago
|
||
(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•9 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 10•9 years ago
|
||
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)
Comment 11•9 years ago
|
||
(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•9 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)
Comment 13•9 years ago
|
||
Aren't you already opting in by using the following CSS:
#titlebar-buttonbox {
-moz-appearance: -moz-window-button-box;
}
Comment 14•9 years ago
|
||
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•9 years ago
|
||
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-
Assignee | ||
Comment 16•9 years ago
|
||
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)
Comment 17•9 years ago
|
||
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)
Comment 18•9 years ago
|
||
SeaMonkey doesn't do anything silly like drawing in the title bar anyway.
Flags: needinfo?(neil)
Assignee | ||
Comment 19•9 years ago
|
||
OK, let's just close this then.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
Updated•9 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.
Description
•