Comment on attachment 334606 [details] [diff] [review] v1.0 r=me on widget changes; MenuPopup changes look good too.

Comment on attachment 334606 [details] [diff] [review] v1.0 r + ui-r =mconnor thanks a bunch, this looks great!

Pushed as 17119:7f3879f42151.

You apply this to all potential .KUI-panel panels (bug 434946, bug 436304, extensions...), but you only make the text work for the ctrlTab panel. Was the reflection dropped for technical or aesthetic reasons? Is height="100%" needed on the html:span? I think it might be a no-op. Wouldn't a xul:label have worked with textContent too? It seems that winstripe and pinstripe would need an update too.

> It seems that winstripe and pinstripe would need an update too. Err, gnomestripe and pinstripe.

This also caused a ctrlTab test to fail. Updating it should be easy. I didn't do that but backed out the patch instead, for the reasons / open questions in comment 4.

Not sure how much time you want to or can spend on this -- feel free to land the layout and widget changes separately and delegate the front end stuff to me.

Please remove the part of '<html:div id="ctrlTab-label-parent"> <html:span id="ctrlTab-label" height="100%"></html:span> </html:div> As the original 'label' should work (see comment #4), and this change is causing the problems for other themes and also seems to give a lot of overhead...

(In reply to comment #4) > You apply this to all potential .KUI-panel panels (bug 434946, bug 436304, > extensions...), but you only make the text work for the ctrlTab panel. CtrlTab was the only KUI panel I knew about. How about a KUI-panel-text class which we can then add the text-shadow glow to. > > Was the reflection dropped for technical or aesthetic reasons? Aesthetic. Alt-tab doesn't have a reflection either. > > Is height="100%" needed on the html:span? I think it might be a no-op. Wouldn't > a xul:label have worked with textContent too? > > It seems that winstripe and pinstripe would need an update too. > xul:labels don't get text-shadow, sadly. (In reply to comment #5) > > It seems that winstripe and pinstripe would need an update too. > > Err, gnomestripe and pinstripe. > Ok (In reply to comment #8) > Please remove the part of '<html:div id="ctrlTab-label-parent"> > <html:span id="ctrlTab-label" height="100%"></html:span> > </html:div> > As the original 'label' should work (see comment #4), and this change is > causing the problems for other themes and also seems to give a lot of > overhead... I can't remove it without having potentially unreadable text. I know this is problematic for other themes, but there's no way around it as far as I know.

(In reply to comment #9) > (In reply to comment #4) > > You apply this to all potential .KUI-panel panels (bug 434946, bug 436304, > > extensions...), but you only make the text work for the ctrlTab panel. > > CtrlTab was the only KUI panel I knew about. How about a KUI-panel-text class > which we can then add the text-shadow glow to. That doesn't help much because text-shadow doesn't work everywhere. The simplest thing would be to add aero glass only for the ctrlTab panel. > xul:labels don't get text-shadow, sadly. They should when using textContent.

> > > xul:labels don't get text-shadow, sadly. > > They should when using textContent. > Ok, roc seemed to think that they weren't. The downside is that the crop property isn't supported now, so long titles will wrap.

I am right to conclude that the textshadow is only needed when the 'Glass' effect is used. Can therefor the 'html' not added as a separate binding for Vista only (winstripe with the glass decoration enabled).

Comment on attachment 334606 [details] [diff] [review] v1.0 Forgot to mention: >+html|*#ctrlTab-label-parent { >+ text-align: center; >+ overflow: hidden; overflow belongs in browser/base/content/browser.css, text-align could be put there too.

Pushed the layout/widget changes to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/1df65cbf518a

Rob, are you targeting 1.9.1 with bug 438517? If not, do you want me to do the remaining work here?

Dão, I was hoping to get a patch up for bug 438517 in the next few days. When that bug gets fixed, then I can take my CSS changes from the patch on this bug and clean them up a bit so that there's only changes when the compositor is enabled. Then all that remains is a bug when the user switches themes or disables the compositor while firefox is running. I haven't figured out how that can be solved yet, but I don't think it should block since that's a rare situation.

This is just the CSS changes from above. The text-shadow stuff works fine with my current posted patch to bug 438517. At some point between when I wrote v1.0 and now, glass panels stopped working. I'm trying to figure out why, but that probably won't happen before the beta freeze on Tuesday. I suspect bug 452385 or its dependent bug 451015. Either way, there needs to be some more code changes to nsWindow before the CSS changes can/should be checked in.

Comment on attachment 340877 [details] [diff] [review] v2.0 >+ -moz-border-radius: 0 0 0 0; nit: -moz-border-radius: 0; >+ font-size: 16px; Where does this value come from? Might be better to use em or %, so that this depends on the default font size. >+ text-shadow: white 0px 0px 9px, white 0px 0px 9px, white 0px 0px 9px; nit: text-shadow: white 0 0 9px, ...

This fixes glass panels that broke in between my last patch and now. Toggling desktop composition breaks the effect still.

I changed the text-shadow to be 4 9px radius blurs, each offset by 1 pixel from the black text to give it better contrast. Also, I noticed that the panel doesn't take focus anymore; this makes the glass color slightly different and has a small negative effect on readability on a white background. I'd like to get the text higher in the glass pane, but that doesn't seem possible with WS_CAPTION. I'll play with it some more, but I'd like to get this landed tonight for b1.

This replaces WS_CAPTION with WS_THICKFRAME which allows the text on the Ctrl-Tab panel to be closer to the window top (like Alt-Tab). It (for some reason) requires that the panel have noautohide="true" set. Filed bug 457997 for this issue. Changing the ctrlTab popup to have noautohide="true" seems to fix the problem and still passes unit tests. Patch for that to follow

As described in the previous post, this simply adds noautohide="true" to the ctrlTab-panel panel. r=mconnor over AIM.

Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/f63a4b1a4397 http://hg.mozilla.org/mozilla-central/rev/75de7f5efe4b

It is fixed?

Yes. I cleared up one issue with Dão this morning that I thought might keep it from being marked fixed.

Here's a sample screenshot for those who aren't sure what it looks like

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b1pre) Gecko/20081003 Minefield/3.1b1pre running a machine with Win Vista Ultimate.

Rob, as what I can see our implementation of the look is different from the one Microsoft offers when using the task switcher. I'm using Vista Business instead of Ultimate. Marcia, can you also see the difference?

Henrik, this is the focus issue I mentioned in comment 20. The glass effect becomes more opaque when the window has focus (like the task switch) as a visual cue to the user. You can see this with Windows Media Player or other glass applications.

Rob, it's not only the opaque I'm talking about. Also the font doesn't look well at least on my box. Shouldn't we better use a bold one and the same font-face as Windows uses it within the application switcher?

I'm not sure why your font looks different. I intentionally hard coded the font to match the default Vista UI font (Segoe UI). You don't seem to have anti-aliased text in your firefox window (and possibly your task switcher?) which might be related? The Vista fonts are designed with Cleartype in mind. Anti aliasing will give it a bolder look. Besides the font (the size isn't right either I just noticed) and the opacity, is there anything else? We should match on color. I have noticed that the text sometimes seems to be too pixel aligned; I suspect this is ClearType's fault, but it could be our code too. If by chance you mean that we don't show the tab previews the same way, then I should clarify that the goal of this patch was not to make that kind of change. Take a look at bug 456088 for more info.

Rob, I've added bug 458866 to cover the latest issue.

<http://hg.mozilla.org/mozilla-central/annotate/b66468507a59/browser/themes/winstripe/browser/browser-aero.css#l93>: Segoe UI needs to be enclosed in quotation marks. Currently, the whole font rule gets dropped because of this. Do I need to file a separate bug for this?

(In reply to comment #33) > <http://hg.mozilla.org/mozilla-central/annotate/b66468507a59/browser/themes/winstripe/browser/browser-aero.css#l93>: > > Segoe UI needs to be enclosed in quotation marks. Currently, the whole font > rule gets dropped because of this. Do I need to file a separate bug for this? bug 458866?

Is this still working? I was looking at bug 458864 today with a trunk build and my task panel is a normal gray window with typical glass border.

Bug 478784, maybe? Should be fixed on trunk now.

(In reply to comment #36) > Bug 478784, maybe? Should be fixed on trunk now. I've got a build going that I updated just a couple minutes ago. If it's still not in glass I'll post a screenshot.

On Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090731 Minefield/3.6a1pre (.NET CLR 3.5.30729), this build doesn't show the panel having Glass enabled or look like the screenshot in the testcase. It appears that there are many bugs, particularly like bug 458866 comment 13 which explains the behavior we now see?