Jim Mathies [:jimm] Reporter
	Description • 14 years ago

Original titlebar work was done in bug 575870. Fixing this involves removing the rest of the window chrome rendered by windows, and replacing it with xul styled appropriately using the new moz-appearance styles implemented in bug 574454.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 2 • 14 years ago

What's the size and scope of this work? It's marked as BetaN+ but hasn't been touched in a month, so that scares me a little bit here. Somehow I doubt that this is all gonna be Jim, too.

Decisions to be made:
 - do we need to consider the case where the menuBar is shown instead of the Firefox Button
 - who's gonna work on this

Jim: can you type a few notes about what needs to be done here?

	Jim Mathies [:jimm] Reporter
	Comment 3 • 14 years ago

- building the window frame in browser.xul using xul markup.
- applying the appropriate styling so the frame is painted correctly.
- support for mouse window resizing. (the frame has to work just like a normal window frame.)
- fixing any bugs related to glass clipping (similar to the left/right corner problems we had with the titlebar and personas)

	Dão Gottwald [:dao]
	Comment 4 • 14 years ago

Why is this blocking at all?

	Stephen Horlander [:shorlander]
	Comment 6 • 14 years ago

(In reply to comment #4)
> Why is this blocking at all?

We need this to make XP look nice and it is also required for making personas fill the entire window if I understand things correctly.

	Peter Henkel [:Terepin]
	Comment 7 • 14 years ago

(In reply to comment #6)
> (In reply to comment #4)
> > Why is this blocking at all?
> 
> ... and it is also required for making personas
> fill the entire window if I understand things correctly.

Yep. Which might look interesting with Panorama: http://www.stephenhorlander.com/images/blog-posts/incontent-ui/win7-tabcandy-glass.jpg

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 8 • 14 years ago

> Yep. Which might look interesting with Panorama:
> http://www.stephenhorlander.com/images/blog-posts/incontent-ui/win7-tabcandy-glass.jpg

That's really not relevant. Please stop commenting on bugs unless you have valuable information to add.

	Dão Gottwald [:dao]
	Comment 9 • 14 years ago

(In reply to comment #5)
> Created attachment 484700 [details]
> Windows XP Frame

This is with the menu bar hidden, which isn't the default setup. I remember having seen similar mockups with the menu bar shown, but those looked unpleasantly non-native to me... (Yes, I still use XP most of the time.)

	Peter Henkel [:Terepin]
	Comment 10 • 14 years ago

You mean this one?
http://www.mozilla.sk/wp-content/uploads/2010/05/Firefox-4-Mockup-i06-XP-LunaBlue-TabsTop-MenuBar.png

	Dão Gottwald [:dao]
	Comment 11 • 14 years ago

Yes.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 12 • 14 years ago

(In reply to comment #10)
> You mean this one?
> http://www.mozilla.sk/wp-content/uploads/2010/05/Firefox-4-Mockup-i06-XP-LunaBlue-TabsTop-MenuBar.png

I actually think that looks better than the standard XP approach, and fits better with the rest of our needs for being able to draw into the titlebar. We should continue forward with this work.

The question of whether or not it blocks, I think, depends largely on how it looks now. Anyone want to furnish me with a screenshot (both Luna and Classic?)

	Stephen Horlander [:shorlander]
	Comment 13 • 14 years ago

Attachment 484700 [details] is how it looks now in Luna. I will attach a classic SS.

	Stephen Horlander [:shorlander]
	Comment 14 • 14 years ago

(In reply to comment #13)
> Attachment 484700 [details] is how it looks now in Luna. I will attach a classic SS.

I meant to say "the top image" :)

	:Felipe Gomes (needinfo for replies!)
	Comment 15 • 14 years ago

Does XP theming really need this? My understanding is that it was only waiting on bug 543910

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 17 • 14 years ago

Thanks, Stephen. My feeling is that fixing this bug would make us look much more polished and modern (though, as Dao points out, less XP native) on XP systems. I'm not sure I'd put it at the level of a blocker, though, since we're not break-my-eyes ugly on Luna. I think we'd probably want to only do it for Luna, though, and not for Classic, even at the cost of Personas not working properly in Classic.

	Jim Mathies [:jimm] Reporter
	Comment 18 • 14 years ago

(In reply to comment #16)
> Created attachment 484791 [details]
> Windows XP Luna and Classic Screenshot

Hmm, when did you snap this Stephen? The button sizes regressed on classic. I wonder if that's fallout from bug 601603 that landed yesterday.

	Stephen Horlander [:shorlander]
	Comment 19 • 14 years ago

(In reply to comment #18)
> (In reply to comment #16)
> > Created attachment 484791 [details] [details]
> > Windows XP Luna and Classic Screenshot
> 
> Hmm, when did you snap this Stephen? The button sizes regressed on classic. I
> wonder if that's fallout from bug 601603 that landed yesterday.

I just took the screenshot from 4.0b8pre but my build was two days out-of-date.

	Jim Mathies [:jimm] Reporter
	Comment 20 • 14 years ago

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #16)
> > > Created attachment 484791 [details] [details] [details]
> > > Windows XP Luna and Classic Screenshot
> > 
> > Hmm, when did you snap this Stephen? The button sizes regressed on classic. I
> > wonder if that's fallout from bug 601603 that landed yesterday.
> 
> I just took the screenshot from 4.0b8pre but my build was two days out-of-date.

Hmm, looks ok on win7. Did you switch themes with the window open right before taking that?

	Stephen Horlander [:shorlander]
	Comment 21 • 14 years ago

(In reply to comment #20)
> Did you switch themes with the window open right before
> taking that?

Yes. I was using Luna, then switched to Classic.

	Jim Mathies [:jimm] Reporter
	Comment 22 • 14 years ago

Ok thanks, that's filed.

	Jim Mathies [:jimm] Reporter
	Comment 23 • 14 years ago

(In reply to comment #13)
> Attachment 484700 [details] is how it looks now in Luna. I will attach a classic SS.

Back to the subject at hand.. looking at this screen shot, the left and right frames look identical to the default xp frames. Couldn't we render the upper area in the titlebar and simply blend that into the default frame without having to implement this bug?

	Jim Mathies [:jimm] Reporter
	Comment 24 • 14 years ago

(In reply to comment #23)
> (In reply to comment #13)
> > Attachment 484700 [details] [details] is how it looks now in Luna. I will attach a classic SS.
> 
> Back to the subject at hand.. looking at this screen shot, the left and right
> frames look identical to the default xp frames. Couldn't we render the upper
> area in the titlebar and simply blend that into the default frame without
> having to implement this bug?

Similarly with aero basic - 

https://wiki.mozilla.org/images/a/a7/Firefox-4-Mockup-i06-%28Win7%29-%28NoAero%29-%28TabsTop%29.png

I don't think there's anything here holding up this work, we just need to match the left and right sides with the default frame. (For an idea of how this would look, take a look at the IE9 beta.)

This bug is specific to getting full skinning of the window frame with personas, but shouldn't hold up our titlebar / tabbar theme work.

	Jim Mathies [:jimm] Reporter
	Comment 25 • 14 years ago

(and probably shouldn't block a beta unless we feel personas skinning of the left, right, and bottom frame is critical.)

	Alex Limi (:limi) — Firefox UX Team
	Comment 26 • 14 years ago

(In reply to comment #25)
> (and probably shouldn't block a beta unless we feel personas skinning of the
> left, right, and bottom frame is critical.)

I would argue that it is, Personas just look buggy and unfinished without it. We certainly see a fair amount of feedback to that effect.

(and yes, I know that we shipped it even worse in 3.6, but that shouldn't be the benchmark ;)

	Benjamin Smedberg
	Comment 27 • 14 years ago

This is a pretty big change to be taking post feature-freeze. It changes the XUL structure in ways that could affect extensions with overlays and will definitely affect extensions that modify window appearance using CSS. I really think it's just too late.

	Dão Gottwald [:dao]
	Comment 28 • 14 years ago

Keep in mind that the common persona header image is only so many pixels high. We'd have to use the solid "accent color" for the frame, which is even worse than it sounds, because authors often specify arbitrary accent colors.

	Jim Mathies [:jimm] Reporter
	Comment 29 • 14 years ago

(In reply to comment #28)
> Keep in mind that the common persona header image is only so many pixels high.
> We'd have to use the solid "accent color" for the frame, which is even worse
> than it sounds, because authors often specify arbitrary accent colors.

Is this really what we want in the end? The accent color sounds like temp fix for older personas. I'd like to see graphics covering the entire frame.

I;d suggest we support full graphical skinning on m-c after we branch 4.0. Then work on changes to the personas site and evangelizing to get people to update their existing personas to support the new frame graphics.

	Alex Faaborg [:faaborg] (Firefox UX)
	Comment 30 • 14 years ago

comment #29
>Is this really what we want in the end? The accent color sounds like temp fix
>for older personas. I'd like to see graphics covering the entire frame.

Yeah, graphics should ideally cover the entire frame.  We'll need to continue to work with the personas team to transition personas to the new size.  (I wonder to what extent we used the current size because OS X doesn't happen to have a window frame).

comment #27
>This is a pretty big change to be taking post feature-freeze. It changes the
>XUL structure in ways that could affect extensions with overlays and will
>definitely affect extensions that modify window appearance using CSS. I really
>think it's just too late.

Had those concerns been brought up earlier we likely would have tried to get this blocking an earlier beta, but in terms of the changes need for our XP theme, I believe this still needs to block betaN.  If personas were the only concern, we could just push all of this work back and deal with it later, but about half of our market share is still using XP and this bug plays a major role in how we craft the main window to appear more modern.

	Jim Mathies [:jimm] Reporter
	Comment 31 • 14 years ago

(In reply to comment #30)
> comment #29
> Had those concerns been brought up earlier we likely would have tried to get
> this blocking an earlier beta, but in terms of the changes need for our XP
> theme, I believe this still needs to block betaN.  

Why can't we blend our titlebar in with the existing window border? Looks like it would be pretty easy to do on themes like Luna and Aero Basic.

	Johnathan Nightingale [:johnath]
	Comment 32 • 14 years ago

Notes from the Grand Retriage: too late, we'll design around it.

	Alex Faaborg [:faaborg] (Firefox UX)
	Comment 33 • 14 years ago

Note that bug 625307 covers implementing the visual appearance we want, with just modifying the title bar.  However, when we can draw the entire window frame we might later make some tweaks to the window border.  Also either way this bug is still very much wanted for personas.

	Jim Mathies [:jimm] Reporter
	Comment 34 • 14 years ago

(In reply to comment #33)
> Note that bug 625307 covers implementing the visual appearance we want, with
> just modifying the title bar.  However, when we can draw the entire window
> frame we might later make some tweaks to the window border.  Also either way
> this bug is still very much wanted for personas.

This is the first thing I want to do once we get 4.0 blockers cleared.

	zigboom
	Comment 35 • 14 years ago

I have just posted in the themes forum about my wish to implement this and got directed here. 
Just wanted to point out that Google Chrome does it and looks great!
It allows designing a more modern frames that could blend if we wish to, with the OS colors.
For example, to make the (I think we can agree - ugly) Luna look better, we could design a slim frame that keeps the blue color but creates a more modern look.

	Sergey Sokoloff
	Comment 36 • 13 years ago

Bug 77790 is probably somewhat related.

	Jim Mathies [:jimm] Reporter
	Comment 37 • 13 years ago

This is how things are looking at this point. I went with repeat-y on the texture which actually works out quite well for the entire frame. I'd say about 80% of the personas I've tried look excellent or acceptable with the texture repeated down the side of the border. The only time you notice it is when the seam lands in the addon bar, but I think that's a worth while trade off. Designers can improve the way the texture stack over time.

On aero basic, xp, and classic everything remains the same as it is now. The normal windows chrome paints over these areas so there's no need to texture them. We can spin off bugs on implementing the xp and aero seemless look and feel after I get this work completed.

It looks like I may be able to land all of this work on the e10s branch so we'll have test coverage and nightly builds.

I should have patches posted in a day or so.

	Jim Mathies [:jimm] Reporter
	Comment 43 • 13 years ago

Comment on attachment 518194 [details]
screen shot

Alex, requesting ui-review for a couple things - I've added slight opacity to the fx button when it's not hovered or active. Just thought it looked pretty cool. Curious what you think. Also, prelim ui-review on the way the textures are being repeated. I'll get a try build going as well for testing purposes.

	Josh Tumath
	Comment 44 • 13 years ago

I'm not an expert on this, but I was wondering...

+#main-window:not([chromemargin]) #window-frame-left-top,
+#main-window:not([chromemargin]) #window-frame-left,
+#main-window:not([chromemargin]) #window-frame-right-top,
+#main-window:not([chromemargin]) #window-frame-right,
+#main-window:not([chromemargin]) #window-frame-bottom,
+#main-window[sizemode="maximized"] #window-frame-left-top,
+#main-window[sizemode="maximized"] #window-frame-left,
+#main-window[sizemode="maximized"] #window-frame-right-top,
+#main-window[sizemode="maximized"] #window-frame-right,
+#main-window[sizemode="maximized"] #window-frame-bottom,
+#main-window[inFullscreen] #window-frame-left-top,
+#main-window[inFullscreen] #window-frame-left,
+#main-window[inFullscreen] #window-frame-right-top,
+#main-window[inFullscreen] #window-frame-right,
+#main-window[inFullscreen] #window-frame-bottom {
+  display: none;
+}

It might be better to use the -moz-any() selector here. Correct me if I'm wrong.

	Jim Mathies [:jimm] Reporter
	Comment 45 • 13 years ago

Comment on attachment 518185 [details] [diff] [review]
corner styles v.1

David, this adds two moz-appearance styles that represent the upper left and upper right corners of the titlebar. (bug 612026)

	Alex Faaborg [:faaborg] (Firefox UX)
	Comment 46 • 13 years ago

Comment on attachment 518194 [details]
screen shot

Moving review over to Stephen since this is entirely visual and he's given this more thought.  For the Firefox button, I think we should consider falling back to transparent instead of orange, since we really have no idea what background the orange is going to appear on.

	Jim Mathies [:jimm] Reporter
	Comment 47 • 13 years ago

(In reply to comment #46)
> Comment on attachment 518194 [details]
> screen shot
> 
> Moving review over to Stephen since this is entirely visual and he's given this
> more thought.  For the Firefox button, I think we should consider falling back
> to transparent instead of orange, since we really have no idea what background
> the orange is going to appear on.

I'd suggest taking a try build for a spin before deciding. With .9 opacity the texture adds the persona's hue to the orange of the button, but doesn't corrupt it. IMHO it looks pretty good. I'll post a link to try builds once they finish.

	Jim Mathies [:jimm] Reporter
	Comment 48 • 13 years ago

A try build should be available here in about six hours or so:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/jmathies@mozilla.com-f30a58beabea

	Richard Marti (:Paenglab)
	Comment 49 • 13 years ago

(In reply to comment #48)
> A try build should be available here in about six hours or so:
> 
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/jmathies@mozilla.com-f30a58beabea

I tried this under Win7. With Aero and Classic the Personas are applied to the border, but under Basic not.

	Jim Mathies [:jimm] Reporter
	Comment 50 • 13 years ago

(In reply to comment #49)
> (In reply to comment #48)
> > A try build should be available here in about six hours or so:
> > 
> > http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/jmathies@mozilla.com-f30a58beabea
> 
> I tried this under Win7. With Aero and Classic the Personas are applied to the
> border, but under Basic not.

I need to fix the border on classic, it's not supposed to show up there or on aero basic. Basically, classic, aero basic, and xp these should all look normal. (Even though we are now painting the entire frame and titlebar areas ourselves.) Aero glass desktops should have full personas skinning of all glass areas, including the window frame.

	redsign
	Comment 51 • 13 years ago

Using the Aero Peek feature by hovering over the rectangular button on the right of the task bar in Windows 7 the window of Fx is disappearing completely instead of showing the outline.

	Stephen Horlander [:shorlander]
	Comment 52 • 13 years ago

(In reply to comment #51)
> Using the Aero Peek feature by hovering over the rectangular button on the
> right of the task bar in Windows 7 the window of Fx is disappearing completely
> instead of showing the outline.

I noticed this as well.

	Stephen Horlander [:shorlander]
	Comment 53 • 13 years ago

Comment on attachment 518194 [details]
screen shot

This looks great! The translucency on the Firefox button is a nice touch.

	Jim Mathies [:jimm] Reporter
	Comment 54 • 13 years ago

(In reply to comment #51)
> Using the Aero Peek feature by hovering over the rectangular button on the
> right of the task bar in Windows 7 the window of Fx is disappearing completely
> instead of showing the outline.

Interesting. Chrome has the same problem. I don't think it should hold up the initial patch landings here. I'll file a follow up if I can't find a simple fix.

	Jim Mathies [:jimm] Reporter
	Comment 59 • 13 years ago

Comment on attachment 518621 [details] [diff] [review]
theme cleanup v.2

Simple theme code cleanup - swapping if else with switch.

	Jim Mathies [:jimm] Reporter
	Comment 60 • 13 years ago

Comment on attachment 518623 [details] [diff] [review]
theme work v.2

Theme code changes. There's two main chunks in here, the first uses the gfxContext to clear the four corners of the window so that they don't end up square.

The second chunk is the classic rendering for the left, right, and bottom window frames plus misc. additions to the various nsITheme calls to make everything work right.

	Jim Mathies [:jimm] Reporter
	Comment 61 • 13 years ago

One feature I had hoped to add was a thinning of the glass chrome when the browser is in "normal" window mode (menu bar enabled). Unfortunately the Windows dwm doesn't seem to like this. I can remove the chrome completely, or expand it in, but shaving a couple pixels off the inside edge fails. 

In order to get this to work we'll have to remove the default glass chrome completely in this mode as well. This presents some problems with personas and the default titlebar and it's contents. I'm going to avoid the complexity of adding this here and file a follow up.

	Jim Mathies [:jimm] Reporter
	Comment 62 • 13 years ago

maybe with the right stylistic additions, but stand alone this does not work.

	Jim Mathies [:jimm] Reporter
	Comment 63 • 13 years ago

(up to date patch and switching reviewers as Vlad is not with us anymore.)

> Theme code changes. There's two main chunks in here, the first uses the
> gfxContext to clear the four corners of the window so that they don't end up
> square.
> 
> The second chunk is the classic rendering for the left, right, and bottom
> window frames plus misc. additions to the various nsITheme calls to make
> everything work right.

	neil@parkwaycc.co.uk
	Comment 64 • 13 years ago

Comment on attachment 519169 [details] [diff] [review]
theme work v.2

Since I don't have Aero Basic, I'm not sure what the issue is with the top corners. Can you not draw the corners as part of the left and right sides, hopefully this will completely avoid the problem?

	Jim Mathies [:jimm] Reporter
	Comment 65 • 13 years ago

(In reply to comment #64)
> Comment on attachment 519169 [details] [diff] [review]
> theme work v.2
> 
> Since I don't have Aero Basic, I'm not sure what the issue is with the top
> corners. Can you not draw the corners as part of the left and right sides,
> hopefully this will completely avoid the problem?

Aero basic isn't the issue, it's glass. Since we don't render the main window background here, what we do is clear the contents when we have the chance. We could clip the painting of the main window in the OnPaint handler in nsWindow, or we can handle it here. (We are already handling the command buttons this way just above this code.)

	Jim Mathies [:jimm] Reporter
	Comment 66 • 13 years ago

+    case NS_THEME_WINDOW_FRAME_LEFT_TOP:
+      // Push the center area and the opposite edge off the surface we'll render
+      // to. This must be a large number to prevent window edge drawing anomalies
+      // on aero basic.
+      widgetRect.right += 100;
+      break;
+    case NS_THEME_WINDOW_FRAME_RIGHT_TOP:
+      widgetRect.left -= 100;
+      break;

Or were you asking about this part of the patch?

Since we can only ask Windows to render the titlebar in one piece (left corner plus mid section plus right corner), I tell Windows to render it in an elongated section where most of the surface is off the hdc. If I ask Window to render just the corner, it'll try to squish the entire titlebar into that little part.

	neil@parkwaycc.co.uk
	Comment 67 • 13 years ago

(In reply to comment #66)
> Or were you asking about this part of the patch?
Yes, I was.

> Since we can only ask Windows to render the titlebar in one piece (left corner
> plus mid section plus right corner), I tell Windows to render it in an
> elongated section where most of the surface is off the hdc. If I ask Window to
> render just the corner, it'll try to squish the entire titlebar into that
> little part.
Sorry, I'm not seeing where the call to render the titlebar is in the patch.

	Jim Mathies [:jimm] Reporter
	Comment 68 • 13 years ago

(In reply to comment #67)
> Sorry, I'm not seeing where the call to render the titlebar is in the patch.

The code to render the sections is already in place. This all falls through to nsUXThemeData::drawThemeBG.

For the mid section, we set things up in similar fashion:

  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);
  }

This additional code:

+    case NS_THEME_WINDOW_FRAME_LEFT_TOP:
+      // Push the center area and the opposite edge off the surface we'll render
+      // to. This must be a large number to prevent window edge drawing anomalies
+      // on aero basic.
+      widgetRect.right += 100;
+      break;
+    case NS_THEME_WINDOW_FRAME_RIGHT_TOP:
+      widgetRect.left -= 100;
+      break;


sets up the surface we render to using nsUXThemeData::drawThemeBG for the corners. Between these three theme types we can render a complete titlebar.

	neil@parkwaycc.co.uk
	Comment 69 • 13 years ago

Ah, so WP_CAPTION actually draws the corners too?

	Jim Mathies [:jimm] Reporter
	Comment 70 • 13 years ago

(In reply to comment #69)
> Ah, so WP_CAPTION actually draws the corners too?

Yes, windows considers it one piece. We needed it broken down to do the titlebar work in 4.0. We could overhaul all that and combine the edges with the existing titlebar style, but that would require a lot of code/css/xul rework, and we'd loose the "drawintitlebar" functionality we currently have.

	Jim Mathies [:jimm] Reporter
	Comment 71 • 13 years ago

Note I did not break out the lower left and right corners as individual styles. The base is just one piece.

	Jim Mathies [:jimm] Reporter
	Comment 73 • 13 years ago

Comment on attachment 522470 [details] [diff] [review]
add clear to nsmargin v.1

simple patch that adds a utility method for clearing an nsMargin to zero.

	Jim Mathies [:jimm] Reporter
	Comment 74 • 13 years ago

UpdateNonClientMargins changes so that we can have these custom margins working all the time, which gives us the ability to compensate for our borderless glass adjustment.

	Jim Mathies [:jimm] Reporter
	Comment 75 • 13 years ago

glass calculation changes:

1) replaces MAX_HORIZONTAL_GLASS_MARGIN with GetSystemMetrics(SM_CXFRAME). 

Insures we fall into the margin calculations when we have an nc client area we render ourselves.

2) Set margins to the max of either the calculated opaque region or the nc client area margin, which insures the rounded corners of the window are covered by glass. Without this you get squared off corners.

3) only adjust the margin values by kGlassMarginAdjustment if we are letting windows handle the nc client area. If we aren't there no need to compensate for the border since windows doesn't draw it.

	Jim Mathies [:jimm] Reporter
	Comment 76 • 13 years ago

Update the hit test code to make sure we respect the width and height of the window border.

	Jim Mathies [:jimm] Reporter
	Comment 77 • 13 years ago

easy peasy hit test debug feature. This is invaluable in finding mathematical problems with our mouse hit test code.

There's a bug filed on converting all this printf output to nspr logging, but I've never gotten around to it. So I stuck with printf on this.

	Jim Mathies [:jimm] Reporter
	Comment 78 • 13 years ago

Browser css changes -

1) due to XUL structure changes I replaced some child selectors with id selectors.
2) added border styling on all sides of the window frame.
3) added border radius settings for the round corners. Note radius of these is based on the margin width/height in system settings. The function updateFrameMarginSettings in browser.js sets these values dynamically based on the boxObject of the frame. updateFrameMarginSettings also sets the proper offset on the status panel, which has absolute positioning.
4) set the opacity on the fx button per the screen shots I posted.
5) handles hiding the frame when it's not needed / enabled.

	Jim Mathies [:jimm] Reporter
	Comment 79 • 13 years ago

Comment on attachment 522482 [details] [diff] [review]
browser css v.1

(see comments 78)

	Rob Arnold [:robarnold]
	Comment 80 • 13 years ago

Comment on attachment 522478 [details] [diff] [review]
hit test debug feature

DebugHitTest would probably look cleaner using a switch statement.

	Dão Gottwald [:dao]
	Comment 81 • 13 years ago

Comment on attachment 522482 [details] [diff] [review]
browser css v.1

>+  #browser-bottombox[lwthemefooter="true"] {
>+    background-image: none !important;
>+    background: none !important;
>   }

This doesn't seem right, the footer image should still be used.

	Deb Richardson [:dria] (plz NEEDINFO)
	Comment 82 • 13 years ago

Not using the footer image is a significant change and something designers would not be thrilled about.  Many are already a little annoyed (or more) that the Firefox 4 UI shows so much less of the Persona by default :)

Would it be possible to continue using the footer image?  Having this be inconsistent across OSs seems like a flaw to me.

	Jim Mathies [:jimm] Reporter
	Comment 83 • 13 years ago

Attaching one example. Aesthetically it's a pretty big trade off. If we could give the footer a z level higher than the header it would meet in the frame and probably wouldn't look too bad. (Dao, any ideas?) If that's not possible I'd say just ditch the footer.

	Frank Yan (:fryn)
	Comment 84 • 13 years ago

I agree with Jim. I don't think designers would be entirely upset by this change, because they will actually have more room for their designs compared to 4.0.

If we must include the footer image with the add-ons bar, we could include a border around it to separate the two images and to match the width of the border around the content area, although that makes the add-ons bar somewhat spoof-able. I'd go with removing the footer image.

	Jim Mathies [:jimm] Reporter
	Comment 85 • 13 years ago

Looking through getpersonas, the footers tend to be unique textures, so they generally won't match well with headers even if we could get the overlapping to work. Not sure what to do here.

I can build up a couple of try builds if folks want to take each design for a spin.

	Jim Mathies [:jimm] Reporter
	Comment 86 • 13 years ago

(In reply to comment #82)
> Many are already a little annoyed (or more) that
> the Firefox 4 UI shows so much less of the Persona by default :)

That's likely bug 622366, and we would like to get that fixed.

	Dão Gottwald [:dao]
	Comment 87 • 13 years ago

(In reply to comment #83)
> If we could
> give the footer a z level higher than the header it would meet in the frame and
> probably wouldn't look too bad. (Dao, any ideas?)

I don't understand what kind of difference giving it "a z level higher than the header" would make visually, nor what exactly it means really...

	Dão Gottwald [:dao]
	Comment 88 • 13 years ago

Comment on attachment 522482 [details] [diff] [review]
browser css v.1

This already makes it hard enough to create a decent appearance for aero glass when you're not using aero glass (i.e. if you're on XP, Win7 with aero glass unavailable, Linux or Mac). Hiding the footer image for aero glass crosses the line of cross platform compatibility.

	Jim Mathies [:jimm] Reporter
	Comment 89 • 13 years ago

(In reply to comment #88)
> Comment on attachment 522482 [details] [diff] [review]
> browser css v.1
> 
> This already makes it hard enough to create a decent appearance for aero glass
> when you're not using aero glass (i.e. if you're on XP, Win7 with aero glass
> unavailable, Linux or Mac). Hiding the footer image for aero glass crosses the
> line of cross platform compatibility.

Could you clarify why this was r-'d? Are you saying you want the footer visible as in the screen shot on the left here:

https://bug590945.bugzilla.mozilla.org/attachment.cgi?id=522550

or did you minus the patch for some other reason? I don't understand what you mean by "create a decent appearance for aero glass when you're not using aero glass".

	Dão Gottwald [:dao]
	Comment 90 • 13 years ago

> I don't understand what you
> mean by "create a decent appearance for aero glass when you're not using aero
> glass".

People on said platforms won't see anything close to attachment 522550 [details], which makes it more complicated for them to create images that work fine across the board.

Hiding the footer image for aero glass makes this situation even worse (e.g. for designers who do use aero glass), and that's what my r- is for.

	Jim Mathies [:jimm] Reporter
	Comment 91 • 13 years ago

(In reply to comment #90)
> > I don't understand what you
> > mean by "create a decent appearance for aero glass when you're not using aero
> > glass".
> 
> People on said platforms won't see anything close to attachment 522550 [details], which
> makes it more complicated for them to create images that work fine across the
> board.
> 
> Hiding the footer image for aero glass makes this situation even worse (e.g.
> for designers who do use aero glass), and that's what my r- is for.

Ok, not a big deal since it's not that visible anyway. Updated patch with the footer re-enabled coming up.

	Jim Mathies [:jimm] Reporter
	Comment 92 • 13 years ago

updated.

	Dão Gottwald [:dao]
	Comment 93 • 13 years ago

Comment on attachment 522672 [details] [diff] [review]
browser css w/footer v.1

>+function updateFrameMarginSettings(chromemargin) {
>+  // setup the correct border radii on the corners of the window
>+  // based on the width/height of the frame. These values are
>+  // dynamically set down in widget theming code.
>+  let element = document.getElementById("window-frame-left-top-inner");
>+  let leftwidth = element.boxObject.width;
>+  element.style.borderTopLeftRadius = leftwidth + "px";
>+  element = document.getElementById("window-frame-right-top-inner");
>+  let rightwidth = element.boxObject.width;
>+  element.style.borderTopRightRadius = rightwidth + "px";
>+  element = document.getElementById("window-frame-bottom-inner");
>+  element.style.borderBottomLeftRadius = element.boxObject.height + "px";
>+  element.style.borderBottomRightRadius = element.boxObject.height + "px";

What happens when you set these radii to some large value regardless of the element dimensions?

>+  // set the padding on the status panel
>+  let e = document.getElementById("statusbar-display");
>+  if (!e.hasAttribute("mirror")) {
>+    e.style.left = (chromemargin ? leftwidth : 0) + "px";
>+    e.style.right = "auto";
>+  } else {
>+    e.style.left = "auto";
>+    e.style.right = (chromemargin ? rightwidth : 0) + "px";
>+  }

I haven't tested this, but this looks wrong. The mirror attribute isn't static, it comes and goes every now and then.

Also, most of the above code doesn't look rtl-ready. (=> r-)


>+@media all and (-moz-windows-compositor) {
>+  #appmenu-button:-moz-lwtheme {
>+    opacity: .9;
>+  }
>+}

If you add :not(:hover):not([open]) here, you can remove this:

>+#appmenu-button:hover,
>+#appmenu-button[open] {
>+  opacity: 1;
>+}


>+#main-window:not([chromemargin]) #window-frame-left-top,
>+#main-window:not([chromemargin]) #window-frame-left,
>+#main-window:not([chromemargin]) #window-frame-right-top,
>+#main-window:not([chromemargin]) #window-frame-right,
>+#main-window:not([chromemargin]) #window-frame-bottom,
>+#main-window[sizemode="maximized"] #window-frame-left-top,
>+#main-window[sizemode="maximized"] #window-frame-left,
>+#main-window[sizemode="maximized"] #window-frame-right-top,
>+#main-window[sizemode="maximized"] #window-frame-right,
>+#main-window[sizemode="maximized"] #window-frame-bottom,
>+#main-window[inFullscreen] #window-frame-left-top,
>+#main-window[inFullscreen] #window-frame-left,
>+#main-window[inFullscreen] #window-frame-right-top,
>+#main-window[inFullscreen] #window-frame-right,
>+#main-window[inFullscreen] #window-frame-bottom {
>+  display: none;
>+}

This should probably be in browser/base/content/browser.css. In fact, maybe the rules setting the -moz-appearance for these nodes should be in content too, to make theme authors' lives easier.

	neil@parkwaycc.co.uk
	Comment 94 • 13 years ago

Comment on attachment 519169 [details] [diff] [review]
theme work v.2

Sorry for taking so long to get back to this.

>+      ctx->Arc(gfxPoint(offset,offset), offset, 180 * (M_PI / 180.0f),
>+               270 * (M_PI / 180.0f));
>+      ctx->LineTo(gfxPoint(0,0));
>+    } else {
>+      ctx->MoveTo(gfxPoint(0,0));
>+      ctx->Arc(gfxPoint(0,offset), offset, 270 * (M_PI / 180.0f), 2 * M_PI);
As well as 2 * M_PI, why not also 0.5 * M_PI, M_PI, 1.5 * M_PI as appropriate? Or if you prefer, 360 * (M_IP / 180.0f) instead of 2 * M_PI.

	Jim Mathies [:jimm] Reporter
	Comment 95 • 13 years ago

(In reply to comment #93)
> Comment on attachment 522672 [details] [diff] [review]
> browser css w/footer v.1
> 
> >+  element = document.getElementById("window-frame-bottom-inner");
> >+  element.style.borderBottomLeftRadius = element.boxObject.height + "px";
> >+  element.style.borderBottomRightRadius = element.boxObject.height + "px";
> 
> What happens when you set these radii to some large value regardless of the
> element dimensions?

They should always be based on the size of the margin to a point. I did some testing with custom border padding and noticed Windows clamps this so that the radius is always <= eight. I need to add that.

> 
> >+  // set the padding on the status panel
> >+  let e = document.getElementById("statusbar-display");
> >+  if (!e.hasAttribute("mirror")) {
> >+    e.style.left = (chromemargin ? leftwidth : 0) + "px";
> >+    e.style.right = "auto";
> >+  } else {
> >+    e.style.left = "auto";
> >+    e.style.right = (chromemargin ? rightwidth : 0) + "px";
> >+  }
> 
> I haven't tested this, but this looks wrong. The mirror attribute isn't static,
> it comes and goes every now and then.
> 
> Also, most of the above code doesn't look rtl-ready. (=> r-)

I'll check things with ehsan's rtl extension and test a bit more. The statusbar-display was a real pita since it's absolutely positioned. Without the proper left/right margin settings (which vary depending on Windows system settings) the panel ends up overlaping the glass window frame.

> 
> 
> >+@media all and (-moz-windows-compositor) {
> >+  #appmenu-button:-moz-lwtheme {
> >+    opacity: .9;
> >+  }
> >+}
> 
> If you add :not(:hover):not([open]) here, you can remove this:
> 
> >+#appmenu-button:hover,
> >+#appmenu-button[open] {
> >+  opacity: 1;
> >+}
> 
> 
> >+#main-window:not([chromemargin]) #window-frame-left-top,
> >+#main-window:not([chromemargin]) #window-frame-left,
> >+#main-window:not([chromemargin]) #window-frame-right-top,
> >+#main-window:not([chromemargin]) #window-frame-right,
> >+#main-window:not([chromemargin]) #window-frame-bottom,
> >+#main-window[sizemode="maximized"] #window-frame-left-top,
> >+#main-window[sizemode="maximized"] #window-frame-left,
> >+#main-window[sizemode="maximized"] #window-frame-right-top,
> >+#main-window[sizemode="maximized"] #window-frame-right,
> >+#main-window[sizemode="maximized"] #window-frame-bottom,
> >+#main-window[inFullscreen] #window-frame-left-top,
> >+#main-window[inFullscreen] #window-frame-left,
> >+#main-window[inFullscreen] #window-frame-right-top,
> >+#main-window[inFullscreen] #window-frame-right,
> >+#main-window[inFullscreen] #window-frame-bottom {
> >+  display: none;
> >+}
> 
> This should probably be in browser/base/content/browser.css. In fact, maybe the
> rules setting the -moz-appearance for these nodes should be in content too, to
> make theme authors' lives easier.

will update.

	David Baron :dbaron:
	Comment 96 • 13 years ago

Comment on attachment 518185 [details] [diff] [review]
corner styles v.1

If these are corners, I'd much prefer top-left and top-right, which is much more consistent with the rest of CSS (and I think with general English usage, though not Chinese).

Or are these something other than corners?

r=dbaron.

	Jim Mathies [:jimm] Reporter
	Comment 97 • 13 years ago

(In reply to comment #96)
> Comment on attachment 518185 [details] [diff] [review]
> corner styles v.1
> 
> If these are corners, I'd much prefer top-left and top-right, which is much
> more consistent with the rest of CSS (and I think with general English usage,
> though not Chinese).
> 
> Or are these something other than corners?
> 
> r=dbaron.

They are corners. The reason I went with left-top/right-top is that they are associated with the left and right sides. We could flip the side/top.. or maybe associate them with the titlebar via something like 'moz-window-titlebar-left'/'moz-window-titlebar-right'. (That seems a bit confusing to me since we also have 'moz-window-titlebar-maximized' which doesn't need corners!) So I ended up with left-top, right-top. I'm not partial to any of these so whatever you think fits best I'm fine with.

	Jim Mathies [:jimm] Reporter
	Comment 98 • 13 years ago

This fixes a bug that's been around since the original titlebar work landed. When a user changes certain advanced ui settings, instead of a theme changed event we get a settings changed event with wParam equal to SPI_SETNONCLIENTMETRICS. The patch detects that and refreshes all our ui settings plus it fires off a theme changed event.

	neil@parkwaycc.co.uk
	Comment 99 • 13 years ago

Comment on attachment 523137 [details] [diff] [review]
settings changed notification v.1

Can we not refactor this and the WM_THEMECHANGED code into a separate method?

	Jim Mathies [:jimm] Reporter
	Comment 100 • 13 years ago

rtl updates on the frame plus previous changes requested. status panel support is not in this, it also has some rtl issues. I'll post that in a followup patch.

	Jim Mathies [:jimm] Reporter
	Comment 101 • 13 years ago

(In reply to comment #99)
> Comment on attachment 523137 [details] [diff] [review]
> settings changed notification v.1
> 
> Can we not refactor this and the WM_THEMECHANGED code into a separate method?

sure, I'll update that locally.

	Jim Mathies [:jimm] Reporter
	Comment 103 • 13 years ago

I went ahead and flipped left-top / right-top to top-left / top-right per dbaron's suggestion. I also updated the xul element ids to match.

	Jim Mathies [:jimm] Reporter
	Comment 104 • 13 years ago

(In reply to comment #74)
> Created attachment 522472 [details] [diff] [review]
> UpdateNonClientMargins v.1
> 
> UpdateNonClientMargins changes so that we can have these custom margins working
> all the time, which gives us the ability to compensate for our borderless glass
> adjustment.

One small update to this:

+  mNonClientMargins.Clear();
+  mNonClientOffset.Clear();

should have been:

+  mNonClientMargins.SizeTo(-1,-1,-1,-1);
+  mNonClientOffset.Clear();

this caused a mochitest chrome test failure. The initial values should be set to disable chrome margins, vs. removing them. I've fixed this in my local patch queue.

	Jim Mathies [:jimm] Reporter
	Comment 105 • 13 years ago

try builds with everything put together available in a few hours:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/jmathies@mozilla.com-92796d2403f6

	Valerio
	Comment 106 • 13 years ago

with the latest tryserver build there is the same problem for the borders appears in a precedent bug (I don't remember its number), the border around the content area is opaque and when the window is maximized appears a white line above the tabs toolbar

	Jim Mathies [:jimm] Reporter
	Comment 107 • 13 years ago

(In reply to comment #106)
> Created attachment 523774 [details]
> border
> 
> with the latest tryserver build there is the same problem for the borders
> appears in a precedent bug (I don't remember its number), the border around the
> content area is opaque and when the window is maximized appears a white line
> above the tabs toolbar

Looks like bug 637018. I'll see if I can track down the cause, but I wouldn't hold up landing these patches for it.

	Alex Faaborg [:faaborg] (Firefox UX)
	Comment 108 • 13 years ago

I think the main disconnect with the footers was that the personas team was primarily on OS X, where the title bar and footer are two completley seperate elements.  On every version of windows (classic, luna, aero, glass), the window frame is a single surface, with the top and bottom connected together.  I propose we:

-for now ignore the footer images on windows

-in the future switch to a new personas format that encourages designers to draw out the entire frame, and then leverage 9 image tiling for increasing the size of the window, or cropping the sides off entirely for OS X

	Dão Gottwald [:dao]
	Comment 109 • 13 years ago

(In reply to comment #107)
> (In reply to comment #106)
> > Created attachment 523774 [details]
> > border
> > 
> > with the latest tryserver build there is the same problem for the borders
> > appears in a precedent bug (I don't remember its number), the border around the
> > content area is opaque and when the window is maximized appears a white line
> > above the tabs toolbar
> 
> Looks like bug 637018.

Nope, this looks like you're regressing bug 629709.

	Valerio
	Comment 110 • 13 years ago

(In reply to comment #109)
> (In reply to comment #107)
> > (In reply to comment #106)
> > > Created attachment 523774 [details]
> > > border
> > > 
> > > with the latest tryserver build there is the same problem for the borders
> > > appears in a precedent bug (I don't remember its number), the border around the
> > > content area is opaque and when the window is maximized appears a white line
> > > above the tabs toolbar
> > 
> > Looks like bug 637018.
> 
> Nope, this looks like you're regressing bug 629709.

bug 629709 was exactly what I meant

	Deb Richardson [:dria] (plz NEEDINFO)
	Comment 111 • 13 years ago

Ignoring the footer image is a significant change that will have to be communicated loudly to the Personas designer community and will require fairly extensive changes to the GetPersonas website (and probably the Personas Plus add-on, but I'll have to double check that).

If this is required by the UX team that's fine, it will just be difficult (possibly impossible) to the rest of this ready in time for the next release of Firefox so it may have to wait for a later release.

	Jim Mathies [:jimm] Reporter
	Comment 112 • 13 years ago

(In reply to comment #111)
> Ignoring the footer image is a significant change that will have to be
> communicated loudly to the Personas designer community and will require fairly
> extensive changes to the GetPersonas website (and probably the Personas Plus
> add-on, but I'll have to double check that).
> 
> If this is required by the UX team that's fine, it will just be difficult
> (possibly impossible) to the rest of this ready in time for the next release of
> Firefox so it may have to wait for a later release.

Dropping the footer entirely for all platforms isn't what we want to do. I was thinking that for (7 / Vista) + aero glass we would simply not display it. That would not require much of a change to the site (just text on the differences in behavior) and would not require changes to personas or the personas add-on.

As far schedule goes, this is purely a (really really big) cosmetic win for Windows 7 / Vista glass users. We can delay it to 6 or 7 if needed.

	Deb Richardson [:dria] (plz NEEDINFO)
	Comment 113 • 13 years ago

It requires changes to the site because I will have to explain to designers that their footer images will not be used on those platforms, as well as updating the documentation and tutorials (which also require L10N changes).

Based on the stats I have this will impact around 35 million users (41% of Firefox users are on Win 7/Vista, and 19% use Personas), so it would be great to get this out to them ASAP.

I spoke with morgamic and apparently I will be able to get resources to make the site changes.  I'll sort out what needs to be done and file those bugs as soon as I can.

	Alex Faaborg [:faaborg] (Firefox UX)
	Comment 114 • 13 years ago

We can discuss the footer choices at the personas meeting tomorrow.  Generally speaking we need to move towards one big image that we can use, not just for modern windows, but contexts like Panorama (on OS X, Linux, etc.)

	Deb Richardson [:dria] (plz NEEDINFO)
	Comment 115 • 13 years ago

I'm currently working on sorting out and filing the necessary bugs to get the documentation and website updated to account for this change.  Would it make sense to mark this as dependent on those when they're filed?

	Jim Mathies [:jimm] Reporter
	Comment 116 • 13 years ago

(In reply to comment #115)
> I'm currently working on sorting out and filing the necessary bugs to get the
> documentation and website updated to account for this change.  Would it make
> sense to mark this as dependent on those when they're filed?

Not dependent but please post them here so we can track. 

I'm hoping to get these reviews done and land on m-c next week. (I need to post a couple patches here for the footer and a fix for the white line problem mentioned in comment 106 still but that shouldn't be a big deal.) That means we'd be landing two weeks into the 6.0 release cycle, so we'd have about 3.5 months before we release to the world. Do you think that will provide enough time for site changes Deb? If not we'll have to wait for 7.0.

	Richard Marti (:Paenglab)
	Comment 117 • 13 years ago

Why have you added in browser/browser.css:

#main-window[inFullscreen] #titlebar {
  display: none;
}

and not only changed in content/browser.css:

#main-window[inFullscreen] > #titlebar,
#main-window[inFullscreen] .titlebar-placeholder,
#main-window:not([tabsintitlebar]) .titlebar-placeholder {
  display: none;
}

This would be easier and not leave a unused selector.

	Jim Mathies [:jimm] Reporter
	Comment 118 • 13 years ago

(In reply to comment #117)
> Why have you added in browser/browser.css:
> 
> #main-window[inFullscreen] #titlebar {
>   display: none;
> }
> 
> and not only changed in content/browser.css:
> 
> #main-window[inFullscreen] > #titlebar,
> #main-window[inFullscreen] .titlebar-placeholder,
> #main-window:not([tabsintitlebar]) .titlebar-placeholder {
>   display: none;
> }
> 
> This would be easier and not leave a unused selector.

ah, thanks. The xul structure changes broke that selector. Updated.

	Jim Mathies [:jimm] Reporter
	Comment 119 • 13 years ago

Dao asked me to check this. This looks comparable to fx without these patches. Roc, see anything here that looks out of the ordinary?

	Jim Mathies [:jimm] Reporter
	Comment 120 • 13 years ago

Added back in the bit of css that removes the footer image:

+  #browser-bottombox[lwthemefooter="true"] {
+    background-image: none !important;
+    background: none !important;
+  }

	Jim Mathies [:jimm] Reporter
	Comment 121 • 13 years ago

(In reply to comment #63)
> Created attachment 519169 [details] [diff] [review]
> theme work v.2

The Arc fill code in this regressed talos paint by about 50ms which is pretty bad. So I'm changing this up to use a poor man's arc via a set of three rectangles. Visually it looks the same (we clip the background then paint a rounded border on top) and it's fast.

	Jim Mathies [:jimm] Reporter
	Comment 122 • 13 years ago

(In reply to comment #121)
> (In reply to comment #63)
> > Created attachment 519169 [details] [diff] [review]
> > theme work v.2
> 
> The Arc fill code in this regressed talos paint by about 50ms which is pretty
> bad. So I'm changing this up to use a poor man's arc via a set of three
> rectangles. Visually it looks the same (we clip the background then paint a
> rounded border on top) and it's fast.

Note this only applies to glass. Windows handles the clipping when rendering themed window frames.

	Valerio
	Comment 123 • 13 years ago

Jim, I have tried this http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/jmathies@mozilla.com-809481a3522d/  but I noticed that when I have applied a Persona the four corner of the window are not round

	Valerio
	Comment 124 • 13 years ago

And the browser-bottombox, when I have a Persona applied, still have the border around it, as opposed to how it looks in the image on the right in this screenshot: https://bug590945.bugzilla.mozilla.org/attachment.cgi?id=522550

	Deb Richardson [:dria] (plz NEEDINFO)
	Comment 125 • 13 years ago

Do we know which release this is expected to land in yet?  I'm working on updating the docs and knowing whether this will be "Firefox 6+" or whatever would be useful.

	bogas04
	Comment 126 • 13 years ago

Its severe bug with current patches i guess

	Jim Mathies [:jimm] Reporter
	Comment 127 • 13 years ago

(In reply to comment #124)
> And the browser-bottombox, when I have a Persona applied, still have the border
> around it, as opposed to how it looks in the image on the right in this
> screenshot: https://bug590945.bugzilla.mozilla.org/attachment.cgi?id=522550

That was a test build with features disabled. I was searching for a talos paint regression.

	Jim Mathies [:jimm] Reporter
	Comment 128 • 13 years ago

(In reply to comment #125)
> Do we know which release this is expected to land in yet?  I'm working on
> updating the docs and knowing whether this will be "Firefox 6+" or whatever
> would be useful.

The cut off for Fx 6 is mid May so at this point Fx6 still seems plausible. 

ping: Dao! (for reviews)

	Jim Mathies [:jimm] Reporter
	Comment 129 • 13 years ago

tpaint regression fix

	Dão Gottwald [:dao]
	Comment 130 • 13 years ago

Comment on attachment 522470 [details] [diff] [review]
add clear to nsmargin v.1

this doesn't seem to apply (anymore?)

	Jim Mathies [:jimm] Reporter
	Comment 131 • 13 years ago

Comment on attachment 527854 [details] [diff] [review]
theme work v.3

Confirmed on try server the tpaint regression is fixed. Neil, same patch except for the clearing code. It's no longer using Arc.

	Jim Mathies [:jimm] Reporter
	Comment 132 • 13 years ago

updated to latest mc.

	Jim Mathies [:jimm] Reporter
	Comment 133 • 13 years ago

updated to latest. the only change here:

+  #browser-bottombox[lwthemefooter="true"] {

should have been:

+  #browser-bottombox:-moz-lwtheme {

	neil@parkwaycc.co.uk
	Comment 134 • 13 years ago

Comment on attachment 527854 [details] [diff] [review]
theme work v.3

If the only difference is some GFX stuff then maybe it would be better to get a GFX peer to review the change? It's not as if I have a system that I can actually test the change on...

	Jim Mathies [:jimm] Reporter
	Comment 135 • 13 years ago

Comment on attachment 527854 [details] [diff] [review]
theme work v.3

Joe, this has already been reviewed except for the gfxContext code in the middle that does the corner clearing. Curious if you would be able to look that over?

	Dão Gottwald [:dao]
	Comment 136 • 13 years ago

I still can't get this set of patches to apply. Not sure if there's just a lot of movement in these files or if I applied them in the wrong order, although I think I tried all possible combinations...

	Jim Mathies [:jimm] Reporter
	Comment 137 • 13 years ago

(In reply to comment #136)
> I still can't get this set of patches to apply. Not sure if there's just a lot
> of movement in these files or if I applied them in the wrong order, although I
> think I tried all possible combinations...

Hrm, bug 614720 and it's descendants broke all the widget and theme patches. Here's a roll up to 'settings changed notification v.1'. 'browser frame v.3' & 'status panel v.1' sit on top of that.

	[not reading bugmail]
	Comment 138 • 13 years ago

(In reply to comment #137)
> Created attachment 528632 [details] [diff] [review]
> rollup to settings changes patch
> 
> (In reply to comment #136)
> > I still can't get this set of patches to apply. Not sure if there's just a lot
> > of movement in these files or if I applied them in the wrong order, although I
> > think I tried all possible combinations...
> 
> Hrm, bug 614720 and it's descendants broke all the widget and theme patches.
> Here's a roll up to 'settings changed notification v.1'. 'browser frame v.3' &
> 'status panel v.1' sit on top of that.

bug 473687 specifically landed on 4.24

	Joe Drew (not getting mail)
	Comment 139 • 13 years ago

Comment on attachment 527854 [details] [diff] [review]
theme work v.3

Review of attachment 527854 [details] [diff] [review]:

Looks good. I've suggested some minor improvements, but I think this is all ok.

Note that I didn't review the Thebes code for correctness, just for API usage.

::: widget/src/windows/nsNativeThemeWin.cpp
@@ +1620,5 @@
+             nsUXThemeData::CheckForCompositor()) {
+    // Clear the corner regions underneath us where we don't want
+    // drawing to take place. This keeps the glass corners rounded
+    // by removing all content.
+    ctx->Save();

You can use gfxContextAutoSaveRestore here.

@@ +1625,5 @@
+    ctx->ResetClip();
+    ctx->Translate(tr.TopLeft());
+
+    gfxContext::GraphicsOperator currentOp = ctx->CurrentOperator();
+    ctx->SetOperator(gfxContext::OPERATOR_CLEAR);

Turns out you don't actually have to save the currentOp here, since Save() and Restore() do that for you.

(These two comments also apply to the if block below.)

@@ +1695,5 @@
+    ctx->Fill();
+    ctx->Restore();
+
+    // Clear the right corner
+    ctx->Save();

Restoring, then saving again, seems a little heavyweight here. You could just save the matrix before translating, then explicitly set it back right here.

	Dão Gottwald [:dao]
	Comment 140 • 13 years ago

Comment on attachment 523591 [details] [diff] [review]
status panel v.1

The border around the content area becomes opaque when I apply these patches.

	Dão Gottwald [:dao]
	Comment 141 • 13 years ago

Comment on attachment 527941 [details] [diff] [review]
browser frame v.3

Err, I meant to r- this patch with the previous comment.

As far as the status panel patch is concerned, it causes the status panel to be misaligned when maximizing the window.

	Jim Mathies [:jimm] Reporter
	Comment 142 • 13 years ago

(In reply to comment #140)
> Comment on attachment 523591 [details] [diff] [review] [review]
> status panel v.1
> 
> The border around the content area becomes opaque when I apply these patches.

Hmm, I'm not able to reproduce. (I also have the patches in bug 633282 applied underneath this.) Is this the border you're referring to Dao? If so it looks like the combination of the two sets addresses the problem.

(In reply to comment #141)
> Comment on attachment 527941 [details] [diff] [review] [review]
> browser frame v.3
> 
> Err, I meant to r- this patch with the previous comment.
> 
> As far as the status panel patch is concerned, it causes the status panel to
> be misaligned when maximizing the window.

I can reproduce this, looks like I need to update when the size mode changes. I'll work something up.

	Jim Mathies [:jimm] Reporter
	Comment 145 • 13 years ago

I broke these up a little differently this time - css and js changes. The js changes include the maximized/fullscreen fix for the status panel.

	Jim Mathies [:jimm] Reporter
	Comment 146 • 13 years ago

(In reply to comment #125)
> Do we know which release this is expected to land in yet?  I'm working on
> updating the docs and knowing whether this will be "Firefox 6+" or whatever
> would be useful.

Deb, since the merge is next tuesday and considering this will need a lot of bake time, I'm pushing this back to the 7.0 release.

	Ahmed Aly
	Comment 147 • 13 years ago

https://bugzilla.mozilla.org/show_bug.cgi?id=594318
is a duplicate of this bug

	Dão Gottwald [:dao]
	Comment 149 • 13 years ago

I got there by maximizing and restoring the window. The border stayed like this when moving the window (also offscreen), resizing the window fixed it.

	Dão Gottwald [:dao]
	Comment 150 • 13 years ago

Comment on attachment 533333 [details] [diff] [review]
browser css

All ids and -moz-appearance values containing "left" and "right" should say "start" and "end" instead, so that they make sense for RTL.

>+  #browser-bottombox:-moz-lwtheme {
>+    background-image: none !important;
>+    background: none !important;
>   }

background-image is redundant

>+  #main-window[sizemode="normal"] #window-frame-top-left-inner:-moz-lwtheme:-moz-locale-dir(ltr) {
>+    border-left: 2px solid;
>+    -moz-border-left-colors: @glassActiveBorderColor@ @glassBorderHighlightColor@;
>+  }
>+
>+  #main-window[sizemode="normal"] #window-frame-top-left-inner:-moz-lwtheme:-moz-locale-dir(rtl) {
>+    border-right: 2px solid;
>+    -moz-border-right-colors: @glassActiveBorderColor@ @glassBorderHighlightColor@;
>+  }

use -moz-border-start: 2px solid; here

>   #main-window[sizemode="normal"] #titlebar-buttonbox:-moz-lwtheme {
>     margin-top: -2px;
>   }
> 
>+  #main-window[sizemode="normal"] #titlebar-buttonbox:-moz-lwtheme:-moz-locale-dir(ltr) {
>+    margin-right: 1px;
>+  }
>+
>+  #main-window[sizemode="normal"] #titlebar-buttonbox:-moz-lwtheme:-moz-locale-dir(rtl) {
>+    margin-left: 1px;
>+  }

use -moz-margin-end

	Dão Gottwald [:dao]
	Comment 151 • 13 years ago

Comment on attachment 533335 [details] [diff] [review]
browser js

>+var FrameSettings = {
>+  handleEvent: function (event) {
>+    if (event.type == "resize") { 
>+      if (event.target != window)
>+        return;
>+      this.update();
>+    }
>+  },
>+
>+  init: function () {
>+    window.addEventListener("resize", this, false);
>+  },
>+
>+  uninit: function () {
>+    window.removeEventListener("resize", this, false);

I don't think this is needed.

>+  },
>+
>+  update: function () {
>+    this.updateFrameMarginSettings(this._chromeMarginsEnabled);
>+  },
>+
>+  updateFrameMarginSettings: function (chromemargin) {
>+    this._chromeMarginsEnabled = chromemargin;

Why do you need the chromemargin argument? Can't you just check the window's chromemargin attribute?

>+    let sm = document.documentElement.getAttribute("sizemode");
>+    if (sm == "maximized" || sm == "fullscreen") {
>+      let sp = document.getElementById("statusbar-display");
>+      sp.style.marginLeft = "0px";
>+      sp.style.marginRight = "0px";
>+      return;
>+    }
>+
>+    // setup the correct border radii on the corners of the window
>+    // based on the width/height of the frame. we also limit this
>+    // since there's a maximum radius Windows applies. 
>+    let maxRadius = 6;
>+    let isLTR = window.getComputedStyle(document.documentElement, null).direction ==
>+      "ltr" ? true : false;
>+
>+    let element = document.getElementById("window-frame-top-left-inner");
>+    let leftwidth = element.boxObject.width > maxRadius ? maxRadius : element.boxObject.width;
>+    if (isLTR)
>+      element.style.borderTopLeftRadius = leftwidth + "px";
>+    else
>+      element.style.borderTopRightRadius = leftwidth + "px";
>+
>+    element = document.getElementById("window-frame-top-right-inner");
>+    let rightwidth = element.boxObject.width > maxRadius ? maxRadius : element.boxObject.width;
>+    if (isLTR)
>+      element.style.borderTopRightRadius = rightwidth + "px";
>+    else
>+      element.style.borderTopLeftRadius = rightwidth + "px";
>+
>+    element = document.getElementById("window-frame-bottom-inner");
>+    let height = element.boxObject.height > maxRadius ? maxRadius : element.boxObject.height;
>+    element.style.borderBottomLeftRadius = height + "px";
>+    element.style.borderBottomRightRadius = height + "px";
>+
>+    // set the margin on the status panel equal to the frame
>+    // width.
>+    let sp = document.getElementById("statusbar-display");
>+    sp.style.marginLeft = (chromemargin ? leftwidth : 0) + "px";
>+    sp.style.marginRight = (chromemargin ? leftwidth : 0) + "px";

chromemargin ? leftwidth + "px" : "";

>+  },

You seem to be doing a lot of unnecessary work here for resize events that don't change the sizemode, i.e. when resizing a restored window.

	bogas04
	Comment 152 • 13 years ago

Jimm/Dao , can you push this to UX branch for testing?

	[not reading bugmail]
	Comment 153 • 13 years ago

Hi Jim, would this code still allow the theme/appearance look and act more like Zune Software or iTunes, meaning no or minimal visible frameborders, custom min/max/close buttons, etc?  It seems to me that we could go that direction with Personas skinning the entire window to look more modern.

	Thomas Gladdines
	Comment 154 • 13 years ago

@Dennis,
I think we can't skin the captions with this since this code only modifies the borders and not the content atm.
But it fixes all the bugs found with larger or smaller border padding.

	Asa Dotzler [:asa]
	Comment 155 • 13 years ago

Jim, Dao, how close are we on this. We're about to wrap up Firefox 7 development period and it would be really nice to get this (bug 618353) in before the cut-over to aurora.

	Jim Mathies [:jimm] Reporter
	Comment 156 • 13 years ago

(In reply to comment #155)
> Jim, Dao, how close are we on this. We're about to wrap up Firefox 7
> development period and it would be really nice to get this (bug 618353) in
> before the cut-over to aurora.

Making the next train (July 5th) is questionable. I'm currently working on the updates now though so we shall see how it goes.

	Jim Mathies [:jimm] Reporter
	Comment 157 • 13 years ago

(In reply to comment #150)
> Comment on attachment 533333 [details] [diff] [review] [review]
> browser css
> 
> All ids and -moz-appearance values containing "left" and "right" should say
> "start" and "end" instead, so that they make sense for RTL.

Should I also update the naming for window-frame-left & window-frame-right, which are already in mc?

	Dão Gottwald [:dao]
	Comment 158 • 13 years ago

Those aren't used anywhere yet, are they? Anyway, sounds like they should be updated.

	Jim Mathies [:jimm] Reporter
	Comment 159 • 13 years ago

(In reply to comment #158)
> Those aren't used anywhere yet, are they? Anyway, sounds like they should be
> updated.

Went ahead and did that along with the other changes.

I currently can't reproduce that maximized -> normal bug in the bottom border. I'll push all the patches to try to generate a release build for testing.

	Jim Mathies [:jimm] Reporter
	Comment 163 • 13 years ago

https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jmathies@mozilla.com-18a7363d77a3/

	Jim Mathies [:jimm] Reporter
	Comment 164 • 13 years ago

(In reply to comment #149)
> Created attachment 537989 [details]
> screenshot of broken left border
> 
> I got there by maximizing and restoring the window. The border stayed like
> this when moving the window (also offscreen), resizing the window fixed it.

Dao, would you mind testing with that try build? I've been playing with restore-maximise and don't see the issue.

Also, were you running a theme or persona in this screen shot? Your titlebar looks to be themed, but the glass on the side is not.

	Dão Gottwald [:dao]
	Comment 165 • 13 years ago

> Also, were you running a theme or persona in this screen shot?

A persona.

> Your titlebar looks to be themed, but the glass on the side is not.

That's exactly what I was complaining about.

	Jim Mathies [:jimm] Reporter
	Comment 166 • 13 years ago

I didn't find the theme you were using, but I found one that has the same side effect on the left side. There's not much we can do here though, in this case the developer themed the #titlebar via css. Fixing this requires changes to the theme. I believe the add-on site has version checks, so themes not compatible would be disabled on the site. (Deb could probably confirm this.)

#titlebar{
	box-shadow:0 -1px rgba(178,111,0,.5) inset;
	border-top:2px solid;
	border-bottom:2px solid;
	-moz-border-top-colors:#B26F00 #EED4A2;
	-moz-border-bottom-colors:#EED4A2 #B26F00;
	background-image:url(chrome://global/skin/hout-hover.png)
}

	Jim Mathies [:jimm] Reporter
	Comment 167 • 13 years ago

(In reply to comment #165)
> > Also, were you running a theme or persona in this screen shot?
> 
> A persona.
> 
> > Your titlebar looks to be themed, but the glass on the side is not.
> 
> That's exactly what I was complaining about.

Could you point me to the persona you were running?

	Dão Gottwald [:dao]
	Comment 168 • 13 years ago

It's the fifth from the right on <http://design-noir.de/mozilla/themes/>. There's no way for this bug to be persona-dependent, though.

	Jim Mathies [:jimm] Reporter
	Comment 169 • 13 years ago

(In reply to comment #168)
> It's the fifth from the right on <http://design-noir.de/mozilla/themes/>.
> There's no way for this bug to be persona-dependent, though.

Hmm, that's working fine here, Are you still seeing this in the try build I posted?

	Ahmed Aly
	Comment 170 • 13 years ago

just tried the try build. its perfect

	Csaba Kozák [:WonderCsabo]
	Comment 171 • 13 years ago

I tried the try build with fresh profile. After maximizing the window with maximizing button, all of the caption buttons disappear. Can i file a new bug?

	bogas04
	Comment 172 • 13 years ago

Painting the browser on resizing is slow , it feels laggy and twitchy. Will it be fixed in this bug or some other work around bug would be created?

	Jim Mathies [:jimm] Reporter
	Comment 173 • 13 years ago

(In reply to comment #171)
> I tried the try build with fresh profile. After maximizing the window with
> maximizing button, all of the caption buttons disappear. Can i file a new
> bug?

What operating system and desktop were you testing on?

	Csaba Kozák [:WonderCsabo]
	Comment 174 • 13 years ago

Win7 x64, default theme with Aero Glass. But i cannot reproduce the problem. :S

BTW, while resizing/maximizing, all glass surfaces became black. It is annoying and very ugly. Also, i can confirm bogas' report: resizing is very laggy.

	Jim Mathies [:jimm] Reporter
	Comment 175 • 13 years ago

(In reply to comment #174)
> Win7 x64, default theme with Aero Glass. But i cannot reproduce the problem.
> :S
> 
> BTW, while resizing/maximizing, all glass surfaces became black. 

We have that problem currently in the titlebar. It's unrelated to these patches. It has to do with timing in setting glass surfaces and the repaint.

	Csaba Kozák [:WonderCsabo]
	Comment 176 • 13 years ago

(In reply to comment #175)
> (In reply to comment #174)
> > Win7 x64, default theme with Aero Glass. But i cannot reproduce the problem.
> > :S
> > 
> > BTW, while resizing/maximizing, all glass surfaces became black. 
> 
> We have that problem currently in the titlebar. It's unrelated to these
> patches. It has to do with timing in setting glass surfaces and the repaint.

Yes, you are right. Is there a bug about it?
And what about laggy resizing? It is indeed came with this patch.

	Ahmed Aly
	Comment 177 • 13 years ago

Win7 x64, modded theme with Aero Glass.
resizing causes pc ui unresponsive lags with aero,,
the problem didnt happen wearing a persona which was how i tested first time
couldn't reproduce the maximizing button issue

	Csaba Kozák [:WonderCsabo]
	Comment 178 • 13 years ago

OK, i can clearly reproduce the maximizing button issue. STR:

1. Maximize/demaximize the window with the button
2. Do the same with Aero Snap
3. Repeat the first two steps randomly until the maximizing button disappear

	HRipper
	Comment 179 • 13 years ago

(In reply to comment #178)
> OK, i can clearly reproduce the maximizing button issue. STR:
> 
> 1. Maximize/demaximize the window with the button
> 2. Do the same with Aero Snap
> 3. Repeat the first two steps randomly until the maximizing button disappear

I'm using Soft7 theme and it works for me.

	Thomas Gladdines
	Comment 180 • 13 years ago

When I hover a button or open the bookmarks sidepane it shows the titlebar drawn by dwm instead of firefox...
Could that be fixed so the titlebar is always drawn by firefox?

	Deb Richardson [:dria] (plz NEEDINFO)
	Comment 181 • 13 years ago

Did this make the cutoff for Fx7? I'm a little confused :)

	Asa Dotzler [:asa]
	Comment 182 • 13 years ago

(In reply to comment #181)
> Did this make the cutoff for Fx7? I'm a little confused :)

Nope. It'll have to wait until Firefox 8 gets uplifted.

	Jim Mathies [:jimm] Reporter
	Comment 183 • 13 years ago

I think the perceived resizing performance may just be the persona on the edge tricking you into thinking things are slower. I don't see a noticeable difference between these patches and a nightly. But the resize performance of the window is made more apparent when the frame is textured with the persona.

Note in testing, please check to see if you have hardware acceleration enabled through the options. Resizing with acceleration enabled is much slower than with gdi. If you see performance issues, please note whether or not acceleration is enabled and whether or not you see the same problem with gdi.

Also, please do not post bug reports here about try builds if you are running a non-ms sanctioned Windows theme. Any problems with these themes will be handled in separate bugs.

There are two bugs with these patch that I know of currently -

1) w/hardware acceleration enabled including d3d9/d3d10 layers, resizing the window to large dimensions can cause a redraw ('flash') problem with the window. I've been trying to track this down.

2) On XP, maximize/restore can result in content not getting positioned correctly, independent of acceleration settings.

I'll post new try builds as I make progress.

	bogas04
	Comment 184 • 13 years ago

Whats the status of the new patch? Waiting for it to land to UX build for testing!

	Jim Mathies [:jimm] Reporter
	Comment 185 • 13 years ago

(In reply to comment #184)
> Whats the status of the new patch? Waiting for it to land to UX build for
> testing!

Bug 672885 is a hard blocker on these patches landing.

	Javier "Darth"
	Comment 189 • 12 years ago

I've posted Bug 730219 that maybe is related to these changes. (see also 'Related info' there)

Jim, 
I can't find your last try-build for this bug. Searched a lot in ftp and hg (for keyword).

Thanks everybody for your work.