610643 - Firefox 4 doesn't render the title bar precisely when maximized in Windows XP

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Law, Cheuk-wing]

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b6) Gecko/20100101 Firefox/4.0b6
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b6) Gecko/20100101 Firefox/4.0b6

The title bar rendered by Firefox is very different from the original Microsoft one.

Reproducible: Always

Steps to Reproduce:
1. Hide the menu bar.
2. Maximize the window.
3. Compare it to other window which contain a title bar painted by Windows XP.(IE8, Firefox 3, Windows Explorer etc.)
Actual Results:  
The title bar painted by Firefox is different.

Expected Results:  
The title bar painted by Firefox is pixel perfect.

A comparison between IE8 and Firefox version 4.0b6 is attached.

Comment 1

[Law, Cheuk-wing]

Attachment: The comparison between IE8 and Firefox

Comment 2

[Jim Mathies [:jimm]]

We seem to be missing a lighter upper border for some reason.

Comment 3

[Dão Gottwald [:dao]]

Also, the left and right sides should be darker. We have front-end code that cuts off these areas, which seems slightly bogus:

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

Comment 4

[Dão Gottwald [:dao]]

FWIW, Opera lacks the upper border as well.

Comment 5

[Jim Mathies [:jimm]]

(In reply to comment #3)
> Also, the left and right sides should be darker. We have front-end code that
> cuts off these areas, which seems slightly bogus:
> 
> 308 #titlebar {
> 309   -moz-appearance: -moz-window-titlebar;
> 310   /* we only need to the middle section, hide the edges of the
> 311   theme background beyond the window frame. */
> 312   margin-left: -15px;
> 313   margin-right: -15px;
> 314 }

Unfortunately that's unavoidable. The win theme lib doesn't offer a titlebar inner section background. We have to render the whole thing including edges. Since we only want the inner part, we have to push the edged out of view.

We might be able to compensate here by decreasing the margin values so borders are off screen but the immediate border area we want is visible.

Comment 6

[Jim Mathies [:jimm]]

(In reply to comment #3)
> 312   margin-left: -15px;
> 313   margin-right: -15px;

Remove those two on aero basic and you'll see the problem.

Comment 7

[Jim Mathies [:jimm]]

We might be able to do the right clipping down in the theme lib as well. We have more info down there on the os/desktop settings. I'll see what I can work up.

Comment 8

[Jim Mathies [:jimm]]

Attachment: top border fix

This addresses the missing top border when the window is maximized.

Comment 9

[Jim Mathies [:jimm]]

requesting blocking so the patches here can land.

Comment 10

[Jim Mathies [:jimm]]

Attachment: corners v.1

Comment 11

[Jim Mathies [:jimm]]

(In reply to comment #10)
> Created attachment 490405 [details] [diff] [review]
> corners v.1

This addresses the problem, but it has the funny side effect of squishing all the titlebar content in. Dao, any ideas? I'll attach a screen shot.

Comment 12

[Jim Mathies [:jimm]]

Attachment: titlebar squished

Comment 13

[Jim Mathies [:jimm]]

(In reply to comment #11)
> (In reply to comment #10)
> > Created attachment 490405 [details] [diff] [review] [details]
> > corners v.1
> 
> This addresses the problem, but it has the funny side effect of squishing all
> the titlebar content in. Dao, any ideas? I'll attach a screen shot.

doh! right below it.

Comment 14

[Jim Mathies [:jimm]]

Attachment: corners

Comment 15

[Jim Mathies [:jimm]]

Attachment: classic edges

with the other changes, this fixes maximized windows in classic mode - remove the edges and paint the gradient across the entire titlebar.

Comment 16

[Jim Mathies [:jimm]]

Attachment: corners

no need for the zero margins, they should reset on their own.

Comment 17

[Jim Mathies [:jimm]]

Attachment: classic mode close button padding

fix for classic mode, the buttons are inset by one additional pixel.

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 490410 [details] [diff] [review]
corners

>-#titlebar {
>+#main-window[sizemode="normal"] > #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;
> }

This is actually a bit off for sizemode=normal too. Can we decrease the values or handle this in widget code as suggested earlier?

Comment 19

[Jim Mathies [:jimm]]

(In reply to comment #18)
> Comment on attachment 490410 [details] [diff] [review]
> corners
> 
> >-#titlebar {
> >+#main-window[sizemode="normal"] > #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;
> > }
> 
> This is actually a bit off for sizemode=normal too. Can we decrease the values
> or handle this in widget code as suggested earlier?

On which theme are you seeing an issue?

I don't think we want to do this down in widget because if we ever get around to implementing bug 590945, we're going to need the corners. (Alternatively I guess we could create matching titlebar corner styles, but that is probably more hassle than just doing the stretching here for now.)

Comment 20

[Dão Gottwald [:dao]]

XP Luna. It looks right if I change the margins to -4px.

Comment 21

[Jim Mathies [:jimm]]

(In reply to comment #20)
> XP Luna. It looks right if I change the margins to -4px.

Hmm, that value needs to be equal to the border frame width, which changes based on the theme and user customizations.

I'm not see anything wrong though on Luna, can you post a screen shot of the problem?

Comment 22

[Dão Gottwald [:dao]]

Attachment: sizemode=normal screenshot (native on top, Firefox on bottom)

Comment 23

[Jim Mathies [:jimm]]

Attachment: corners

Alright, updated patch that does the clipping in widget. I'll file a follow up on implementing titlebar left edge and right edge appearance styles. 

I need to throw all this at try and do a bunch of testing.

Comment 24

[Jim Mathies [:jimm]]

try builds:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-96e8f39a1210/tryserver-win32/

Comment 25

[Dão Gottwald [:dao]]

(In reply to comment #24)
> try builds:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-96e8f39a1210/tryserver-win32/

looks pixel-perfect

Comment 26

[Jim Mathies [:jimm]]

Attachment: classic edges

Fixes a couple problems in classic mode.

Comment 27

[Jim Mathies [:jimm]]

Attachment: theme changes

makes sure button metrics get refreshed when flipping between themes.

I just pushed this last set to try, if everything looks good I'll kick off some reviews.

Comment 28

[Jim Mathies [:jimm]]

Attachment: theme changes

Comment 29

[Jim Mathies [:jimm]]

Comment on attachment 490425 [details] [diff] [review]
corners

Part of this is in 'top border fix'. Clip out the left and right edges as well.

Comment 30

[Jim Mathies [:jimm]]

Comment on attachment 490483 [details] [diff] [review]
classic edges

Don't draw the edges either now that they're clipped.

Comment 31

[Jim Mathies [:jimm]]

Comment on attachment 490650 [details] [diff] [review]
theme changes

Sorry neil. One more, reset the button info flags so when the theme changes we refresh button metrics.

Comment 32

[Dão Gottwald [:dao]]

Comment on attachment 490411 [details] [diff] [review]
classic mode close button padding

I realize this is just continuing the current pattern, but given bug 606160, this seems like the wrong approach.

Comment 33

[Jim Mathies [:jimm]]

Attachment: 3 in 1 - native themeing changes

Actually, let make this simpler by combining three into one.

Comment 34

[Jim Mathies [:jimm]]

Comment on attachment 490658 [details] [diff] [review]
3 in 1 - native themeing changes

r? -> Dao for the browser.css changes in here.

Comment 35

[[e-mail]]

Comment on attachment 490650 [details] [diff] [review]
theme changes

>+      nsUXThemeData::InitTitlebarInfo();
Am I going mad or is this the only real change in this patch?

Comment 36

[Jim Mathies [:jimm]]

(In reply to comment #35)
> Comment on attachment 490650 [details] [diff] [review]
> theme changes
> 
> >+      nsUXThemeData::InitTitlebarInfo();
> Am I going mad or is this the only real change in this patch?

-  // Use system metrics for pre-vista
-  if (nsWindow::GetWindowsVersion() < VISTA_VERSION) {
-    sTitlebarInfoPopulatedAero = sTitlebarInfoPopulatedThemed = PR_TRUE;
-  }
+  // Use system metrics for pre-vista, otherwise trigger a
+  // refresh on the next layout.
+  sTitlebarInfoPopulatedAero = sTitlebarInfoPopulatedThemed =
+    (nsWindow::GetWindowsVersion() < VISTA_VERSION);

This also sets the two flags to false on vista+.

Comment 37

[[e-mail]]

Comment on attachment 490658 [details] [diff] [review]
3 in 1 - native themeing changes

>+  if (aWidgetType == NS_THEME_WINDOW_TITLEBAR) {
>+    // Clip out the left and right corners of the frame, all we want in
>+    // is the middle section.
>+    widgetRect.left -= GetSystemMetrics(SM_CXFRAME);
>+    widgetRect.right += GetSystemMetrics(SM_CXFRAME);
>+  } else if (aWidgetType == NS_THEME_WINDOW_TITLEBAR_MAXIMIZED) {
>+    // The origin of the window is off screen when maximized and windows
>+    // doesn't compensate for this in rendering the background. Push the
>+    // top of the bitmap down by SM_CYFRAME so we get the full graphic.
>+    widgetRect.top += GetSystemMetrics(SM_CYFRAME);
>+  }
>+
>   // For left edge and right edge tabs, we need to adjust the widget
>   // rects and clip rects so that the edges don't get drawn.
>   if (aWidgetType == NS_THEME_TAB) {
This patch seems reasonable although I'd probably like to see an else added before check in.

Comment 38

[[e-mail]]

(In reply to comment #36)
> This also sets the two flags to false on vista+.
I was confused because the bug only claims to relate to XP.

Comment 39

[Jim Mathies [:jimm]]

(In reply to comment #38)
> (In reply to comment #36)
> > This also sets the two flags to false on vista+.
> I was confused because the bug only claims to relate to XP.

To fix the xp issue we changed things generically. The old way of doing this was to paint the full titlebar, including left and right sides, on the surface handed into the theme code. Then we stretched the titlbar in css to compensate. 

Originally we were going to handle the whole window frame in xul, but we ended up with just the middle section of the titlebar. So this moves the clipping down into the theme code and removes the stretching from css, which was basically a temporary hack.

I filed off a new bug blocking the full xul frame bug to add titlebar corner widgets. I don't think that's going to get in for 4.0. It's primarily aimed at full persona skinning, but people seem pretty happy with the partial skinning we have now.

Comment 40

[Jim Mathies [:jimm]]

(In reply to comment #37)
> Comment on attachment 490658 [details] [diff] [review]
> 3 in 1 - native themeing changes
> This patch seems reasonable although I'd probably like to see an else added
> before check in.

Sure. What we really need to do is over haul this code to use a switch. I've been doing a little cleanup here and there but haven't tackled the whole thing since I don't have the time now.

Comment 41

[[e-mail]]

Comment on attachment 490658 [details] [diff] [review]
3 in 1 - native themeing changes

I agree, switch(!)ing to a switch is beyond the scope of this bug.

Comment 43

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/d9cf761c6af4
http://hg.mozilla.org/mozilla-central/rev/6f14ee061815

Comment 44

[horvathd]

With a hourly build (build id: 20101129082826) the maximized window looks as it should be, but if it's not then a gray line appears on top of the title bar. See attached screenshot.

Comment 45

[horvathd]

Attachment: Minimized window with build ID: 20101129082826

Comment 46

[horvathd]

And the maximize button is stuck in the maximized form. It should show only one rectangle if the window is not maximized.

Comment 47

[Jim Mathies [:jimm]]

(In reply to comment #44)
> With a hourly build (build id: 20101129082826) the maximized window looks as it
> should be, but if it's not then a gray line appears on top of the title bar.
> See attached screenshot.

Odd, I just took a look at 82f2a2e86728, and everything looks fine on Luna. Are there specific steps to reproduce?

Comment 48

[horvathd]

I just downloaded the zipped version from http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/1291087888/ (which is as far as I know the lasted build at this time) and unzipped it to a different location than the regular Firefox installation.

Comment 49

[horvathd]

Strange the nightly build (build ID: 20101130030317) works perfectly

Comment 50

[horvathd]

But not every time.

If I go to http://logout.hu/temak/faradt_goz/listaz.php and click on one name next to to avatars, a new window pop-up.

If the original window is not maximized then the new one looks as it should. But when the original is maximized then the new one looks as the attachment.

I guess the problem is that the properties of the maximized and non-maximized title bar get mixed up. It seems that the height comes from the non-maximized, but the "design" from the maximized one. and the gray line above the title bar comes from the height difference from the two design.

Comment 51

[Jim Mathies [:jimm]]

(In reply to comment #50)
> But not every time.
> 
> If I go to http://logout.hu/temak/faradt_goz/listaz.php and click on one name
> next to to avatars, a new window pop-up.
> 
> If the original window is not maximized then the new one looks as it should.
> But when the original is maximized then the new one looks as the attachment.
> 
> I guess the problem is that the properties of the maximized and non-maximized
> title bar get mixed up. It seems that the height comes from the non-maximized,
> but the "design" from the maximized one. and the gray line above the title bar
> comes from the height difference from the two design.

Sounds like this will be fixed by bug 575725 or bug 610057.

Comment 52

[horvathd]

After enabling the tabs in title bar the rendering of the title bar doesn't looks as a normal title bar.