Persona theme with disabled menu bar completely covers aero caption buttons (no minimize, maximize, close buttons)

VERIFIED FIXED in Firefox 4.0b5

Status

()

Firefox
Theme
P2
major
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: imradyurrad, Assigned: Felipe)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug, {regression})

Trunk
Firefox 4.0b5
x86
Windows 7
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-litmus +

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [Input], URL)

Attachments

(11 attachments, 4 obsolete attachments)

37.02 KB, image/jpeg
Details
186.69 KB, image/png
Details
11.40 KB, text/plain
Details
414.88 KB, application/zip
Details
139.64 KB, image/png
Details
110.41 KB, image/png
Details
110.54 KB, image/png
Details
603.73 KB, image/png
beltzner
: ui-review+
faaborg
: feedback+
Details
15.11 KB, patch
jimm
: review+
Details | Diff | Splinter Review
351.50 KB, image/png
Details
36.93 KB, image/jpeg
Details
(Reporter)

Description

7 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a6pre) Gecko/20100625 Minefield/3.7a6pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a6pre) Gecko/20100625 Minefield/3.7a6pre

With the latest nightly, installing a lightweight persona theme will result in the window caption buttons being completely covered. 


Reproducible: Always

Steps to Reproduce:
1. Install persona
2. Persona covers caption buttons

Actual Results:  
Caption buttons are covered by persona theme.

Expected Results:  
Caption buttons should still be visible and functioning.
(Reporter)

Comment 1

7 years ago
Created attachment 454157 [details]
screenshot

Updated

7 years ago
Blocks: 513162
Component: Theme → Widget: Win32
Keywords: regression
Product: Firefox → Core
QA Contact: theme → win32
(Reporter)

Updated

7 years ago
(Reporter)

Updated

7 years ago
Summary: Installing Persona theme completely overs the caption buttons → Installing Persona theme completely covers the caption buttons
This is when menu button is active

I think it will be desired to draw the persona to the edge eventually, but the buttons should be on top/show instead.
(Reporter)

Updated

7 years ago
Depends on: 574739
(Reporter)

Comment 3

7 years ago
(In reply to comment #2)
> This is when menu button is active
> 
> I think it will be desired to draw the persona to the edge eventually, but the
> buttons should be on top/show instead.

You can still perform the functions if you hover over them. Also, in Vista/7 the hover glow for the caption buttons extend past the top border so it's kind of visible.
Confirming they are there, since they glow and clickable, the persona is drawn on top, the rest of the widgets and menu bar are drawn below.
(Reporter)

Updated

7 years ago
Severity: normal → critical
(Reporter)

Updated

7 years ago
Version: unspecified → Trunk

Comment 5

7 years ago
confirmed. We paint right over them.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 6

7 years ago
Is there a proposed fixed for this and is it possible to paint around the caption buttons?

Updated

7 years ago
Duplicate of this bug: 574975
Also, it's necessary to draw the upper window border too.
In the build i'm using (Mozilla/5.0 (Windows; U; Windows NT 6.1; es-ES; rv:1.9.3a6pre) Gecko/20100627 Minefield/3.7a6pre ID:20100627035822), the buttons don't work and glow at all! I need to get back to default theme.
(In reply to comment #8)
> buttons don't work and glow at all! I need to get back to default theme.

Not sure what you meant ;), but technically this would not block functionality of going to Addon manager and checking enable the default theme.
blocking2.0: --- → beta2+
Whiteboard: relnote
Keywords: relnote
Whiteboard: relnote

Updated

7 years ago
No longer depends on: 574739

Updated

7 years ago
Blocks: 513170

Updated

7 years ago
No longer blocks: 513162
(Reporter)

Comment 10

7 years ago
Since Bug 555987 landed, clicking on the covered caption buttons no longer does anything. The button glows are also gone.
(In reply to comment #10)
> Since Bug 555987 landed, clicking on the covered caption buttons no longer does
> anything. The button glows are also gone.

I was thinking it might be the full screen changes bug 575044 but need to review the builds to verify since bug 555987 is just to fix the core bug for keeping the toolbars in order.

Updated

7 years ago
Duplicate of this bug: 575865

Updated

7 years ago
Duplicate of this bug: 575554

Updated

7 years ago
Duplicate of this bug: 575908
Duplicate of this bug: 575837
Severity: critical → major
Maybe we should update the summary to reflect the specificness to the window controls min/max/close are not showing/usable since this will also be relnoted.

Updated

7 years ago
Duplicate of this bug: 575977

Updated

7 years ago
Duplicate of this bug: 575715

Updated

7 years ago
Duplicate of this bug: 576195

Updated

7 years ago
Duplicate of this bug: 576294

Updated

7 years ago
Whiteboard: [input]
Whiteboard: [input] → [Input]

Updated

7 years ago
Duplicate of this bug: 576747

Updated

7 years ago
Duplicate of this bug: 576717
(Reporter)

Updated

7 years ago
Priority: -- → P2
(Reporter)

Updated

7 years ago
Blocks: 575406

Comment 23

7 years ago
Seriously this makes themes useless -.-"
Why isn't this fixed?
(In reply to comment #23)
> Seriously this makes themes useless -.-"
> Why isn't this fixed?

Because no one has time to work on it, its relatively new and has been targeted for a fix for a future beta milestone.  Its an unfortunate regression from redesigning the app for other features.  If you know how to fix it, please feel free to help.  

In the meantime, if you want to use personas, enable the Menu Bar.

Updated

7 years ago
Duplicate of this bug: 577420

Updated

7 years ago
Duplicate of this bug: 577414

Updated

7 years ago
Duplicate of this bug: 577425

Updated

7 years ago
Depends on: 574454
Summary: Installing Persona theme completely covers the caption buttons → Persona theme with disabled menu bar completely covers caption buttons (no minimize, maximize, close buttons)
FYI, but we have quite a number of people on input saying they're seeing this problem: http://input.mozilla.com/search/?q=personas+minimize&product=firefox

Comment 29

7 years ago
Isn't there a way to disable Persona and use the default theme? It looks like the default theme is disabled on 4.0b1...

Comment 30

7 years ago
(In reply to comment #29)
> Isn't there a way to disable Persona and use the default theme? It looks like
> the default theme is disabled on 4.0b1...

Im on Beta 1 as well, and you can put it back to default

1) Click the orange Fire Fox Button
2) Customize
3) Choose Menu Bar
4) Click Tools then Add-ons
5) Choose Themes
6) Click any other installed theme, let it change then choose Default again.

After doing this, the "Default" theme will have an "Enable" button beside it like the others.

Comment 31

7 years ago
When you enable the menu bar, (Firefox -> Customize -> Menu Bar), the proper window framing is restored with the min, max, close buttons available.

Updated

7 years ago
Duplicate of this bug: 577888
Duplicate of this bug: 577183

Updated

7 years ago
Duplicate of this bug: 578728

Updated

7 years ago
Duplicate of this bug: 578729
(Reporter)

Comment 36

7 years ago
How come the firefox button isn't effected by this?

Comment 37

7 years ago
(In reply to comment #36)
> How come the firefox button isn't effected by this?

The fx button is drawn in content by fx, the caption buttons are drawn by the dwm.

Comment 38

7 years ago
There are, of course, other issues here. Enabling the menu bar means I lose the Minefield button. Introduce a persona with the menu bar enabled and a new tab keeps the title 'new tab' even when the page has loaded, (Bug 57918?)
blocking2.0: beta2+ → betaN+
Assignee: nobody → jmathies

Updated

7 years ago
Assignee: jmathies → nobody
Component: Widget: Win32 → Theme
Product: Core → Firefox
QA Contact: win32 → theme

Comment 39

7 years ago
We can't fix this in widget, we don't have access to the graphics. We'll have to come up with something in our theming to address it.
Jimm: I'm sorry, I'm not sure I understand this comment; you're saying that we'll need to draw this in the Windows theme in order for it to work? What needs to be done on a theming side?
Moving this to beta3+ for now so that we can at least understand what our solution is going to have to be for this; might move again!
blocking2.0: betaN+ → beta3+

Comment 42

7 years ago
(In reply to comment #40)
> Jimm: I'm sorry, I'm not sure I understand this comment; you're saying that
> we'll need to draw this in the Windows theme in order for it to work? What
> needs to be done on a theming side?

Yes, I think if we want these buttons on top of personas, we'll have to draw them ourselves. We don't have control over the layering, and we don't have access to the graphics since the buttons are apparently rendered on the GPU using 3d effects.

We have a few options really, draw buttons that look just like the dwm buttons, and leave the dwm buttons enabled. This way we get the off window glow effects without having to render anything. Alternatively we can disable the default buttons and glow and just draw our own custom caption buttons. Personas folks could then customize these.

Comment 43

7 years ago
(In reply to comment #42)
> (In reply to comment #40)
> > Jimm: I'm sorry, I'm not sure I understand this comment; you're saying that
> > we'll need to draw this in the Windows theme in order for it to work? What
> > needs to be done on a theming side?
> 
> Yes, I think if we want these buttons on top of personas, we'll have to draw
> them ourselves. We don't have control over the layering, and we don't have
> access to the graphics since the buttons are apparently rendered on the GPU
> using 3d effects.
> 
> We have a few options really, draw buttons that look just like the dwm buttons,
> and leave the dwm buttons enabled. This way we get the off window glow effects
> without having to render anything. Alternatively we can disable the default
> buttons and glow and just draw our own custom caption buttons. Personas folks
> could then customize these.

I think that's a good idea, even though it makes me think of google chrome when the themes are on. still, it's further customization options for the personas people, which is always good.

Updated

7 years ago
Summary: Persona theme with disabled menu bar completely covers caption buttons (no minimize, maximize, close buttons) → Persona theme with disabled menu bar completely covers aero caption buttons (no minimize, maximize, close buttons)

Comment 44

7 years ago
(In reply to comment #42)
> (In reply to comment #40)
> > Jimm: I'm sorry, I'm not sure I understand this comment; you're saying that
> > we'll need to draw this in the Windows theme in order for it to work? What
> > needs to be done on a theming side?
> 
> Yes, I think if we want these buttons on top of personas, we'll have to draw
> them ourselves. We don't have control over the layering, and we don't have
> access to the graphics since the buttons are apparently rendered on the GPU
> using 3d effects.
> 
> We have a few options really, draw buttons that look just like the dwm buttons,
> and leave the dwm buttons enabled. This way we get the off window glow effects
> without having to render anything. Alternatively we can disable the default
> buttons and glow and just draw our own custom caption buttons. Personas folks
> could then customize these.

If we go with option one, I do have have access to a bounds rect through the DwmGetWindowAttribute api. So positioning might not be too much of a headache.
It's not just the caption buttons but also the window border.

(In reply to comment #43)
> still, it's further customization options for the personas
> people, which is always good.

We don't really want this. Personas are lightweight themes with emphasis on "lightweight" -- people should basically drop in an image and be done with it.

Comment 46

7 years ago
(In reply to comment #45)
> It's not just the caption buttons but also the window border.
> 

I'm not sure where we'll handle that, we might be able to use a clipping region down in widget to get the rounded corners, or maybe there is something we can do in css to do the same?
(In reply to comment #45)
> It's not just the caption buttons but also the window border.
> 
> (In reply to comment #43)
> > still, it's further customization options for the personas
> > people, which is always good.
> 
> We don't really want this. Personas are lightweight themes with emphasis on
> "lightweight" -- people should basically drop in an image and be done with it.

I was thinking this too, but I cannot speak for anything technical, but wall paper should be wall paper if at all possible.
Adding Ryan: please look at comment 42 and onwards, I bet you'll have opinions!
I agree with Dão and Dale, customizing OS buttons is beyond the scope of personas. For now they should be just background images.
Jim, do we need to draw personas up through the custom titlebar, or can we make it bound below that zone? 

if so would that at least make us look consistent on the menu bar vs menu button visual look vs non aero theming and might alleviate some of the issues for now, no?

Comment 51

7 years ago
(In reply to comment #49)
> I agree with Dão and Dale, customizing OS buttons is beyond the scope of
> personas. For now they should be just background images.

I have no preference either way, this was one of the possible customizations we wanted to add based on the initial comments in bug 513162 though. independent of personas, we'll have to render & position caption buttons on non-glass desktops anyway.

(In reply to comment #50)
> Jim, do we need to draw personas up through the custom titlebar, or can we make
> it bound below that zone? 
> 
> if so would that at least make us look consistent on the menu bar vs menu
> button visual look vs non aero theming and might alleviate some of the issues
> for now, no?

That's another option, we can clip the texture so that it looks like a normal glass titlebar if we want. We could skip adding the background attribute on the new frame we're going to have to build in bug 575870. Then we'd have a normal window frame without the texture, and the persona would sit inside the client area as it does now. This will also need to mesh well with the new custom xp skins we want to do as well.
What happens whith custom windows themes ?

Comment 53

7 years ago
(In reply to comment #52)
> What happens whith custom windows themes ?

We would only customize parts of themes we recognize and let windows render everything else via the theme library. Look at it this way - we create our own window frame, and let the theme library render it. If we detect a theme we want to customize, we swap out parts of it with our own graphics. For example:

https://wiki.mozilla.org/images/c/c1/Firefox-4-Mockup-i06-%28XP%29-%28LunaBlue%29-%28TabsTop%29.png

Updated

7 years ago
Duplicate of this bug: 577144

Updated

7 years ago
Duplicate of this bug: 580831

Updated

7 years ago
Duplicate of this bug: 580852

Comment 57

7 years ago
Any chance we can make a decision on this soonish? The resolution to this bug effects other open bugs and effects what we'll be working on over the next few weeks. There are two issues that revolve around this:

1) caption buttons on glass desktops. to do full skinning, we will need to position and render the buttons manually. We can use custom buttons (al la Google Chrome) or we can try and mimic the existing glass buttons and glow. Either way these graphics need to be painted by us.

2) clipping on window frame corners. (bug 575406) If we do full skinning, this will need to be addressed, probably down in widget where we can set clipping regions on the corners. It also might be possible to handle this up in content using border radii or some other css trick that leaves the corners transparent.

If we decide to do full skinning for fx 4.0, the above two issues need to be addressed. If we decide to constrain textures within the window frame, neither of these issues needs to be addressed. The decision here also effects glass colorization for privacy mode (bug 513418), which will be afflicted with the same two issues personas has here.
For the best experience I think we would definitely want to be able to draw personas to the edge of the window (on all sides).
Can I vote for full skinning?
(In reply to comment #58)
> For the best experience I think we would definitely want to be able to draw
> personas to the edge of the window (on all sides).

Agreed, it would also bring it to parity with OSX's full window skinning.

Comment 61

7 years ago
If we see how Chrome (browser) handles it , we can get some help
Chrome , when using a theme (non aero) , doesn't use Windows' caption buttons ,instead it creates new buttons itself , so can't we do the same thing with Firefox?
(Reporter)

Comment 62

7 years ago
(In reply to comment #61)
> If we see how Chrome (browser) handles it , we can get some help
> Chrome , when using a theme (non aero) , doesn't use Windows' caption buttons
> ,instead it creates new buttons itself , so can't we do the same thing with
> Firefox?

We can but the question is whether or not we should. Personally, I don't think we should mess with them at all. We should stick to the mockups:
https://bug513170.bugzilla.mozilla.org/attachment.cgi?id=397196
But there's no Firefox button in those! I'd thought it would remain for Firefox 4?
(Reporter)

Comment 64

7 years ago
(In reply to comment #63)
> But there's no Firefox button in those! I'd thought it would remain for Firefox
> 4?

They're old mockups.
The old ones are the ones above or the ones with the Firefox button?

Updated

7 years ago
Duplicate of this bug: 581802

Comment 67

7 years ago
(In reply to comment #57)
> 1) caption buttons on glass desktops. to do full skinning, we will need to
> position and render the buttons manually. We can use custom buttons (al la
> Google Chrome) or we can try and mimic the existing glass buttons and glow.
> Either way these graphics need to be painted by us.

I think it is better to get the position of the caption buttons with WM_GETTITLEBARINFOEX and then clip painting there or use an alpha channel value of ~0.3 in that region (depending on whether the window is focused or not). Then let windows draw min/max/close buttons for us. This way we get the glow-effect for free (and those custom min/max/close buttons look horrible in google chrome).
I noticed the 4b2 relnotes still say the button still work, I think they used to as a previous comments said.  But actually, right now Clicking on the areas only do nothing and double-clicking actually maximizes the browser.  There are no clicks that work to minimize/maximize/close the window anymore.

Updated

7 years ago
Duplicate of this bug: 582582
This is a beta three blocker but has no owner - do we really think it's going to make it?

Comment 71

7 years ago
(In reply to comment #70)
> This is a beta three blocker but has no owner - do we really think it's going
> to make it?

This depends on bug 574454, which is blocking 'betaN'. Also, it probably can't be addressed until we complete the work in bug 575870, which has the blocking 2.0 request flag set.

I think there was some hope that this could be addressed down in widget, but it can't.

I would mark all three of these bugs as blocking beta 4 so we can push this work to completion over the next few weeks.
OK, bug 574454 looks like it's basically ready to go, and I marked bug 575870 as betaN+ - should you be the owner of that one as well? Do you need additional support from Dao or Stephen? What will be left to do on this bug with those other two fixed?

I'm moving this to betaN+ - I take it that we shouldn't expect to see it done by Monday, Aug 2?

Comment 73

7 years ago
(In reply to comment #72)
> OK, bug 574454 looks like it's basically ready to go, and I marked bug 575870
> as betaN+ - should you be the owner of that one as well? Do you need additional
> support from Dao or Stephen? 

I need to chat with Dao or Gavin on that. I think it's going to involve some some pretty hefty changes to browser.xul.

> What will be left to do on this bug with those
> other two fixed?

Well, we need to decide how we want to fix this. See comment 57.

> 
> I'm moving this to betaN+ - I take it that we shouldn't expect to see it done
> by Monday, Aug 2?

I'd say it probably won't be solved until bug 575870 is finished up. I think we need to get it done and into a beta though considering the changes involved. (That work involves rendering the entire window frame and caption controls in xul.)

Updated

7 years ago
Duplicate of this bug: 582853
Jimm: Maybe I'm new to the terminology, but comment 57 asks if we want to do "full skinning" or not, and I'm not clear on what the implications are on that.

Shorlander/Faaborg - looks like we need to make a call here, and quick.

Dao/Gavin - the phrase "some hefty changes to browser.xul" makes me a little nervous; are you guys tracking this, and do you feel like you're on top of it?
Keywords: uiwanted
Full skinning means displaying Backgrounds in Title Bar as well.

Comment 77

7 years ago
(In reply to comment #75)
> Jimm: Maybe I'm new to the terminology, but comment 57 asks if we want to do
> "full skinning" or not, and I'm not clear on what the implications are on that.
> 

Full skinning would involve customizing the titlebar and border areas with graphics we produce. A number of the mock ups include this. That work would require creating the entire window frame in xul.

> Shorlander/Faaborg - looks like we need to make a call here, and quick.
> 
> Dao/Gavin - the phrase "some hefty changes to browser.xul" makes me a little
> nervous; are you guys tracking this, and do you feel like you're on top of it?

FWIW, I'm working on a less intrusive browser patch that just adds support for the fx button on non-aero desktops with default theming of the titlebar. That should be less invasive than full skinning since it won't require a full frame. I'll be posting that to bug 575870 here today or tomorrow.

Comment 78

7 years ago
As far as the personas/caption button problem goes, we need to decide how we want personas skins to mesh with the new titlebar. The hope was that the caption buttons drawn on glass would layer on top of anything we paint to glass areas. It turns out that's not the case.

Let me try to summarize:

1) full skinning: personas background texture covers the entire window, including the glass chrome* or themed chrome*.

- need to implement a full window frame in xul.
- need to solve the problem of caption buttons on glass, they are drawn under the texture.
- themed desktop caption buttons are not an issue, we can render those ourselves in xul as part of the xul window frame.
- need to solve window corner clipping problems.

2) partial skinning:

- same as we have now in 3.6, personas skins are restricted to the client area of the window.
- should "just work" with the titlebar work I'm trying to put together. We apply the persona texture only to the content below the new titlebar/fx button area.


* chrome == default Windows window chrome, not our browser chrome.
It seems like we should get 2) working as Jim's focusing on enabling this anyway for other settings, and then look into how involved 1) exactly would be and whether the listed problems can be solved.

Comment 80

7 years ago
(In reply to comment #78)
> As far as the personas/caption button problem goes, we need to decide how we
> want personas skins to mesh with the new titlebar. The hope was that the
> caption buttons drawn on glass would layer on top of anything we paint to glass
> areas. It turns out that's not the case.

As I said in comment #67, we could just clip the region, where those buttons are. I don't know how hard this is to do in firefox, but i have written a little test app in C with Cairo + win32-api, that shows how it would work. I will attach a screenshot + the sourcecode + the exe

Comment 81

7 years ago
Created attachment 461225 [details]
screenshot of my testapp

Comment 82

7 years ago
Created attachment 461227 [details]
sourcecode of the testapp

Comment 83

7 years ago
Created attachment 461229 [details]
the testapp itself and the backgound image

Comment 84

7 years ago
(In reply to comment #80)
> (In reply to comment #78)
> > As far as the personas/caption button problem goes, we need to decide how we
> > want personas skins to mesh with the new titlebar. The hope was that the
> > caption buttons drawn on glass would layer on top of anything we paint to glass
> > areas. It turns out that's not the case.
> 
> As I said in comment #67, we could just clip the region, where those buttons
> are. I don't know how hard this is to do in firefox, but i have written a
> little test app in C with Cairo + win32-api, that shows how it would work. I
> will attach a screenshot + the sourcecode + the exe

Hey peter, thanks for the example. How did you handle button glow when hovering over one of the buttons?

Comment 85

7 years ago
(In reply to comment #84)
> Hey peter, thanks for the example. How did you handle button glow when hovering
> over one of the buttons?

I let Windows draw the buttons and just do not paint where they apear. I do not draw these buttons myself.

Comment 86

7 years ago
(In reply to comment #85)
> (In reply to comment #84)
> > Hey peter, thanks for the example. How did you handle button glow when hovering
> > over one of the buttons?
> 
> I let Windows draw the buttons and just do not paint where they apear. I do not
> draw these buttons myself.

This is great. Definitely one of the possible options with full skinning.  Could you snap a screen shot of the window with the minimize button highlighted? I'm curious how the button highlighting looks with your texture.

Comment 87

7 years ago
Created attachment 461234 [details]
screenshot of test app with highlighted minimize button

Comment 88

7 years ago
(In reply to comment #87)
> Created attachment 461234 [details]
> screenshot of test app with highlighted minimize button

So the glow doesn't extend to the repainted areas below and to the left give it a slightly unnatural view of glowing on 2 of 4 sides.

Comment 89

7 years ago
(In reply to comment #88)
> So the glow doesn't extend to the repainted areas below and to the left give it
> a slightly unnatural view of glowing on 2 of 4 sides.

maybe this could be addressed by _smoothly_ changing the alpha value around those buttons, but I have not tested this yet...

Comment 90

7 years ago
(In reply to comment #88)
> (In reply to comment #87)
> > Created attachment 461234 [details] [details]
> > screenshot of test app with highlighted minimize button
> 
> So the glow doesn't extend to the repainted areas below and to the left give it
> a slightly unnatural view of glowing on 2 of 4 sides.

It's not a bad tradeoff though. Peter, any chance we could see the same thing with a darker texture to get a better feel for how it would look with a dark persona?

Comment 91

7 years ago
Created attachment 461242 [details]
slightly modified test app with dark texture

I have adjusted the lines 284..287 to use an offset of 0.5 instead of 1, so now there is a little white/lighter halo visible around the caption buttons:

cairo_move_to(     cr, buttons.left+0.5,   buttons.top);
cairo_arc_negative(cr, buttons.left+0.5  + buttonradius, buttons.bottom-0.5 - buttonradius, buttonradius, M_PI, M_PI/2);
cairo_arc_negative(cr, buttons.right-0.5 - buttonradius, buttons.bottom-0.5 - buttonradius, buttonradius, M_PI/2, 0);
cairo_line_to(     cr, buttons.right-0.5,  buttons.top);
(In reply to comment #88)
> (In reply to comment #87)
> > Created attachment 461234 [details] [details]
> > screenshot of test app with highlighted minimize button
> 
> So the glow doesn't extend to the repainted areas below and to the left give it
> a slightly unnatural view of glowing on 2 of 4 sides.

I bet we could fake it ourselves with CSS's box-shadow.

Comment 93

7 years ago
Confirming: http://yfrog.com/n974531561j.
Since the buttons work without aero, I think bug 582631 is of interest.
(Reporter)

Comment 94

7 years ago
(In reply to comment #93)
> Confirming: http://yfrog.com/n974531561j.
> Since the buttons work without aero, I think bug 582631 is of interest.

Dupe of Bug 575870

Comment 95

7 years ago
It look so much better with the normal windows chrome though. The area where you switch from normal chrome to personas looks horrid.

This would make me stop using personas. 

Can't you stop painting personas where the windows-chrome begins?
Which element of Persona in Title Bar is horrid?

Comment 97

7 years ago
(In reply to comment #95)
> It look so much better with the normal windows chrome though. The area where
> you switch from normal chrome to personas looks horrid.
> 
> This would make me stop using personas. 
> 
> Can't you stop painting personas where the windows-chrome begins?

A good solution would be to ask the user when he changes the persona or first launches ff4 whether the background image should extend to the window border or reside in the client area.

Comment 98

7 years ago
(In reply to comment #97)
> A good solution would be to ask the user when he changes the persona or first
> launches ff4 whether the background image should extend to the window border or
> reside in the client area.

Personas are supposed to be dead simple wallpapers. We shouldn't be prompting users about anything here. A pref (hidden) somewhere might make sense, but it may not be worth it.
(Reporter)

Comment 99

7 years ago
@Peter That seems rather excessive for the average user. The options menu is gonna be cluttered enough as it is. I say we follow the mockups and extend to the title bar.

Comment 100

7 years ago
Extending personas into the title bar seems to be necessary if just because we're cutting chrome space elsewhere and will need the extra line to make showing a persona worth it.
Another thing is the scenario when tabs will be in title bar. If Persona woll not be drawed in title bar, then persona will be aplied only to location bar and personal bar. That would look horrid.
(Reporter)

Comment 102

7 years ago
(In reply to comment #101)
> Another thing is the scenario when tabs will be in title bar. If Persona woll
> not be drawed in title bar, then persona will be aplied only to location bar
> and personal bar. That would look horrid.

You're exaggerating. By default, tabs in titlebar will just be an option. As for the personas - so we'd lose ~5px of vertical height. Not a big deal. I doubt users just sit and stare at the persona all day.

Comment 103

7 years ago
(In reply to comment #102)

> 
> You're exaggerating. By default, tabs in titlebar will just be an option. As
> for the personas - so we'd lose ~5px of vertical height. Not a big deal. I
> doubt users just sit and stare at the persona all day.

I think you are under rating Personas. People like them because they bring pleasure and can personalise the screen - and they can be changed so easily. Most personas don't actually fit the top of either Firefox or Thunderbird and so users add one or two dummy toolbars with spaces into order to see more. So losing space is an issue.
Comments 94 and onward are off-topic and not helpful to this bug - please do NOT continue this discussion in this bug.
Moving this to b4 blocking, but happy to see the forward motion. Hopefully we'll be able to land this early in the b4 code cycle (ie: late next week?)
blocking2.0: beta3+ → beta4+

Comment 106

7 years ago
Created attachment 461351 [details]
Bad looking parts about the exampled way to draw personas

@Peter Henkel
This is the parts I think looks realy bad.
However, as general I don't think personas should mess with the windows-chrome. I really loved personas in FF3.6, but if it turned out like this I would stop using it (personal oppinion of cause).

(sorry about bad english, not my native)

Updated

7 years ago
Duplicate of this bug: 583185

Updated

7 years ago
Duplicate of this bug: 583213

Updated

7 years ago
No longer depends on: 574454
I have the same problem, too. But the problem is only occurring in FF 4. FF 3 and and other versions did not have this problem. Why is it different now?
Persona is drawing also in title bar.

Updated

7 years ago
Duplicate of this bug: 583743

Updated

7 years ago
Duplicate of this bug: 583785

Updated

7 years ago
Duplicate of this bug: 584300
Duplicate of this bug: 585242

Updated

7 years ago
Duplicate of this bug: 585556
This appears to be a regression from beta1 or earlier.  While this bug is unfortunate, I don't feel like we need to block beta4 to get this fixed.  With that said, this fix does need beta coverage, so moving to blocking betaN.  Keep in mind that we are running out of betas, so this needs to be fixed pronto.
blocking2.0: beta4+ → betaN+
It's a bug that's probably the most commented issue via Input since Beta 1. If it's not blocking beta4, what eta can we expect from "pronto"?

Updated

7 years ago
Duplicate of this bug: 586860

Updated

7 years ago
Duplicate of this bug: 586884

Comment 120

7 years ago
I don't see why everyone is making such a big deal over this. Why can't we just tell the painter never to paint in <the area of the control box>? And if you're ever bored, you could tweak the shadows, put the persona there in low alpha (when no focus), etc.

Comment 121

7 years ago
(In reply to comment #120)
> I don't see why everyone is making such a big deal over this. Why can't we just
> tell the painter never to paint in <the area of the control box>? And if you're
> ever bored, you could tweak the shadows, put the persona there in low alpha
> (when no focus), etc.

Because Mozilla make a big deal out of Persona's even going as far as adverts.

Updated

7 years ago
Duplicate of this bug: 580912

Updated

7 years ago
Duplicate of this bug: 587402

Comment 124

7 years ago
Just tried Beta 2 and updated to Beta 3. Same issue. Quite annoying. Seems a fix should occur before final release.


Mozilla/5.0 (Windows; Windows NT 6.0; WOW64; rv:2.0b3) Gecko/20100805 Firefox/4.0b3

See Images:

Default theme, no persona, no menu bar -
http://i34.tinypic.com/263ik9y.jpg

Default theme, persona active, no menu bar -
http://i38.tinypic.com/vo27fn.jpg

Default theme, persona active, menu bar active -
http://i37.tinypic.com/2r4u5nn.jpg
Still the same issue in today's nightly.

Comment 126

7 years ago
This is marked blocking betaN+. It will get fixed for a beta, eventually. Stop spamming the 80 some odd people here with stuff we already know.

Comment 127

7 years ago
(In reply to comment #126)
> This is marked blocking betaN+. It will get fixed for a beta, eventually. Stop
> spamming the 80 some odd people here with stuff we already know.


My one and only post on this bug. I just found out about this bug, votesd and commented. Hardly can be considered "spam." Really appreciate you berating my post/vote like it doesn't mean anything. I thought FF users were better than that.

Comment 128

7 years ago
Each post here sends out 80+ emails, and if any few of those are unwanted it's spam by definition. General comments are referred to as bugspam, often regardless of connotation. Multiple people posting complaining about a bug (not just you) is not helpful, so lets all please just be quiet until someone fixes this thing.

Updated

7 years ago
Duplicate of this bug: 588060

Comment 130

7 years ago
Interesting to note that the persona will also block off the top 3-5px of the screen. The top border is cut off completely, and there is a 3-5 pixel area at the top where the mouse cursor will not display the resize cursor like it should. It will show it a few pixels down.
felipe, can you take this?
(Assignee)

Comment 132

7 years ago
yes, taking
Assignee: nobody → felipc
Status: NEW → ASSIGNED
(Assignee)

Comment 133

7 years ago
Created attachment 468866 [details] [diff] [review]
Patch v1

As the window controls are drawn under the personas theme, clip the controls out of the drawing region. The controls' area that Windows report is a rect, but as the buttons are rounded on the bottom, we need to create a more elaborate region around them.

To get the buttons to draw correctly, they must be under a glass area, so we force the minimum size for the top glass edge to be the size of the buttons. 
This also fixes the top border edges not being rounded when there's a personas applied (bug 575406)

I've tested this with different system settings wrt. border sizes and button sizes, everything looked correct as far as I could see.

The metrics used to adjust the clipping size are a bit crazy, but after various combinations, that's what I ended up with. The fine tune adjustments are due to some shadow and highlights that are included in the metric.
Attachment #468866 - Flags: review?(jmathies)
(Assignee)

Comment 134

7 years ago
Comment on attachment 468866 [details] [diff] [review]
Patch v1

Also needs a small review from dao for browser-aero.css. As we need a top glass area where the buttons are placed, we need -moz-win-borderless-glass applied to trigger the glass calculations. As long as there's the opaque background-color or image, this area will be calculated as 0, and then a min-height will be forced on widget as the height of the buttons.
Attachment #468866 - Flags: review?(dao)
Comment on attachment 468866 [details] [diff] [review]
Patch v1

prelim drive by, an updated patch would be nice. I'll do a follow up tomorrow.

>+void nsWindow::UpdateCaptionButtonsClippingRect() {

nit, brace on the next line.

>+
>+  mCaptionButtonsRoundedRegion.SetEmpty();
>+  if (!mCustomNonClient || !mCompositorFlag || mSizeMode == nsSizeMode_Fullscreen || 
>+      mSizeMode == nsSizeMode_Minimized || !mWnd) {
>+    mCaptionButtons.Empty();
>+    return;
>+  }

mCompositorFlag has gone away. An updated patch would be appreciated. Also, I don't think we need to check mWnd here do we?

>+
>+  RECT captionButtons;
>+  if (S_OK != nsUXThemeData::dwmGetWindowAttributePtr(mWnd, DWMWA_CAPTION_BUTTON_BOUNDS,
>+                                                      &captionButtons, sizeof(captionButtons))) {
>+    mCaptionButtons.Empty();
>+    return;
>+  }

nit, I prefer FAILED() on COM results. Also

#if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN
#endif

ifdef'ing around the dwm call. Plus, you're calling this openly from Resize(). The dwm stuff doesn't have wrappers, can't dwmGetWindowAttributePtr be null?


>+
>+  mCaptionButtons = nsWindowGfx::ToIntRect(captionButtons);
>+
>+  // Adjustments to reported area
>+  PRInt32 leftMargin   = mNonClientMargins.left   == -1 ? mHorResizeMargin  : mNonClientMargins.left;

weird spacing. paren around that conditional statement would be nice too.

>+  PRBool maximized = mSizeMode == nsSizeMode_Maximized;

nit: PRBool maximized = (mSizeMode == nsSizeMode_Maximized);

>+  mCaptionButtons.x -= leftMargin - 1;
>+  mCaptionButtons.width += maximized ? - 2 : leftMargin - 1;
>+  mCaptionButtons.height += maximized ? -3 : -mVertResizeMargin - 1;

hmm, these are some funky constants. (below as well.) Can we pull this stuff via metrics?  Regardless, some commenting explaining where these values come from would be helpful. It also might be clearer to simply

if (mSizeMode == nsSizeMode_Maximized) {
} else {
}

this. Which would give you the space to comment the values. (I'm really curious where these values come from!)

>+
>+  // Create a rounded region
>+  nsIntRect round1(mCaptionButtons.x, mCaptionButtons.y, mCaptionButtons.width, mCaptionButtons.height - 2);
>+  nsIntRect round2(mCaptionButtons.x + 1, mCaptionButtons.YMost() - 2, mCaptionButtons.width - 2, 1);
>+  nsIntRect round3(mCaptionButtons.x + 2, mCaptionButtons.YMost() - 1, mCaptionButtons.width - 13, 1);
>+  mCaptionButtonsRoundedRegion.Or(mCaptionButtonsRoundedRegion, round1);
>+  mCaptionButtonsRoundedRegion.Or(mCaptionButtonsRoundedRegion, round2);
>+  mCaptionButtonsRoundedRegion.Or(mCaptionButtonsRoundedRegion, round3);
>+}
>+
> /**************************************************************
>  *
>  * SECTION: nsIWidget::HideWindowChrome
>  *
>  * Show or hide window chrome.
>  *
>  **************************************************************/
> 
>@@ -6988,16 +7024,18 @@ PRBool nsWindow::OnMove(PRInt32 aX, PRIn
> PRBool nsWindow::OnResize(nsIntRect &aWindowRect)
> {
> #ifdef CAIRO_HAS_D2D_SURFACE
>   if (mD2DWindowSurface) {
>     mD2DWindowSurface = NULL;
>     Invalidate(PR_FALSE);
>   }
> #endif
>+  UpdateCaptionButtonsClippingRect();
>+
>   // call the event callback
>   if (mEventCallback) {
>     nsSizeEvent event(PR_TRUE, NS_SIZE, this);
>     InitEvent(event);
>     event.windowSize = &aWindowRect;
>     RECT r;
>     if (::GetWindowRect(mWnd, &r)) {
>       event.mWinWidth  = PRInt32(r.right - r.left);
>diff --git a/widget/src/windows/nsWindow.h b/widget/src/windows/nsWindow.h
>--- a/widget/src/windows/nsWindow.h
>+++ b/widget/src/windows/nsWindow.h
>@@ -449,16 +449,17 @@ protected:
> #if !defined(WINCE)
>   static void             ActivateOtherWindowHelper(HWND aWnd);
>   static PRUint16         GetMouseInputSource();
> #endif
> #ifdef ACCESSIBILITY
>   static STDMETHODIMP_(LRESULT) LresultFromObject(REFIID riid, WPARAM wParam, LPUNKNOWN pAcc);
> #endif // ACCESSIBILITY
>   void                    ClearCachedResources();
>+  void                    UpdateCaptionButtonsClippingRect();
> 
> protected:
>   nsCOMPtr<nsIWidget>   mParent;
>   nsIntSize             mLastSize;
>   nsIntPoint            mLastPoint;
>   HWND                  mWnd;
>   WNDPROC               mPrevWndProc;
>   HBRUSH                mBrush;
>@@ -500,16 +501,19 @@ protected:
>   static PRUint32       sOOPPPluginFocusEvent;
> #endif
> 
>   // Non-client margin settings
>   // Pre-calculated outward offset applied to default frames
>   nsIntMargin           mNonClientOffset;
>   // Margins set by the owner
>   nsIntMargin           mNonClientMargins;
>+  
>+  nsIntRect             mCaptionButtons;
>+  nsIntRegion           mCaptionButtonsRoundedRegion;
>   // Indicates custom frames are enabled
>   PRPackedBool          mCustomNonClient;
>   // Disable non client margins on non-comsitor desktops
>   PRPackedBool          mCompositorFlag;
>   // Cached copy of L&F's resize border  
>   PRInt32               mHorResizeMargin;
>   PRInt32               mVertResizeMargin;
>   // Height of the caption plus border
>diff --git a/widget/src/windows/nsWindowGfx.cpp b/widget/src/windows/nsWindowGfx.cpp
>--- a/widget/src/windows/nsWindowGfx.cpp
>+++ b/widget/src/windows/nsWindowGfx.cpp
>@@ -241,21 +241,24 @@ void nsWindowGfx::OnSettingsChangeGfx(WP
> }
> 
> // GetRegionToPaint returns the invalidated region that needs to be painted
> // it's abstracted out because Windows XP/Vista/7 handles this for us, but
> // we need to keep track of it our selves for Windows CE and Windows Mobile
> 
> nsIntRegion nsWindow::GetRegionToPaint(PRBool aForceFullRepaint,
>                                        PAINTSTRUCT ps, HDC aDC)
>-{ 
>+{
>   if (aForceFullRepaint) {
>     RECT paintRect;
>     ::GetClientRect(mWnd, &paintRect);
>-    return nsIntRegion(nsWindowGfx::ToIntRect(paintRect));
>+    nsIntRegion region(nsWindowGfx::ToIntRect(paintRect));
>+    region.Sub(region, mCaptionButtonsRoundedRegion);
>+
>+    return region;
>   }
> 
> #if defined(WINCE_WINDOWS_MOBILE) || !defined(WINCE)
>   HRGN paintRgn = ::CreateRectRgn(0, 0, 0, 0);
>   if (paintRgn != NULL) {
> # ifdef WINCE
>     int result = GetUpdateRgn(mWnd, paintRgn, FALSE);
> # else
>@@ -265,21 +268,26 @@ nsIntRegion nsWindow::GetRegionToPaint(P
>       POINT pt = {0,0};
>       ::MapWindowPoints(NULL, mWnd, &pt, 1);
>       ::OffsetRgn(paintRgn, pt.x, pt.y);
>     }
>     nsIntRegion rgn(nsWindowGfx::ConvertHRGNToRegion(paintRgn));
>     ::DeleteObject(paintRgn);
> # ifdef WINCE
>     if (!rgn.IsEmpty())
>+      return rgn;
>+# else
>+    rgn.Sub(rgn, mCaptionButtonsRoundedRegion);
>+    return rgn;
> # endif
>-      return rgn;
>   }
> #endif
>-  return nsIntRegion(nsWindowGfx::ToIntRect(ps.rcPaint));
>+  nsIntRegion region(nsWindowGfx::ToIntRect(ps.rcPaint));
>+  region.Sub(region, mCaptionButtonsRoundedRegion);
>+  return region;
> }
> 
> #define WORDSSIZE(x) ((x).width * (x).height)
> static PRBool
> EnsureSharedSurfaceSize(gfxIntSize size)
> {
>   gfxIntSize screenSize;
>   screenSize.height = GetSystemMetrics(SM_CYSCREEN);

Updated

7 years ago
Attachment #468866 - Flags: review?(dao) → review+

Updated

7 years ago
Duplicate of this bug: 590296

Updated

7 years ago
Duplicate of this bug: 590564
(Assignee)

Comment 138

7 years ago
I'm working on an updated patch. Replying to your comments:

> mCompositorFlag has gone away. An updated patch would be appreciated. Also, I
> don't think we need to check mWnd here do we?
> 

I'll verify that. I thought I saw this sometimes being called with an invalid mWnd and returning invalid button positions, but maybe it was when I was trying the function being called from different places (e.g. nResize checks for mWnd but the callers of OnResize do not).
In any case, I'll check if it never gets called and if that's the case, remove the mWnd check.. (and maybe add an assertion?)


> >+
> >+  RECT captionButtons;
> >+  if (S_OK != nsUXThemeData::dwmGetWindowAttributePtr(mWnd, DWMWA_CAPTION_BUTTON_BOUNDS,
> >+                                                      &captionButtons, sizeof(captionButtons))) {
> >+    mCaptionButtons.Empty();
> >+    return;
> >+  }
> 
> nit, I prefer FAILED() on COM results. Also
> 
> #if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN
> #endif
> 
> ifdef'ing around the dwm call. Plus, you're calling this openly from Resize().

Oh well, I totally forgot the ifdefs. I think I'll ifdef the existence of this function, and its call on OnResize, and leave the nsIntRect and nsIntRegion as is. Should I put ifdefs around the code added to GetRegionToPaint?


> The dwm stuff doesn't have wrappers, can't dwmGetWindowAttributePtr be null?
> 

That's true, I guess it can be. It seems some similar code calls CheckForCompositor() and if it returns true, assume that the others dwm*Ptr exist. Should this be enough, or better null-check this too?

> 
> hmm, these are some funky constants. (below as well.) Can we pull this stuff
> via metrics?  Regardless, some commenting explaining where these values come
> from would be helpful.

Some of these are based on the border sizes plus some fine tune adjustments. For example, notice that on a maximized window, the buttons are visually shifted 3px from the edge of the screen, although they're still clickable on the edge. The size reported includes that invisible area, but we need to adjust that to apply the skin there.
Another one: there's this 1-pixel semi-transparent highlight line around the buttons, but that blends with the background glass color, not our texture, so they look incorrect on anything other than a blue-tone texture. That's the -1 in the width adjustment.
I'll add some comments about that. Some of the other numbers are just ... trial and error
(In reply to comment #138)
> Oh well, I totally forgot the ifdefs. I think I'll ifdef the existence of this
> function, and its call on OnResize, and leave the nsIntRect and nsIntRegion as
> is. Should I put ifdefs around the code added to GetRegionToPaint?

If you ifdef out the functionality, I would ifdef out the member variables and everything that references them.

On GetRegionToPaint, you might check what's involved with Sub with an empty region. If there's no check in the region code you might want to check to see if it's empty before you call Sub. I'd be extra paranoid about perf regressions there.

> 
> 
> > The dwm stuff doesn't have wrappers, can't dwmGetWindowAttributePtr be null?
> > 
> 
> That's true, I guess it can be. It seems some similar code calls
> CheckForCompositor() and if it returns true, assume that the others dwm*Ptr
> exist. Should this be enough, or better null-check this too?

I think that's ok, you only want this to execute if the compositor is enabled and the dwm calls would be available in that case.

> 
> > 
> > hmm, these are some funky constants. (below as well.) Can we pull this stuff
> > via metrics?  Regardless, some commenting explaining where these values come
> > from would be helpful.
> 
> Some of these are based on the border sizes plus some fine tune adjustments.
> For example, notice that on a maximized window, the buttons are visually
> shifted 3px from the edge of the screen, although they're still clickable on
> the edge. The size reported includes that invisible area, but we need to adjust
> that to apply the skin there.
> Another one: there's this 1-pixel semi-transparent highlight line around the
> buttons, but that blends with the background glass color, not our texture, so
> they look incorrect on anything other than a blue-tone texture. That's the -1
> in the width adjustment.
> I'll add some comments about that. Some of the other numbers are just ... trial
> and error

You might move these to #defines so they have descriptive names. Does the window border width come into play here at all? That can come from metrics.

Comment 140

7 years ago
(In reply to comment #138)
> > hmm, these are some funky constants. (below as well.) Can we pull this stuff
> > via metrics?  Regardless, some commenting explaining where these values come
> > from would be helpful.
> 
> Some of these are based on the border sizes plus some fine tune adjustments.
> For example, notice that on a maximized window, the buttons are visually
> shifted 3px from the edge of the screen, although they're still clickable on
> the edge. The size reported includes that invisible area, but we need to adjust
> that to apply the skin there.

Note that the WM_GETTITLEBARINFOEX message gives more acureate results than dwmGetWindowAttributePtr() with DWMWA_CAPTION_BUTTON_BOUNDS: The message returns the exact outer bounds of every single caption button. One only has to add an offset for the 1-pixel semi-transparent highlight line and a button edge radius. So the stange constants like 2, 3 or 13 would disappear, as you can see in the get_system_button_bounds() function of my testapp (the only irritating constant I used in my code is 5 for the button edge radius).
However, if you prefer dwmGetWindowAttributePtr(), the constants that depend on border size should realy be calculated by calls to GetSystemMetrics(SM_CXSIZEFRAME) and so on, because a user can adjust the border width easily.
One thing is certain though, don't forget its got to work with the custom drawing implementation window, which I'm sure Jim will be able to ascertain if that's the right approach or not.
(In reply to comment #140)
> (In reply to comment #138)
> Note that the WM_GETTITLEBARINFOEX message gives more acureate results than
> dwmGetWindowAttributePtr() with DWMWA_CAPTION_BUTTON_BOUNDS: The message
> returns the exact outer bounds of every single caption button. One only has to
> add an offset for the 1-pixel semi-transparent highlight line and a button edge
> radius. So the stange constants like 2, 3 or 13 would disappear, as you can see
> in the get_system_button_bounds() function of my testapp (the only irritating
> constant I used in my code is 5 for the button edge radius).
> However, if you prefer dwmGetWindowAttributePtr(), the constants that depend on
> border size should realy be calculated by calls to
> GetSystemMetrics(SM_CXSIZEFRAME) and so on, because a user can adjust the
> border width easily.

We just happen to have a freshly minted way to use WM_GETTITLEBARINFOEX:

http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsUXThemeData.cpp#255
(Assignee)

Comment 143

7 years ago
(In reply to comment #139)
> (In reply to comment #138)
> > Oh well, I totally forgot the ifdefs. I think I'll ifdef the existence of this
> > function, and its call on OnResize, and leave the nsIntRect and nsIntRegion as
> > is. Should I put ifdefs around the code added to GetRegionToPaint?
> 
> If you ifdef out the functionality, I would ifdef out the member variables and
> everything that references them.
> 
> On GetRegionToPaint, you might check what's involved with Sub with an empty
> region. If there's no check in the region code you might want to check to see
> if it's empty before you call Sub. I'd be extra paranoid about perf regressions
> there.

right, I checked nsRegion::Sub, and it exits early if the region is empty

> 
> > That's true, I guess it can be. It seems some similar code calls
> > CheckForCompositor() and if it returns true, assume that the others dwm*Ptr
> > exist. Should this be enough, or better null-check this too?
> 
> I think that's ok, you only want this to execute if the compositor is enabled
> and the dwm calls would be available in that case.
> 
Ok


> You might move these to #defines so they have descriptive names. Does the
> window border width come into play here at all? That can come from metrics.
and
>the constants that depend on border size should realy be calculated by calls to
> GetSystemMetrics(SM_CXSIZEFRAME) and so on, because a user can adjust the
> border width easily.

Yeah, I'm already using the border size into the leftMargin variable. It is the system border-size or the custom chrome margin value, if any.


I think I'd prefer to keep using dwmGetWindowAttributePtr() with DWMWA_CAPTION_BUTTON_BOUNDS because it already provides a single rect with the information wanted (although we need to make the adjustments). If I use WM_GETTITLEBARINFOEX there's only individual button info, and we'd have to build 3 rectangles from there, merge them and hope that the region is optimized well enough.

Peter, Jim, I am attaching an updated patch with comments that should hopefully make it much more clear where all these values are coming from. Please take a look and let me know what you think
(Assignee)

Comment 144

7 years ago
Created attachment 469171 [details] [diff] [review]
Patch v2

Previous review nits addressed, comments about the margin calcs updated.
Attachment #468866 - Attachment is obsolete: true
Attachment #468866 - Flags: review?(jmathies)

Comment 145

7 years ago
(In reply to comment #142)
> (In reply to comment #140)
> > (In reply to comment #138)
> > Note that the WM_GETTITLEBARINFOEX message gives more acureate results than
> > dwmGetWindowAttributePtr() with DWMWA_CAPTION_BUTTON_BOUNDS: The message
> > returns the exact outer bounds of every single caption button. One only has to
> > add an offset for the 1-pixel semi-transparent highlight line and a button edge
> > radius. So the stange constants like 2, 3 or 13 would disappear, as you can see
> > in the get_system_button_bounds() function of my testapp (the only irritating
> > constant I used in my code is 5 for the button edge radius).
> > However, if you prefer dwmGetWindowAttributePtr(), the constants that depend on
> > border size should realy be calculated by calls to
> > GetSystemMetrics(SM_CXSIZEFRAME) and so on, because a user can adjust the
> > border width easily.
> 
> We just happen to have a freshly minted way to use WM_GETTITLEBARINFOEX:
> 
> http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsUXThemeData.cpp#255

But in this code, the the three buttons are expected to have the same width, which is not the case with DWM enabled. 

(In reply to comment #143)
> If I use
> WM_GETTITLEBARINFOEX there's only individual button info, and we'd have to
> build 3 rectangles from there, merge them and hope that the region is optimized
> well enough.

I am not sure what you mean exactly by "region is optimized". Do you mean we cannot be sure that the union of these 3 rectangles is a single big rectangle? Ok, that is true; it might realy need some more code than only a few UnionRect calls. I was only concerned about the strange rectangle adjustments, but they are well explained in the new patch. except:

>+    // Adjustments to the buttons' shift from the edge of the screen,
>+    // plus some apparently transparent drop shadow below them.
>+    mCaptionButtons.width -= 2;
>+    mCaptionButtons.height -= 3;

Are these realy constant? What if I set the border size to 10 pixels instead of 1 via the "Advanced Appearance dialog box"? I must admit that changing this value might be no common use-case, but the comment could at least mention that those settings are ignored or that the default SM_C[X|Y]SIZEFRAME value of 1 is expected.
(Assignee)

Comment 146

7 years ago
> I am not sure what you mean exactly by "region is optimized". Do you mean we
> cannot be sure that the union of these 3 rectangles is a single big rectangle?
Yes, exactly.


> >+    // Adjustments to the buttons' shift from the edge of the screen,
> >+    // plus some apparently transparent drop shadow below them.
> >+    mCaptionButtons.width -= 2;
> >+    mCaptionButtons.height -= 3;
> 
> Are these realy constant? What if I set the border size to 10 pixels instead of
> 1 via the "Advanced Appearance dialog box"? I must admit that changing this
> value might be no common use-case, but the comment could at least mention that
> those settings are ignored or that the default SM_C[X|Y]SIZEFRAME value of 1 is
> expected.

Yeah. I tested that: changed various border sizes (active window border, border padding) and titlebar sizes, and AFAICT these values are constant.
Felipe, what were the results of trying to add a matching glass window border to the top of the window?
(Assignee)

Comment 148

7 years ago
Created attachment 469317 [details]
Screenshot of current patch

Screenshot of current patch with light and dark personas skins. Also includes the mouseover details and a maximized window screenshot
(Assignee)

Comment 149

7 years ago
Tryserver build at http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/felipc@gmail.com-931cae82f7e0/tryserver-win32/

Comment 150

7 years ago
Comment on attachment 469317 [details]
Screenshot of current patch

This looks great except for the fact that the Minefield button doesn't go completely up to the top edge, showing a little persona in between.
(In reply to comment #150)
> Comment on attachment 469317 [details]
> Screenshot of current patch
> 
> This looks great except for the fact that the Minefield button doesn't go
> completely up to the top edge, showing a little persona in between.

I believe that is bug 574681.
Is there a bug for drawing personas on the left/right/bottom edges of the window ? Or will it be fixed by this bug ?
(In reply to comment #150)
> Comment on attachment 469317 [details]
> Screenshot of current patch
> 
> This looks great except for the fact that the Minefield button doesn't go
> completely up to the top edge, showing a little persona in between.

Not only that, but also caption buttons look OK when you look at them from the right (they are connected with edge), but not from the left (the are not connected with the edge). Isn't this paradox?

(In reply to comment #152)
> Is there a bug for drawing personas on the left/right/bottom edges of the
> window ? Or will it be fixed by this bug ?

Yes, but don't know which one.
Comment on attachment 469317 [details]
Screenshot of current patch

we can address the texture above the button. What this fix does not address is the following:

1) left, right, and bottom glass borders.
2) button glow is clipped by the texture.
Attachment #469317 - Flags: ui-review?(beltzner)

Updated

7 years ago
Attachment #469317 - Flags: feedback?(faaborg)
(In reply to comment #152)
> Is there a bug for drawing personas on the left/right/bottom edges of the
> window ? Or will it be fixed by this bug ?

That was originally bug 575870, which got morphed into simply getting the titlebar drawing in xul. There's no bug filed yet on implementing the rest of the frame in xul afaik.
(In reply to comment #155)
> (In reply to comment #152)
> > Is there a bug for drawing personas on the left/right/bottom edges of the
> > window ? Or will it be fixed by this bug ?
> 
> That was originally bug 575870, which got morphed into simply getting the
> titlebar drawing in xul. There's no bug filed yet on implementing the rest of
> the frame in xul afaik.

filed bug 590945.
Comment on attachment 469317 [details]
Screenshot of current patch

Jim/Felipe: great work.

We should file a follow-up bug on allowing the persona to draw into the glass border. I think it would be nice (or perhaps nice to just remove the glass border when there's a persona installed?) but isn't germane to the larger issue which is that when a persona is applied, there's no way to close the window ;)

The clipping of the glow is fine; I doubt many people will notice.
Attachment #469317 - Flags: ui-review?(beltzner) → ui-review+
(In reply to comment #149)
> Tryserver build at
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/felipc@gmail.com-931cae82f7e0/tryserver-win32/

I'm still seeing obscured buttons with this build. The rounded corners are back though. I had the same thing in a debug build with these patches, so I'll try to debug.
Comment on attachment 469317 [details]
Screenshot of current patch

overall good, couple quick questions:

-if in the future we wanted to draw the window controls ourselves (so that the persona could be shown under them), would that still be possible?
-is it possible to cover the window border as well?
Attachment #469317 - Flags: feedback?(faaborg) → feedback+
(In reply to comment #159)
> -is it possible to cover the window border as well?

That's being covered in bug 590945
(In reply to comment #159)
> Comment on attachment 469317 [details]
> Screenshot of current patch
> 
> overall good, couple quick questions:
> 
> -if in the future we wanted to draw the window controls ourselves (so that the
> persona could be shown under them), would that still be possible?

We're currently drawing these in content for everything except glass desktops. For glass, we would have to investigate how to remove the buttons down in widget so we could paint our own. We should be able to do this through window styles or maybe there's a dwm call we can make.
(In reply to comment #161)
> (In reply to comment #159)
> > Comment on attachment 469317 [details] [details]
> > Screenshot of current patch
> > 
> > overall good, couple quick questions:
> > 
> > -if in the future we wanted to draw the window controls ourselves (so that the
> > persona could be shown under them), would that still be possible?
> 
> We're currently drawing these in content for everything except glass desktops.
> For glass, we would have to investigate how to remove the buttons down in
> widget so we could paint our own. We should be able to do this through window
> styles or maybe there's a dwm call we can make.

Actually, if the content is completely opaque, we could go ahead and do this now, since painting opaque content clips out glass and the caption buttons. We might have to re-solve the rounded edges problem again (bug 575406) which should be fixed as things are now with felipe's patch here.
(Assignee)

Comment 163

7 years ago
Created attachment 469699 [details] [diff] [review]
Patch v3

Thanks a lot Jim for figuring out how to fix the GDI issue.

Third patch, includes the region clipping for GDI and the workaround needed (black background first) to make the buttons appear.
Attachment #469171 - Attachment is obsolete: true
Attachment #469699 - Flags: review?(jmathies)
(In reply to comment #157)
> The clipping of the glow is fine; I doubt many people will notice.
So this won't be fixed ?
(In reply to comment #164)
> (In reply to comment #157)
> > The clipping of the glow is fine; I doubt many people will notice.
> So this won't be fixed ?

It will, but not in this bug. File follow up bugs on any remaining issues you find once this bug is resolved.
Comment on attachment 469699 [details] [diff] [review]
Patch v3

>+#if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN
>+void nsWindow::UpdateCaptionButtonsClippingRect()
>+{
>+  NS_ASSERTION(mWnd, "UpdateCaptionButtonsClippingRect called with invalid mWnd.");
>+
>+  mCaptionButtonsRoundedRegion.SetEmpty();
>+  if (!mCustomNonClient || mSizeMode == nsSizeMode_Fullscreen || 
>+      mSizeMode == nsSizeMode_Minimized) {
>+    mCaptionButtons.Empty();
>+    return;
>+  }
>+
>+  RECT captionButtons;
>+  if (!nsUXThemeData::CheckForCompositor() ||
>+      FAILED(nsUXThemeData::dwmGetWindowAttributePtr(mWnd, DWMWA_CAPTION_BUTTON_BOUNDS,
>+                                                     &captionButtons, sizeof(captionButtons)))) {
>+    mCaptionButtons.Empty();
>+    return;
>+  }

Can we call mCaptionButtons.Empty() on entry, move captionButtons up, and combine these two if statements into one?

Also, we are pretty lax on the 80 character limit in windows/widget, but lets try and tighten all this up a bit.

>+
>+  mCaptionButtons = nsWindowGfx::ToIntRect(captionButtons);
>+
>+  // Adjustments to reported area
>+  PRInt32 leftMargin = (mNonClientMargins.left == -1) ? mHorResizeMargin  : mNonClientMargins.left;
>+
>+  // (leftMargin - 1) represents the resizer border and an
>+  // one pixel adjustment to hide the semi-transparent highlight.
>+  // The extra width is already excluded when the window is maximized.
>+  mCaptionButtons.x -= (leftMargin - 1);
>+
>+  if (mSizeMode != nsSizeMode_Maximized) {
>+    mCaptionButtons.width += (leftMargin - 1);
>+    mCaptionButtons.height -= mVertResizeMargin + 1;

Paren or no paren, but don't mix and match.


>+#if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN
>+          if (IsRenderMode(gfxWindowsPlatform::RENDER_GDI) &&
>+              mTransparencyMode != eTransparencyTransparent &&
>+              !mCaptionButtons.IsEmpty()) {
>+            RECT rect;
>+            rect.top = mCaptionButtons.y;
>+            rect.left = mCaptionButtons.x;
>+            rect.right = mCaptionButtons.x + mCaptionButtons.width;
>+            rect.bottom = mCaptionButtons.y + mCaptionButtons.height;
>+            FillRect(hDC, &rect, (HBRUSH)GetStockObject(BLACK_BRUSH));
>+
>+            const nsIntRect* r;
>+            for (nsIntRegionRectIterator iter(event.region);
>+                 (r = iter.Next()) != nsnull;) {
>+              thebesContext->Rectangle(gfxRect(r->x, r->y, r->width, r->height), PR_TRUE);
>+            }
>+            thebesContext->Clip();
>+          }
>+#endif
>+
>           {
>             AutoLayerManagerSetup
>                 setupLayerManager(this, thebesContext, doubleBuffering);
>             result = DispatchWindowEvent(&event, eventStatus);
>           }
> 
> #ifdef MOZ_XUL
>           if ((IsRenderMode(gfxWindowsPlatform::RENDER_GDI) ||


Don't forget the follow up bug on parity in RENDER_IMAGE_STRETCH24 & RENDER_IMAGE_STRETCH32.
Attachment #469699 - Flags: review?(jmathies) → review+
> >+            RECT rect;
> >+            rect.top = mCaptionButtons.y;
> >+            rect.left = mCaptionButtons.x;
> >+            rect.right = mCaptionButtons.x + mCaptionButtons.width;
> >+            rect.bottom = mCaptionButtons.y + mCaptionButtons.height;
> >+            FillRect(hDC, &rect, (HBRUSH)GetStockObject(BLACK_BRUSH));
> >+
> >+            const nsIntRect* r;
> >+            for (nsIntRegionRectIterator iter(event.region);
> >+                 (r = iter.Next()) != nsnull;) {
> >+              thebesContext->Rectangle(gfxRect(r->x, r->y, r->width, r->height), PR_TRUE);
> >+            }
> >+            thebesContext->Clip();
> >+          }

Also, comment this.
Comment on attachment 469699 [details] [diff] [review]
Patch v3

I'd like to see the final patch.
Attachment #469699 - Flags: review+ → review-
(Assignee)

Comment 169

7 years ago
Created attachment 469897 [details] [diff] [review]
Patch v4

Updated patch with latest comments addressed.
Attachment #469699 - Attachment is obsolete: true
Attachment #469897 - Flags: review?(jmathies)

Updated

7 years ago
Attachment #469897 - Flags: review?(jmathies) → review+
(Assignee)

Updated

7 years ago
Depends on: 591391
(Assignee)

Updated

7 years ago
Depends on: 591392
(Assignee)

Comment 170

7 years ago
> Don't forget the follow up bug on parity in RENDER_IMAGE_STRETCH24 &
> RENDER_IMAGE_STRETCH32.

Filed bug 591391 for that.

Also filed bug 591392 to cover the taskbar[tab]previews side of this bug.
Be careful with this. If someone use a darker persona, the Tab Candy (or what its name is...) icon won't be noticeable.

The Fx button is not on the very top of the window, why?

BTW, those screenshots look very nice, congrats!

Did you tested it with tabs in title bar?
Someone told me that others bugs exist to fix the things i mentioned. Sorry guys.
(Assignee)

Comment 173

7 years ago
http://hg.mozilla.org/mozilla-central/rev/895362d68203
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b5
Depends on: 591598

Updated

7 years ago
Duplicate of this bug: 591804
It would be great if someone could verify this fix. There should be enough on this list who were affected by it. Please grab our latest nightly build for testing purposes. Thanks.

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/
Flags: in-litmus?
verified fixed with Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b5pre) Gecko/20100830 Firefox/4.0b5pre
Status: RESOLVED → VERIFIED

Updated

7 years ago
Keywords: relnote, uiwanted

Comment 177

7 years ago
Created attachment 470419 [details]
Personas with 2 pixel margin on top

I don't know if this is the right place to write my comments, but I think that Personas background should be applied without covering the first 2 pixel on the top border (see attached image for comparison).
I've already reported that on bug 513170 . (And badly on bug 574681, too)
(In reply to comment #177)
> I don't know if this is the right place to write my comments, but I think that
> Personas background should be applied without covering the first 2 pixel on the
> top border (see attached image for comparison).

Please file new bugs for all remaining issues.
You mean my case, too? Josh told me to report it to bug 513170.
(In reply to comment #180)
> You mean my case, too? Josh told me to report it to bug 513170.

I'll file on this.

Updated

7 years ago
Depends on: 591930
(In reply to comment #181)
> (In reply to comment #180)
> > You mean my case, too? Josh told me to report it to bug 513170.
> 
> I'll file on this.

filed bug 591930.
(Assignee)

Comment 183

7 years ago
Created attachment 470590 [details] [diff] [review]
Follow up patch

Follow up patch, changes the behavior a bit after talking with Bas and Jim. Now instead of clipping the buttons area before drawing our content, we let content draw and then we clear that area to transparent again. This should have a much smaller effect on rendering if I understood it correctly.

The patch reverts GetRegionToPaint, removes the GDI hack and adds the clearing code
Attachment #470590 - Flags: review?(jmathies)
(Assignee)

Comment 184

7 years ago
Comment on attachment 470590 [details] [diff] [review]
Follow up patch

Actually, this should be done as a separate bug
Attachment #470590 - Attachment is obsolete: true
Attachment #470590 - Flags: review?(jmathies)

Comment 185

7 years ago
Just updated to latest build and issue persists.

No close/minimize/restore buttons when using firefox with a skin.

Comment 186

7 years ago
(In reply to comment #185)
> Just updated to latest build

Please be a lot more specific.

Comment 187

7 years ago
(In reply to comment #186)
> (In reply to comment #185)
> > Just updated to latest build
> 
> Please be a lot more specific.

Updated to Beta build 4 (20100818132640) - Release notes claim this to be built on 24th August.

I am still not able to see the minimize/close/restore buttons on the top right corner of firefox when using a skin.

Comment 188

7 years ago
(In reply to comment #187)
> (In reply to comment #186)
> > (In reply to comment #185)
> > > Just updated to latest build
> > 
> > Please be a lot more specific.
> 
> Updated to Beta build 4 (20100818132640) - Release notes claim this to be built
> on 24th August.

See comment 173. The fix was committed on 2010-08-27 and your build is older than that. If you want a newer build with the fix either use Minefield (Trunk) or beta 5 (currently on test build 1) or later.

Comment 189

7 years ago
Cool, thanks for the update. 
I have the automatic update on - would that not pull the Beta 5 on its own?

Comment 190

7 years ago
(In reply to comment #189)
> Cool, thanks for the update. 
> I have the automatic update on - would that not pull the Beta 5 on its own?

It will when it's ready. My magic eight ball says that'll be mid-next week.

Comment 191

7 years ago
If this helps, this is what has happened to me over past three days. I rarely have more than three tabs plus an app tab open, I have tabs on top,the Minefield button, no menu bar and no top border.  I have no style extensions but I do have a persona. I have suddenly noticed on the right that instead of my three buttons I literally have a hole. I can see the wall paper through the rectangular space. I am not aware of having clicked anything nor can I reproduce having tried all sorts of combinations. This has happened now six times.

If I then click 'Options' on the Minefield button I get the Options box plus a top border, gobbledegook writing ion the left and the three control buttons on the right. I then click OK, the border and the writing go, the Minefield button reappears and the three control buttons are correct in their proper position.
(Assignee)

Comment 192

7 years ago
(In reply to comment #191)
> If this helps, this is what has happened to me over past three days. I rarely
> have more than three tabs plus an app tab open, I have tabs on top,the
> Minefield button, no menu bar and no top border.  I have no style extensions
> but I do have a persona. I have suddenly noticed on the right that instead of
> my three buttons I literally have a hole. I can see the wall paper through the
> rectangular space. I am not aware of having clicked anything nor can I
> reproduce having tried all sorts of combinations. This has happened now six
> times.
> 
> If I then click 'Options' on the Minefield button I get the Options box plus a
> top border, gobbledegook writing ion the left and the three control buttons on
> the right. I then click OK, the border and the writing go, the Minefield button
> reappears and the three control buttons are correct in their proper position.

Can you take a screenshot the next time this happens and file a follow-up bug?

Comment 193

7 years ago
I have a similar problem also regarding private browsing.
Sometimes when in private browsing mode the upper part of the screen becomes 
transparent (i can see the desktop and not the buttons/tabs)!!

I'm not using persona.
Only closing ff does restore the standard look (minimize/restore window 
doesn't work).
It seems like the text (tab's title) is drawn on a background copied from the desktop

Next time I'll take a screenshot.
(In reply to comment #193)
> I have a similar problem also regarding private browsing.
> Sometimes when in private browsing mode the upper part of the screen becomes 
> transparent (i can see the desktop and not the buttons/tabs)!!
> 
> I'm not using persona.
> Only closing ff does restore the standard look (minimize/restore window 
> doesn't work).
> It seems like the text (tab's title) is drawn on a background copied from the
> desktop
> 
> Next time I'll take a screenshot.

Please file a new bug with step to reproduce. This bug is specific to persona issues with caption buttons.
(In reply to comment #192)
> (In reply to comment #191)
> > If this helps, this is what has happened to me over past three days. I rarely
> > have more than three tabs plus an app tab open, I have tabs on top,the
> > Minefield button, no menu bar and no top border.  I have no style extensions
> > but I do have a persona. I have suddenly noticed on the right that instead of
> > my three buttons I literally have a hole. I can see the wall paper through the
> > rectangular space. I am not aware of having clicked anything nor can I
> > reproduce having tried all sorts of combinations. This has happened now six
> > times.
> > 
> > If I then click 'Options' on the Minefield button I get the Options box plus a
> > top border, gobbledegook writing ion the left and the three control buttons on
> > the right. I then click OK, the border and the writing go, the Minefield button
> > reappears and the three control buttons are correct in their proper position.
> 
> Can you take a screenshot the next time this happens and file a follow-up bug?

This is appears to be just the inactive window state.  The buttons are transparent glass when its not the active window.  FWIW, IE & TB does the same.

Updated

7 years ago
Duplicate of this bug: 593572

Updated

7 years ago
Duplicate of this bug: 593617

Updated

7 years ago
Duplicate of this bug: 593595

Comment 199

7 years ago
I don't like the UI in window mode, so ugly :(. I think we can improve it. My suggestion is "lengthen the navigation toolbar and all background is Persona."

Comment 200

7 years ago
With 20100906 and both the default theme and a persona, clicking 'New' in the Snippet Tool on and off makes the caption buttons come and go - but no 'hole' to the wallpaper underneath.

Comment 201

7 years ago
We just hit comment 200 in this bug with over 100 people being emailed for each post here. Please post all follow ups in new bugs, blocking this one as needed.

Updated

7 years ago
Duplicate of this bug: 593643
Created attachment 473970 [details]
Using Personas themes removed the aero caption buttons (no minimize, maximize, close buttons)

I guess this issue stood resolved, please see the attachment.
using Using Personas themes removed the aero caption buttons (no minimize, maximize, close buttons)

Updated

7 years ago
Depends on: 595780
Litmus testcase added:
https://litmus.mozilla.org/show_test.cgi?id=12902
Flags: in-litmus? → in-litmus+

Updated

7 years ago
Depends on: 621152
You need to log in before you can comment on or make changes to this bug.