610071 - Stretch the app button to match the caption buttons' size

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

We're currently making bogus assumptions about the font size and the size of caption buttons. One symptom of this is bug 578620.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 488613 [details] [diff] [review]
patch

>diff --git a/browser/themes/winstripe/browser/browser-aero.css b/browser/themes/winstripe/browser/browser-aero.css

> @media all and (-moz-windows-compositor) {
>   /* these should be hidden w/glass enabled. windows draws it's own buttons. */
>-  #titlebar-buttonbox:not(:-moz-lwtheme),
>   .titlebar-button {
>     display: none;

How is this related to the summary?

align=start apparently impacts layout beyond its description at https://developer.mozilla.org/en/XUL/Attribute/align, and I forget exactly how. XUL layout is incredibly non-intuitive to me, so reviewing these patches is really a nightmare. I have no idea what the potential side-effects of these changes are, and can't just review it by inspection. It would help if you could elaborate on the reasoning behind each change...

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 488613 [details] [diff] [review]
patch

>--- a/browser/base/content/browser.xul
>+++ b/browser/base/content/browser.xul

> #ifdef CAN_DRAW_IN_TITLEBAR
>   <vbox id="titlebar">
>   <hbox id="titlebar-content">
>-  <hbox id="appmenu-button-container" align="start">
>+  <hbox id="appmenu-button-container">
>   <button id="appmenu-button"
>           type="menu"
> #ifdef XP_WIN
>           label="&brandShortName;"
> #else
>           label="&appMenuButton.label;"
> #endif
>           style="-moz-user-focus: ignore;">
> #include browser-appmenu.inc
>   </button>
>   </hbox>

>--- a/browser/themes/winstripe/browser/browser.css
>+++ b/browser/themes/winstripe/browser/browser.css

> #titlebar-content {
>   margin-left: 15px;
>   margin-right: 15px;
>-  -moz-box-align: start;
> }

These two changes let the app button stretch across titlebar-content rather than being aligned to the top.

>--- a/browser/themes/winstripe/browser/browser.css
>+++ b/browser/themes/winstripe/browser/browser.css
>@@ -117,17 +117,17 @@
>   border: 1px solid rgba(83,42,6,.9);
>   border-top: none;
>   box-shadow: 0 1px 0 rgba(255,255,255,.25) inset,
>               0 0 0 1px rgba(255,255,255,.25) inset;
>   color: white;
>   text-shadow: 0 0 1px rgba(0,0,0,.7),
>                0 1px 1.5px rgba(0,0,0,.5);
>   font-weight: bold;
>-  padding: .1em 1.5em .15em;
>+  padding: 0 1.5em .05em;
>   margin: 0;
> }

The vertical padding attempted to approximately match the caption buttons' height. The rest of this patch obsoletes that approach. The remaining 0.05em bottom padding maintains the vertical alignment within the button.

>--- a/browser/base/content/browser.xul
>+++ b/browser/base/content/browser.xul

>   <spacer id="titlebar-spacer" flex="1"/>
>-  <hbox id="titlebar-buttonbox">
>+  <hbox id="titlebar-buttonbox" align="start">
>     <toolbarbutton class="titlebar-button" id="titlebar-min" oncommand="window.minimize();"/>
>     <toolbarbutton class="titlebar-button" id="titlebar-max" oncommand="onTitlebarMaxClick();"/>
>     <toolbarbutton class="titlebar-button" id="titlebar-close" command="cmd_closeWindow"/>
>   </hbox>
>   </hbox>
>   </vbox>

This lets thet title bar buttons not stretch in case they are smaller than the app button's minimum height.

>--- a/browser/themes/winstripe/browser/browser-aero.css
>+++ b/browser/themes/winstripe/browser/browser-aero.css
>@@ -24,16 +24,17 @@
>   }
> 
>   #appmenu-button {
>     border: 2px solid;
>     border-top: none;
>     -moz-border-left-colors: rgba(255,255,255,.5) rgba(83,42,6,.9);
>     -moz-border-bottom-colors: rgba(255,255,255,.5) rgba(83,42,6,.9);
>     -moz-border-right-colors: rgba(255,255,255,.5) rgba(83,42,6,.9);
>+    margin-bottom: -1px; /* compensate white outer border */
>     box-shadow: 0 1px 0 rgba(255,255,255,.25) inset,
>                 0 0 2px 1px rgba(255,255,255,.25) inset;
>   }

This lets the button stretch one pixel further down for with the white outline added for aero basic and glass.

> @media all and (-moz-windows-compositor) {
>   /* these should be hidden w/glass enabled. windows draws it's own buttons. */
>-  #titlebar-buttonbox:not(:-moz-lwtheme),
>   .titlebar-button {
>     display: none;
>   }

Not hiding the button box allows the app button to pick up that height rather than being sqeezed.

Comment 3

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/65e97b75aca8