Closed Bug 575870 Opened 14 years ago Closed 14 years ago

Implement the firefox button on xp, classic, and aero basic

Categories

(Firefox :: Theme, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 4.0b5
Tracking Status
blocking2.0 --- beta5+

People

(Reporter: jimm, Assigned: jimm)

References

Details

Attachments

(13 files, 15 obsolete files)

41.50 KB, image/png
Details
230.19 KB, image/png
Details
935.57 KB, image/png
Details
231.82 KB, image/png
Details
72.37 KB, image/png
Details
10.10 KB, patch
dao
: review+
Details | Diff | Splinter Review
25.50 KB, image/jpeg
Details
40.99 KB, image/png
Details
20.63 KB, image/png
Details
23.86 KB, image/png
Details
8.84 KB, image/jpeg
Details
14.24 KB, patch
Details | Diff | Splinter Review
7.42 KB, patch
roc
: review+
dao
: review+
Details | Diff | Splinter Review
This is waiting on bug 574454. Once we have the ability to implement window frame, titlebar, and caption buttons via xul/css, we'll need to add support for this in firefox. This will be for non-glass desktops only, as glass desktops make use of the system frame.
Blocks: 576960
blocking2.0: --- → ?
Moving these down since they will depend on the dimensions of the frame we implement.
Blocks: 578616, 581023
blocking2.0: ? → betaN+
Blocks: 582819
Attached patch patch v.1 (obsolete) — Splinter Review
first rev., tested on win7. I still need to do testing on xp tomorrow. This also address the dependent bugs related to top window padding.
Assignee: nobody → jmathies
Note, the diff for browser.xul looks a little scary, but the actual changes involve new header and footer elements for appmenu-button-container. The rest is all indentation.
Attached patch patch v.1 (obsolete) — Splinter Review
same patch minus the browser.xul indentation changes.
Attachment #461447 - Attachment is obsolete: true
morphing.
Summary: Implement custom drawn window frame and titlebar for themed and classic desktops → Implement the firefox button on xp, 2k, and aero basic
Summary: Implement the firefox button on xp, 2k, and aero basic → Implement the firefox button on xp, classic, and aero basic
Attached patch patch v.2 (obsolete) — Splinter Review
Attachment #461453 - Attachment is obsolete: true
Attached patch patch v.2 (obsolete) — Splinter Review
Attachment #461732 - Attachment is obsolete: true
Blocks: 576312
Blocks: 582020
Blocks: 582632
Blocks: 578729
Comment on attachment 461733 [details] [diff] [review]
patch v.2

Dao, one open question I have on this is how to best handle RTL operating systems. I added :

#titlebar:-moz-locale-dir(rtl) {
  -moz-transform: scaleX(-1);
}

But I don't think that's the right way to do this. The elements in the titlebar had padding assigned to them via moz css styles (bug 574454) which switch out depending on the rtl state of the os. So just flipping the titlebar isn't right. Curious if you could offer some feedback on how that might be done along with a review of the other changes.
Attachment #461733 - Flags: review?(dao)
Found this screen cap of a rtl window on the web:

http://www.microsoft.com/msj/0499/multilangunicode/multilangfig05.gif

Maybe the mirroring is correct, and I should not be flipping button padding in the patch in bug 574454? Looks like approach would work as well.
Comment on attachment 461733 [details] [diff] [review]
patch v.2

use WINSTRIPE_AERO or browser-aero.css instead of [os=foo]

I don't think you need to do anything for rtl, it should work automatically.

Shouldn't the windowdragbox binding be applied on #titlebar rather than #titlebar-spacer?
Attachment #461733 - Flags: review?(dao) → review-
(In reply to comment #11)
> Comment on attachment 461733 [details] [diff] [review]
> patch v.2
> 
> use WINSTRIPE_AERO or browser-aero.css instead of [os=foo]
> 

updated.

> I don't think you need to do anything for rtl, it should work automatically.

Ok, I'll remove the rtl css and update the patch in bug 574454.

> 
> Shouldn't the windowdragbox binding be applied on #titlebar rather than
> #titlebar-spacer?

Yes, that was misplaced. Updated.
(In reply to comment #12)
> > 
> > Shouldn't the windowdragbox binding be applied on #titlebar rather than
> > #titlebar-spacer?
> 
> Yes, that was misplaced. Updated.

Hmm, actually, if I place that up in #titlebar, it robs the caption buttons of mouse events. I'll try to track down the cause.
Attached patch patch v.3 (obsolete) — Splinter Review
Attachment #461733 - Attachment is obsolete: true
Attached patch patch v.3 (obsolete) — Splinter Review
Ok, this addresses all the comments. The caption buttons are now xul buttons so they get mouse input.
Attachment #462066 - Attachment is obsolete: true
Attachment #462067 - Flags: review?(dao)
Comment on attachment 462067 [details] [diff] [review]
patch v.3

>+    document.getElementById("titlebar").hidden = false;
>+    document.getElementById("titlebar-buttonbox").hidden = false;
>+  } else {
>     document.documentElement.removeAttribute("chromemargin");
>+    document.getElementById("titlebar").hidden = true;
>+    document.getElementById("titlebar-buttonbox").hidden = true;
>+  }

Why does titlebar-buttonbox get hidden separately when titlebar is already hidden?

>+function updateTitlebarButtonDisplay() {
>+#ifdef XP_WIN
>+  /* must be updated after the window is visible for vista and up */

Why?
(In reply to comment #16)
> Comment on attachment 462067 [details] [diff] [review]
> patch v.3
> 
> >+    document.getElementById("titlebar").hidden = false;
> >+    document.getElementById("titlebar-buttonbox").hidden = false;
> >+  } else {
> >     document.documentElement.removeAttribute("chromemargin");
> >+    document.getElementById("titlebar").hidden = true;
> >+    document.getElementById("titlebar-buttonbox").hidden = true;
> >+  }
> 
> Why does titlebar-buttonbox get hidden separately when titlebar is already
> hidden?
> 
> >+function updateTitlebarButtonDisplay() {
> >+#ifdef XP_WIN
> >+  /* must be updated after the window is visible for vista and up */
> 
> Why?

This is due to an issue with windows in retrieving information regarding the proper placement & size of caption buttons. On vista and up, the only way to get this info is to query a window with these buttons via a windows message. To get accurate data the window has to be visible when you query it.

Layout queries the theme code for minimum widget size and padding values before the main window is displayed, unless the element in question is collapsed. So I kept these buttons in that state until delayedStartup executes and the main window is visible which insures the data is available.

(I tried doing this in a pageshow handler for main-window as well, but pageshow didn't fire.) 

Another possible fix here might be to trigger a reflow or somehow tell layout to requery for these values once the data is available, maybe through a theme changed event sent from widget. That would probably cause a perf regression though. So I went with the collapsed trick instead.
delayedStartup doesn't have much to do with window visibility, it's just an arbitrary point after the load event (which I think would fire after pageshow if pageshow was fired at all).
(In reply to comment #18)
> delayedStartup doesn't have much to do with window visibility, it's just an
> arbitrary point after the load event (which I think would fire after pageshow
> if pageshow was fired at all).

Alright, rather than that, the 'MozAfterPaint' seems to work since the first paint on the window doesn't arrive until after the initial show. Will post a new patch.
Attached patch patch v.4 (obsolete) — Splinter Review
Attachment #462067 - Attachment is obsolete: true
Attachment #462067 - Flags: review?(dao)
Comment on attachment 462391 [details] [diff] [review]
patch v.4

>+#ifdef MENUBAR_CAN_AUTOHIDE
>+  window.addEventListener("MozAfterPaint", updateTitlebarButtonDisplay, false);
>+#endif
...
>+function updateTitlebarButtonDisplay() {
>+  window.removeEventListener("MozAfterPaint", updateTitlebarButtonDisplay, false);
>+  /* must be updated after the window is visible for vista and up */
>+  document.getElementById("titlebar-buttonbox").style.visibility = "visible";
>+}

Doesn't look like updateTitlebarButtonDisplay needs to be global. Can you declare it inline?

>-  if (displayAppButton)
>+  if (displayAppButton) {
>     document.documentElement.setAttribute("chromemargin", "0,-1,-1,-1");
>-  else
>+    document.getElementById("titlebar").hidden = false;
>+    document.getElementById("titlebar-buttonbox").hidden = false;
>+  } else {
>     document.documentElement.removeAttribute("chromemargin");
>+    document.getElementById("titlebar").hidden = true;
>+    document.getElementById("titlebar-buttonbox").hidden = true;
>+  }

Same question as before, why is #titlebar-buttonbox explicitly hidden/shown when #titlebar already has the same state?

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

>+/* aesthetic - push the fx button off the top window border */
>+#main-window[sizemode="normal"] > #titlebar > #titlebar-content > #appmenu-button-container {
>+  margin-top: 2px;
>+}

Is this dependent on the OS theme?

>+#titlebar {
>+  -moz-appearance:-moz-window-titlebar;
>+  /* we only need to the middle section, hide the edges of the
>+  theme background beyond the window frame. */
>+  margin-left: -15px;
>+  margin-right: -15px;
>+  visibility: visible;
>+  -moz-binding: url("chrome://global/content/bindings/general.xml#windowdragbox");
>+}

visibility: visible; looks redundant here

>+#titlebar-buttonbox {
>+  -moz-appearance: -moz-window-button-box;
>+  -moz-box-align: start;
>+  /* will be updated after the window is visible on vista and up */
>+  visibility: collapse;
>+}

Can the visibility be handled completely in app code? Can you just use the 'collapse' attribute?

>+#titlebar[os="xp"] > #titlebar-content > titlebar-buttonbox {
>+  visibility: visible;
>+}

This is obsolete.

>+#titlebar-close {
>+  -moz-appearance: -moz-window-button-close;
>+  margin-left: 1px;
>+  margin-right: 0px;
>+}
...
>+/* windows classic titlebar treatment */
...
>+  #titlebar-min {
>+    margin-right: 0px !important;
>+  }
>+
>+  #titlebar-max {
>+    margin-left: 0px !important;
>+  }
>+}

-moz-margin-end / -moz-margin-start?
(In reply to comment #21)
> Comment on attachment 462391 [details] [diff] [review]
> patch v.4
> 
> >+#ifdef MENUBAR_CAN_AUTOHIDE
> >+  window.addEventListener("MozAfterPaint", updateTitlebarButtonDisplay, false);
> >+#endif
> ...
> >+function updateTitlebarButtonDisplay() {
> >+  window.removeEventListener("MozAfterPaint", updateTitlebarButtonDisplay, false);
> >+  /* must be updated after the window is visible for vista and up */
> >+  document.getElementById("titlebar-buttonbox").style.visibility = "visible";
> >+}
> 
> Doesn't look like updateTitlebarButtonDisplay needs to be global. Can you
> declare it inline?

Updated.

> >+    document.getElementById("titlebar-buttonbox").hidden = false;
> 
> Same question as before, why is #titlebar-buttonbox explicitly hidden/shown
> when #titlebar already has the same state?

Sorry, didn't update this. That's left over code from testing buttons display, looks like it can come out.

> 
> >--- a/browser/themes/winstripe/browser/browser-aero.css
> >+++ b/browser/themes/winstripe/browser/browser-aero.css
> 
> >+/* aesthetic - push the fx button off the top window border */
> >+#main-window[sizemode="normal"] > #titlebar > #titlebar-content > #appmenu-button-container {
> >+  margin-top: 2px;
> >+}
> 
> Is this dependent on the OS theme?

More the os really, it has to do with the way the fx button hangs on a particular window frame. So with glass/aero basic, IMHO 2px looks "right", with xp's themes, 1 px looks "right", and with classic, 0px looks right. If you want I can take this out and you can tweak these settings yourself. As the comment states, it's purely aesthetic and based on my own perception of how things should look. (I believe the 2px offset though was already there for aero desktops.)

> 
> >+#titlebar {
> >+  -moz-appearance:-moz-window-titlebar;
> >+  /* we only need to the middle section, hide the edges of the
> >+  theme background beyond the window frame. */
> >+  margin-left: -15px;
> >+  margin-right: -15px;
> >+  visibility: visible;
> >+  -moz-binding: url("chrome://global/content/bindings/general.xml#windowdragbox");
> >+}
> 
> visibility: visible; looks redundant here

removed.

> 
> >+#titlebar-buttonbox {
> >+  -moz-appearance: -moz-window-button-box;
> >+  -moz-box-align: start;
> >+  /* will be updated after the window is visible on vista and up */
> >+  visibility: collapse;
> >+}
> 
> Can the visibility be handled completely in app code? Can you just use the
> 'collapse' attribute?

will try that and see.

> 
> >+#titlebar[os="xp"] > #titlebar-content > titlebar-buttonbox {
> >+  visibility: visible;
> >+}
> 
> This is obsolete.

nixed.

> 
> >+#titlebar-close {
> >+  -moz-appearance: -moz-window-button-close;
> >+  margin-left: 1px;
> >+  margin-right: 0px;
> >+}
> ...
> >+/* windows classic titlebar treatment */
> ...
> >+  #titlebar-min {
> >+    margin-right: 0px !important;
> >+  }
> >+
> >+  #titlebar-max {
> >+    margin-left: 0px !important;
> >+  }
> >+}
> 
> -moz-margin-end / -moz-margin-start?

That's what I was looking for to solve the rtl problem! thanks. Will update and test.
Attached patch patch v.5 (obsolete) — Splinter Review
Attachment #462391 - Attachment is obsolete: true
Comment on attachment 462434 [details] [diff] [review]
patch v.5

Mostly nits:

>+#ifdef MENUBAR_CAN_AUTOHIDE
>+  // update the visibility of the titlebar buttons after the window is
>+  // displayed. (required by theme code.)
>+  window.addEventListener("MozAfterPaint", function () {
>+      window.removeEventListener("MozAfterPaint", arguments.callee, false);
>+      document.getElementById("titlebar-buttonbox").removeAttribute("collapsed");
>+    }, false);

s/removeAttribute("collapsed")/collapsed = false/

reduce the indentation of the last three lines by 2 spaces each

>   document.getElementById("appmenu-button-container").hidden = !displayAppButton;
> 
>-  if (displayAppButton)
>+  if (displayAppButton) {
>     document.documentElement.setAttribute("chromemargin", "0,-1,-1,-1");
>-  else
>+    document.getElementById("titlebar").hidden = false;
>+  } else {
>     document.documentElement.removeAttribute("chromemargin");
>+    document.getElementById("titlebar").hidden = true;
>+  }

document.getElementById("appmenu-button-container").hidden also seems redundant now.

By the way, can we get rid of appmenu-button-container in favor of titlebar-content?

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

>+/* aesthetic - push the fx button off the top window border */
>+#main-window[sizemode="normal"] > #titlebar > #titlebar-content > #appmenu-button-container {
>+  margin-top: 2px;
>+}

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

>+/* aesthetic - push the fx button off the top window border */
>+%ifndef WINSTRIPE_AERO
>+#main-window[sizemode="normal"] > #titlebar > #titlebar-content > #appmenu-button-container {
>+  margin-top: 1px;
> }
> %endif

You could actually avoid browser-aero.css here and do this:

foo {
%ifdef WINSTRIPE_AERO
  margin-top: 2px;
%else
  margin-top: 1px;
%endif
}

>+@media all and (-moz-windows-classic) {
>+  #main-window[sizemode="normal"] > #titlebar > #titlebar-content > #appmenu-button-container {
>+    margin-top: 0px !important;
>+  }
>+
>+  #titlebar-close {
>+    min-width: 0px !important;
>+  }
>+
>+  #titlebar-min {
>+    -moz-margin-end: 0px !important;
>+    min-width: 0px !important;
>+  }
>+
>+  #titlebar-max {
>+    -moz-margin-start: 0px !important;
>+    min-width: 0px !important;
>+  }
>+}

s/0px/0/

Isn't the default button min-width always wrong, not just for classic?

Does your patch put the window controls in the tab order?
(In reply to comment #24)
> Comment on attachment 462434 [details] [diff] [review]
> 
> By the way, can we get rid of appmenu-button-container in favor of
> titlebar-content?
> 

The container seems to be what's keeping the fx button from stretching the height of titlebar-content, and align="start" start doesn't have any effect on button.(Will play with it a bit to see.)

This brings up a question I had, what platforms have MENUBAR_CAN_AUTOHIDE defined? Related to that, should I remove this css:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.css#89

or maybe move this css in my patch for winstripe:

#main-window[inFullscreen="true"] > #titlebar {..

over to base/content/browser.css?
> Isn't the default button min-width always wrong, not just for classic?

Hmm, checking in dom inspector, it is actually. But the only place it seems to have an effect on the button widths is on classic. *shrug* I'll move these up though.
(In reply to comment #25)
> This brings up a question I had, what platforms have MENUBAR_CAN_AUTOHIDE
> defined?

Only Windows right now.

> Related to that, should I remove this css:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.css#89

Yes, it doesn't apply anymore.

> or maybe move this css in my patch for winstripe:
> 
> #main-window[inFullscreen="true"] > #titlebar {..
> 
> over to base/content/browser.css?

Yes.
Attached image stretch
(In reply to comment #25)
> (In reply to comment #24)
> The container seems to be what's keeping the fx button from stretching the
> height of titlebar-content, and align="start" start doesn't have any effect on
> button.(Will play with it a bit to see.)

We may want to stretch this actually, maybe to 50% or so, now that customizations are working right. I'll file a follow up.
Attached patch patch v.6 (obsolete) — Splinter Review
> Does your patch put the window controls in the tab order?

AFAICT from testing, no it doesn't. This seems to mimic the behavior when windows is handling this area. (I can't tab to the buttons, the focus moves from the tab to the content.)  

Other nits addressed.
Attachment #462434 - Attachment is obsolete: true
Attachment #462873 - Flags: review?(dao)
I'll have try builds posted here in a few hours.
Attached patch patch v.6 (obsolete) — Splinter Review
Whoops, removed the moz-user-input junk.
Attachment #462873 - Attachment is obsolete: true
Attachment #462988 - Flags: review?(dao)
Attachment #462873 - Flags: review?(dao)
FYI to drivers, this should block beta 4, not beta N.
Tryserver builds are available and I tested one. Great! Is there another bug for putting tabs into titlebar when the window is maximized or will that land with this bug?
Bug 572160
Can you show me link to tryserver build?
Attached image xp - zune
Honestly, I'm not a huge fan of the way the fx button looks on xp themes. It just doesn't seem to fit right. There's plenty of time to tweak the appearance once these patches land though. (shorlander, does this match up with what you have in the mockups?)
Attached image xp - standard
Attached image xp - classic
(In reply to comment #36)

> Honestly, I'm not a huge fan of the way the fx button looks on xp themes. It
> just doesn't seem to fit right. There's plenty of time to tweak the appearance
> once these patches land though. (shorlander, does this match up with what you
> have in the mockups?)

The plan is for the Firefox button to have a more XPish look that integrates with the custom window frame and buttons as seen here: https://wiki.mozilla.org/Firefox/4.0_Windows_Theme_Mockups#Windows_XP
Attached image win7 - aero basic
blocking2.0: betaN+ → beta4+
(In reply to comment #35)
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/jmathies@mozilla.com-719ed62bd8ac/

This actually appears to have focusable window controls. Maybe toolbarbuttons instead of buttons would work? Otherwise style="-moz-user-focus: ignore;" will probably do the job.
Comment on attachment 462988 [details] [diff] [review]
patch v.6

>-  document.getElementById("appmenu-button-container").hidden = !displayAppButton;
>-
>-  if (displayAppButton)
>+  if (displayAppButton) {
>     document.documentElement.setAttribute("chromemargin", "0,-1,-1,-1");
>-  else
>+    document.getElementById("titlebar").hidden = false;
>+  } else {
>     document.documentElement.removeAttribute("chromemargin");
>+    document.getElementById("titlebar").hidden = true;
>+  }

Less code churn if you just replace appmenu-button-container with titlebar.

> #appmenu-button-container {
>   -moz-binding: url("chrome://global/content/bindings/general.xml#windowdragbox");
> }

Remove this?

>+#titlebar {
>+  -moz-appearance:-moz-window-titlebar;

nit: space after colon

>+  /* we only need to the middle section, hide the edges of the
>+  theme background beyond the window frame. */
>+  margin-left: -15px;
>+  margin-right: -15px;
>+  -moz-binding: url("chrome://global/content/bindings/general.xml#windowdragbox");

The last line should probably move to browser/base/content/browser.css.

>+#titlebar-min {
>+  -moz-appearance: -moz-window-button-minimize;
>+  -moz-margin-start: 1px;
>+  -moz-margin-end: 1px;
>+  min-width: 0 !important;
>+}
>+
>+#titlebar-max {
>+  -moz-appearance: -moz-window-button-maximize;
>+  -moz-margin-start: 1px;
>+  -moz-margin-end: 1px;
>+  min-width: 0 !important;
>+}

>+#titlebar-close {
>+  -moz-appearance: -moz-window-button-close;
>+  -moz-margin-start: 1px;
>+  -moz-margin-end: 0;
>+  min-width: 0 !important;
>+}

!important shouldn't be necessary here. (You can remove min-width altogether when using toolbarbuttons.)

>+@media all and (-moz-windows-classic) {
>+  #main-window[sizemode="normal"] > #titlebar > #titlebar-content > #appmenu-button-container {
>+    margin-top: 0 !important;
>   }

Wrapping the part which added the margin-top with "@media not all and (-moz-windows-classic) {...}" (mind the "not") might be better.
Attached patch patch v.7Splinter Review
updated per comments.
Attachment #462988 - Attachment is obsolete: true
Attachment #463329 - Flags: review?(dao)
Attachment #462988 - Flags: review?(dao)
Comment on attachment 463329 [details] [diff] [review]
patch v.7

>+  if (window.windowState == window.STATE_MAXIMIZED) {
>+    window.restore();
>+  } else {
>+    window.maximize();
>+  }

nit: preferred style in this file is to omit the braces

>+#titlebar-min {
>+  -moz-appearance: -moz-window-button-minimize;
>+  -moz-margin-start: 1px;
>+  -moz-margin-end: 1px;
>+}
>+
>+#titlebar-max {
>+  -moz-appearance: -moz-window-button-maximize;
>+  -moz-margin-start: 1px;
>+  -moz-margin-end: 1px;
>+}
>+
>+#main-window[sizemode="maximized"] > #titlebar > #titlebar-content > #titlebar-buttonbox > #titlebar-max {
>+  -moz-appearance: -moz-window-button-restore;
>+  -moz-margin-start: 1px;
>+  -moz-margin-end: 1px;
>+}
>+
>+#titlebar-close {
>+  -moz-appearance: -moz-window-button-close;
>+  -moz-margin-start: 1px;
>+  -moz-margin-end: 0;
>+}
>+
>+/* windows classic titlebar treatment */
>+
>+@media all and (-moz-windows-classic) {
>+  #titlebar-min {
>+    -moz-margin-end: 0px !important;
>   }
>-  #navigator-toolbox[tabsontop="true"] > #toolbar-menubar[autohide="true"] ~ #TabsToolbar:not([inFullscreen]) {
>-    -moz-padding-start: 10em !important;
>+
>+  #titlebar-max {
>+    -moz-margin-start: 0px !important;
>   }
>-%ifdef WINSTRIPE_AERO
> }
>-%endif

Toolbarbuttons don't have a margin by default, which means that you should be able to remove the -moz-margin-*: 0 instances and wrap the 1px stuff to exclude classic like this:

@media not all and (-moz-windows-classic) {
  #titlebar-min,
  #titlebar-max {
    -moz-margin-start: 1px;
    -moz-margin-end: 1px;
  }

  #titlebar-close {
    -moz-margin-start: 1px;
  }
}

And if you add up the 1px margins you can simplify it further:

@media not all and (-moz-windows-classic) {
  #titlebar-min,
  #titlebar-max {
    -moz-margin-end: 2px;
  }
}
Attachment #463329 - Flags: review?(dao) → review+
Using that tryserver build linked a few comments back, Minefield's titlebar is taller than normal when on Windows 7 with Aero enabled. Is it using XP's titlebar height for all platforms?
Left side is Maximized mode, Right is a Normal window size.  The gap between fx button and tabs toolbar vary.
Blocks: 584955
Attached image normal w/notepad
The new titlebar "parts" on aero don't paint, but they are still present and take up space in the layout. The space is determined by system settings. The old titlebar didn't respect titlebar height metrics, the new one does. Attached is a comparison of the normal window before and after w/notepad. (If you max the browser and a notepad window and flip back and forth you'll see maximized state is also correct.) This is why these patch will also fix bug 576960, bug 578729, bug 581023, bug 582020, etc..
Attached image padding
There is a little bit of padding on top of the tabs we might take out. The bottom border, margin, and padding on the #titlebar and top border, margin, and padding on #navigator-toolbar are all zero though.

Dao any ideas on where this extra pixel or two of padding is coming from?
(In reply to comment #48)
> Created attachment 463563 [details]
> padding
> 
> There is a little bit of padding on top of the tabs we might take out. The
> bottom border, margin, and padding on the #titlebar and top border, margin, and
> padding on #navigator-toolbar are all zero though.
> 
> Dao any ideas on where this extra pixel or two of padding is coming from?

What exactly is the bottom red line? Is the top edge or the bottom edge of that line or the area in between of interest?
(In reply to comment #49)
> (In reply to comment #48)
> > Created attachment 463563 [details] [details]
> > padding
> > 
> > There is a little bit of padding on top of the tabs we might take out. The
> > bottom border, margin, and padding on the #titlebar and top border, margin, and
> > padding on #navigator-toolbar are all zero though.
> > 
> > Dao any ideas on where this extra pixel or two of padding is coming from?
> 
> What exactly is the bottom red line? Is the top edge or the bottom edge of that
> line or the area in between of interest?

In dom inspector I highlighted #titlbar and snapped a screen shot.
Looks like the padding below that is in question. If I highlight #navigator-toolbar seems like there's a small amount of space between the two boundaries.
This means that tabs have a one pixel padding at the top, which is expected... It's a very faint shadow, not quite transparent.
(In reply to comment #51)
> Looks like the padding below that is in question. If I highlight
> #navigator-toolbar seems like there's a small amount of space between the two
> boundaries.

Sorry -> '#navigator-toolbox' not '#navigator-toolbar'.

(In reply to comment #52)
> This means that tabs have a one pixel padding at the top, which is expected...
> It's a very faint shadow, not quite transparent.

That would explain it. I'll test with tabs on bottom as well.
Attached image tabsonbottom w/notepad
The shadow looks to be the cause. With tabs on bottom, we line up with notepad perfectly.
(In reply to comment #44)
> Comment on attachment 463329 [details] [diff] [review]
> patch v.7
> 
> >+  if (window.windowState == window.STATE_MAXIMIZED) {
> >+    window.restore();
> >+  } else {
> >+    window.maximize();
> >+  }
> 
> nit: preferred style in this file is to omit the braces
> 
> >+#titlebar-min {
> >+  -moz-appearance: -moz-window-button-minimize;
> >+  -moz-margin-start: 1px;
> >+  -moz-margin-end: 1px;
> >+}
> >+
> >+#titlebar-max {
> >+  -moz-appearance: -moz-window-button-maximize;
> >+  -moz-margin-start: 1px;
> >+  -moz-margin-end: 1px;
> >+}
> >+
> >+#main-window[sizemode="maximized"] > #titlebar > #titlebar-content > #titlebar-buttonbox > #titlebar-max {
> >+  -moz-appearance: -moz-window-button-restore;
> >+  -moz-margin-start: 1px;
> >+  -moz-margin-end: 1px;
> >+}
> >+
> >+#titlebar-close {
> >+  -moz-appearance: -moz-window-button-close;
> >+  -moz-margin-start: 1px;
> >+  -moz-margin-end: 0;
> >+}
> >+
> >+/* windows classic titlebar treatment */
> >+
> >+@media all and (-moz-windows-classic) {
> >+  #titlebar-min {
> >+    -moz-margin-end: 0px !important;
> >   }
> >-  #navigator-toolbox[tabsontop="true"] > #toolbar-menubar[autohide="true"] ~ #TabsToolbar:not([inFullscreen]) {
> >-    -moz-padding-start: 10em !important;
> >+
> >+  #titlebar-max {
> >+    -moz-margin-start: 0px !important;
> >   }
> >-%ifdef WINSTRIPE_AERO
> > }
> >-%endif
> 
> Toolbarbuttons don't have a margin by default, which means that you should be
> able to remove the -moz-margin-*: 0 instances and wrap the 1px stuff to exclude
> classic like this:
> 
> @media not all and (-moz-windows-classic) {
>   #titlebar-min,
>   #titlebar-max {
>     -moz-margin-start: 1px;
>     -moz-margin-end: 1px;
>   }
> 
>   #titlebar-close {
>     -moz-margin-start: 1px;
>   }
> }
> 
> And if you add up the 1px margins you can simplify it further:
> 
> @media not all and (-moz-windows-classic) {
>   #titlebar-min,
>   #titlebar-max {
>     -moz-margin-end: 2px;
>   }
> }

Trying to coalesce all this together. I'm not sure how I arrived at a -moz-margin-start for the min button though. I think that needs to come out.
The positions of close/min/max buttons are not right in non native theme for xp
(In reply to comment #56)
> Created attachment 463752 [details]
> The positions of close/min/max buttons are not right in non native theme for xp
> 
> The positions of close/min/max buttons are not right in non native theme for xp

Please file a new bug on this, after the patches land.
http://hg.mozilla.org/mozilla-central/rev/a0d6e4d37273
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Attached image screenshot
need more height on XP ?
(In reply to comment #59)
> Created attachment 464286 [details]
> screenshot
> 
> need more height on XP ?

Possibly, I would post that in bug 574681.
(In reply to comment #60)
> (In reply to comment #59)
> > Created attachment 464286 [details] [details]
> > screenshot
> > 
> > need more height on XP ?
> 
> Possibly, I would post that in bug 574681.

OK, thanks.
when Firefox Button on by default for Windows XP, classic, and aero basic
like bug 574435 ?
(In reply to comment #62)
> when Firefox Button on by default for Windows XP

Never, as far as I remember.
Jim, any reason why you did this:

@media not all and (-moz-windows-classic) {
  #titlebar-min {
    -moz-margin-end: 1px;
  }

  #titlebar-max {
    -moz-margin-start: 1px;
    -moz-margin-end: 1px;
  }

  #titlebar-close {
    -moz-margin-start: 1px;
    -moz-margin-end: 0;
  }
}

instead of:

@media not all and (-moz-windows-classic) {
  #titlebar-min,
  #titlebar-max {
    -moz-margin-end: 2px;
  }
}

?
Depends on: 585924
(In reply to comment #64)
> Jim, any reason why you did this:
> 
> @media not all and (-moz-windows-classic) {
>   #titlebar-min {
>     -moz-margin-end: 1px;
>   }
> 
>   #titlebar-max {
>     -moz-margin-start: 1px;
>     -moz-margin-end: 1px;
>   }
> 
>   #titlebar-close {
>     -moz-margin-start: 1px;
>     -moz-margin-end: 0;
>   }
> }
> 
> instead of:
> 
> @media not all and (-moz-windows-classic) {
>   #titlebar-min,
>   #titlebar-max {
>     -moz-margin-end: 2px;
>   }
> }
> 
> ?

I can post a patch. Shouldn't each button have it's own padding or does it make any difference?
The outcome should be identical.
This was backed out for the following performance regression:
Talos Regression: Txul increase 4.57% on Win7 Firefox

Other possible culprits:
bug 574454

http://hg.mozilla.org/mozilla-central/rev/9165ca3607bd
http://hg.mozilla.org/mozilla-central/rev/5704ef94d87f
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 577197
No longer blocks: 577197
Blocks: 575036
When this was backed-out the window title bar is now no longer available (on WinXP SP3 x86). With tabs-on-top you can't grab anything to move the window. With tabs on the bottom (or on top) the window controls (minimize, resize/fullscreen, close) are not there. Must close browser with Minefield button>Exit or Alt-F4.
Pic available at
http://www.flickr.com/photos/9086763@N06/4883464958/
Yes, kind of my thinking as well.  If this is too remain backed out, then perhaps bug 574454needs to be backed out also otherwise if you have the browser set to autohide the taskbar, you end up with kind of a non-functional window.
(In reply to comment #68)
> When this was backed-out the window title bar is now no longer available (on
> WinXP SP3 x86). With tabs-on-top you can't grab anything to move the window.
> With tabs on the bottom (or on top) the window controls (minimize,
> resize/fullscreen, close) are not there. Must close browser with Minefield
> button>Exit or Alt-F4.
> Pic available at
> http://www.flickr.com/photos/9086763@N06/4883464958/

It is actually possible to move the window, athough fairly convoluted.  You have to right click on the taskbar icon and select move, then press an arrow key to move the window in any direction.  once you do that the mouse can be used to complete the move of the window to the desired location.
(In reply to comment #69)
> Yes, kind of my thinking as well.  If this is too remain backed out, then
> perhaps bug 574454needs to be backed out also otherwise if you have the browser
> set to autohide the taskbar, you end up with kind of a non-functional window.

Hmm, we also removed compsitor checks to get ready for this landing in bug 574454. I think the only change set that needs to come out is:

http://hg.mozilla.org/mozilla-central/rev/05dde680ade2

I really don't have the bandwidth to do this and watch the tree today. Shawn, any chance you might be able to back this additional patch out?
No longer blocks: 582020
Blocks: 582231
Blocks: 587303
(In reply to comment #71)
> (In reply to comment #69)
> > Yes, kind of my thinking as well.  If this is too remain backed out, then
> > perhaps bug 574454needs to be backed out also otherwise if you have the browser
> > set to autohide the taskbar, you end up with kind of a non-functional window.
> 
> Hmm, we also removed compsitor checks to get ready for this landing in bug
> 574454. I think the only change set that needs to come out is:
> 
> http://hg.mozilla.org/mozilla-central/rev/05dde680ade2
> 
> I really don't have the bandwidth to do this and watch the tree today. Shawn,
> any chance you might be able to back this additional patch out?

done:

http://hg.mozilla.org/mozilla-central/rev/391e102eb863
http://hg.mozilla.org/mozilla-central/rev/cfeb77de159c
I take it this has no chance of making beta4?
blocking2.0: beta4+ → beta5+
Blocks: 586228
No longer blocks: 586228
(In reply to comment #67)
> This was backed out for the following performance regression:
> Talos Regression: Txul increase 4.57% on Win7 Firefox
> 
> Other possible culprits:
> bug 574454
> 
> http://hg.mozilla.org/mozilla-central/rev/9165ca3607bd
> http://hg.mozilla.org/mozilla-central/rev/5704ef94d87f

Took some time today to get txul running locally to try and track this down. Oddly enough though I'm actually getting slightly better results locally with these patches applied. The difference is pretty minor, about 3 msec on my machine.

Will do some more testing tomorrow and see what I can come up with.
(In reply to comment #74)
> Will do some more testing tomorrow and see what I can come up with.

Can we use tryserver and mconnor's compare Talos tool at http://perf.snarkfest.net/compare-talos/
(In reply to comment #75)
> (In reply to comment #74)
> > Will do some more testing tomorrow and see what I can come up with.
> 
> Can we use tryserver and mconnor's compare Talos tool at
> http://perf.snarkfest.net/compare-talos/

I can use try to confirm a fix, but not search for a fix. The lag on results is too high.
In any event, the patch has now bit-rotted.
Attached patch patches (obsolete) — Splinter Review
If you're interested in playing with it, here's the two that were backed out, updated to latest trunk.
So my first guess is this has to do with the clipped titlebar background image rendered on themed desktops:

+#titlebar {
+  -moz-appearance: -moz-window-titlebar;
+  /* we only need to the middle section, hide the edges of the
+  theme background beyond the window frame. */
+  margin-left: -15px;
+  margin-right: -15px;
+}

The reasoning for this is that we only want to render the middle section of the background, so the edges are pushed outside the rendering rectangle to clip them out.

Unfortunately this doesn't explain why xp *didn't* see the ~5msec increase. Our xp talos slaves use the default xp theme, which is no different than aero basic.

I'm still stumped.
Attached patch patches v2Splinter Review
That patch did not really fix the issue of it not applying on current trunk.
Attachment #467780 - Attachment is obsolete: true
(In reply to comment #79)
> So my first guess is this has to do with the clipped titlebar background image
> rendered on themed desktops:
> 
> +#titlebar {
> +  -moz-appearance: -moz-window-titlebar;
> +  /* we only need to the middle section, hide the edges of the
> +  theme background beyond the window frame. */
> +  margin-left: -15px;
> +  margin-right: -15px;
> +}
> 
> The reasoning for this is that we only want to render the middle section of the
> background, so the edges are pushed outside the rendering rectangle to clip
> them out.
> 
> Unfortunately this doesn't explain why xp *didn't* see the ~5msec increase. Our
> xp talos slaves use the default xp theme, which is no different than aero
> basic.
> 
> I'm still stumped.

Could this have something to do with the desktop manager?

I believe that under Vista and Windows 7 even if you are running Aero basic in which case the Desktop Manger adds nothing other than overhead, it still runs.

Of course under Windows/XP it does not exist.
(In reply to comment #81)
> (In reply to comment #79)
> > So my first guess is this has to do with the clipped titlebar background image
> > rendered on themed desktops:
> > 
> > +#titlebar {
> > +  -moz-appearance: -moz-window-titlebar;
> > +  /* we only need to the middle section, hide the edges of the
> > +  theme background beyond the window frame. */
> > +  margin-left: -15px;
> > +  margin-right: -15px;
> > +}
> > 
> > The reasoning for this is that we only want to render the middle section of the
> > background, so the edges are pushed outside the rendering rectangle to clip
> > them out.
> > 
> > Unfortunately this doesn't explain why xp *didn't* see the ~5msec increase. Our
> > xp talos slaves use the default xp theme, which is no different than aero
> > basic.
> > 
> > I'm still stumped.
> 
> Could this have something to do with the desktop manager?
> 
> I believe that under Vista and Windows 7 even if you are running Aero basic in
> which case the Desktop Manger adds nothing other than overhead, it still runs.
> 
> Of course under Windows/XP it does not exist.

I've been running some extended txul runs in vmware, painting of the titlebar doesn't appear to be the issue.
Looks like the likely culprit is the button metrics query we do on Vista and up -

http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsNativeThemeWin.cpp#167

http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsUXThemeData.cpp#273

vmware win7 txul runs:
trunk: 115
minus button query: 117
no painting: 115
fully applied: 121

I'll do some timing tests on these calls to see. If this is the case, there may not be much we can do about it. We need to make this call on Vista and up to get the right button dimensions.
(In reply to comment #83)
> Looks like the likely culprit is the button metrics query we do on Vista and up
> -

Actually it's the changes to layout that takes place when we switch the titlebar buttons from collapsed to visible. The titlebar message we send to get the right metrics isn't the problem.

This regression did show up on xp according to txul results, it just wasn't as obvious.
Attached patch updates v.1 (obsolete) — Splinter Review
prevent using titlebar info calls on xp and vista/win7 w/aero glass. 

These are some improvements for the original patches, but they don't fix the regression on win7 aero basic, which is what our talos slaves run.
Attached patch catmgr experiment (obsolete) — Splinter Review
Another experiment. This seemed to shave a little time off, but it still didn't run as fast as trunk.
Attachment #463752 - Attachment is obsolete: true
Attached patch win experiment (obsolete) — Splinter Review
bsmedberg suggested something like this. The window that gets created won't display on the desktop, but there's a small risk of it showing up very briefly on the taskbar on slower systems. FWIW, I didn't notice it in my vmware image while running txul.
Attachment #468430 - Flags: feedback?(roc)
Attachment #468497 - Flags: feedback?(roc)
roc, the purpose here is to remove or minimize a 4-5 msec txul regression on aero basic and xp themed desktops. I'm leaning toward the win fix myself since it removed the need for collapsed content. Curious if you have any thoughts.
Comment on attachment 468430 [details] [diff] [review]
catmgr experiment

Been testing a bit, I think I'll go with the win patch.
Attachment #468430 - Attachment is obsolete: true
Attachment #468430 - Flags: feedback?(roc)
Attachment #468497 - Flags: feedback?(roc) → review?(roc)
Attachment #468497 - Flags: review?(dao)
Could you use the hidden window here instead of creating one?

I assume the ShowWindow call is necessary here?

It seems like we should be able to stop it showing up in the taskbar, using the right combination of styles. Dialog boxes don't usually show up in the taskbar, for example.
(In reply to comment #90)
> Could you use the hidden window here instead of creating one?

The hidden message window actually has a bit of code tied to it that prevents it from being shown. (in OnWindowPosChanging) I guess at some point a 3rd party app was messing with it. So I went with my own window/class to avoid any conflict. I could pull or trap that bit of code. Since this new window is created just once at the start of a session, I just went with my own isolated (non-nsWindow subclassed) window.

> 
> I assume the ShowWindow call is necessary here?

Yes, the window has to be set to the visible state or the titlebar metrics aren't correct.

> 
> It seems like we should be able to stop it showing up in the taskbar, using the
> right combination of styles. Dialog boxes don't usually show up in the taskbar,
> for example.

Tried that. I used ws_ex_toolwindow. But the metrics values were different from a standard overlapped window.
(In reply to comment #91)
> (In reply to comment #90)
> > 
> > It seems like we should be able to stop it showing up in the taskbar, using the
> > right combination of styles. Dialog boxes don't usually show up in the taskbar,
> > for example.
> 
> Tried that. I used ws_ex_toolwindow. But the metrics values were different from
> a standard overlapped window.

Looked at this a bit more. It's true some dialogs do not show up, but these are modal children. There might be a way to trick windows on this, although since I have no visible parent, it might not be possible. I'll play with it a bit more tomorrow.
Comment on attachment 468497 [details] [diff] [review]
win experiment

>-    <hbox id="titlebar-buttonbox" collapsed="true">
>+    <hbox id="titlebar-buttonbox" collapsed="false">

nit: remove collapsed="false"

> @media all and (-moz-windows-compositor) {
>+  /* these should be hidden w/glass enabled. windows draws it's own buttons. */
>+  #titlebar-min,
>+  #titlebar-max,
>+  #titlebar-close {
>+    visibility: hidden;
>+  }

Does widget code somehow depend on visibility:hidden or would display:none work as well?
Depends on: 590146
Comment on attachment 468497 [details] [diff] [review]
win experiment

Updated patch coming. By passing a parent and creating a stand alone child, even if the parent is hidden, we can still get the right values. This should fix the taskbar problem.

Dao, I'll update to display:none and put the other button css changes in as well in the follow up.
Attachment #468497 - Flags: review?(roc)
Attachment #468497 - Flags: review?(dao)
Attached patch updates v.2Splinter Review
Attachment #468349 - Attachment is obsolete: true
Attachment #468497 - Attachment is obsolete: true
Attachment #468715 - Flags: review?(roc)
Attachment #468715 - Flags: review?(dao)
Blocks: 590146
No longer depends on: 590146
Comment on attachment 468715 [details] [diff] [review]
updates v.2

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

Just hide #titlebar-buttonbox, unless -moz-appearance: -moz-window-button-box; is doing anything useful here?
Attachment #468715 - Flags: review?(dao) → review+
Comment on attachment 468715 [details] [diff] [review]
updates v.2

It seems crazy that we need to create and show a window to get these metrics, but if Windows gives us no choice, so mote it be!
Attachment #468715 - Flags: review?(roc) → review+
No longer blocks: 590146
http://hg.mozilla.org/mozilla-central/rev/323eb3b2eb2a
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
No longer blocks: 582231
The title doesn't refresh for me with maximized window (Win7 Aero basic, b5pre 20100824 but may already exist in older versions). See screenshot:
http://img714.imageshack.us/img714/9802/ffaerobasic.png
(In reply to comment #100)
> The title doesn't refresh for me with maximized window (Win7 Aero basic, b5pre
> 20100824 but may already exist in older versions). See screenshot:
> http://img714.imageshack.us/img714/9802/ffaerobasic.png

Please check the 08/25 nightly when it's out in a bit. This work just landed late last night.
(In reply to comment #0)
Looking at the mockups it looks like the title bar will no longer display the page name, is that accurate?

Are we meant to have an empty blue space at the top of the window and don't actually see the complete page name if it overflows the tab width unless we hover the pointer on the tab? =S

Even Microsoft Office's "ribbon" shows the document name at the top.
(In reply to comment #102)
> (In reply to comment #0)
> Looking at the mockups it looks like the title bar will no longer display the
> page name, is that accurate?
> 
> Are we meant to have an empty blue space at the top of the window and don't
> actually see the complete page name if it overflows the tab width unless we
> hover the pointer on the tab? =S
> 
> Even Microsoft Office's "ribbon" shows the document name at the top.

I believe your looking for bug 575487.
(In reply to comment #103)
> I believe your looking for bug 575487.
Thanks! But that's sadly already RESOLVED WONTFIX
Bug 583905 seems to be my only hope.
(In reply to comment #101)
> (In reply to comment #100)
> > The title doesn't refresh for me with maximized window (Win7 Aero basic, b5pre
> > 20100824 but may already exist in older versions). See screenshot:
> > http://img714.imageshack.us/img714/9802/ffaerobasic.png
> 
> Please check the 08/25 nightly when it's out in a bit. This work just landed
> late last night.

cannot check.
with 0825 Nightly, titlebar is gone.
Depends on: 590880
No longer depends on: 590880
(In reply to comment #105)
> cannot check.
> with 0825 Nightly, titlebar is gone.

It sounds like you might not be using the default Firefox theme.  The new menu and the Firefox button are currently only supported in the default theme (and maybe a handful of others fi some themers are really on-the-ball keeping up with beta releases).

If you are using a theme other than the Firefox default, please try the default theme.

Other themes should start supporting the new UI features as the release date approaches.
The other thing is that some extensions and userchrome.css changes would likely interfere.  I know of at least one extension in particular that exhibits the behavior pal-moz mentioned.
I meant title is not displayed with Firefox Button ON.(Menu Bar OFF)
I know title is displayed with Firefox Button OFF.(Menu Bar ON)
(In reply to comment #108)
> I meant title is not displayed with Firefox Button ON.(Menu Bar OFF)
> I know title is displayed with Firefox Button OFF.(Menu Bar ON)

Oh you said the titlebar was not displayed.  I guess what you meant was that the page title is no longer displayed int he titlebar.
(In reply to comment #109)
> (In reply to comment #108)
> > I meant title is not displayed with Firefox Button ON.(Menu Bar OFF)
> > I know title is displayed with Firefox Button OFF.(Menu Bar ON)
> 
> Oh you said the titlebar was not displayed.  I guess what you meant was that
> the page title is no longer displayed int he titlebar.

bug 583905.
Target Milestone: --- → Firefox 4.0b5
Depends on: 592185
No longer depends on: 592185
Depends on: 592185
Depends on: 594371
No longer depends on: 594371
No longer blocks: 576960
See Also: → 1619777
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: