451300 - Add Aero Glass to Ctrl-Tab

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Rob Arnold [:robarnold]]

Attachment: v1.0

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 334606 [details] [diff] [review]
v1.0

r=me on widget changes; MenuPopup changes look good too.

Comment 2

[Mike Connor [:mconnor]]

Comment on attachment 334606 [details] [diff] [review]
v1.0

r + ui-r =mconnor

thanks a bunch, this looks great!

Comment 3

[Reed Loden [:reed]]

Pushed as 17119:7f3879f42151.

Comment 4

[Dão Gottwald [:dao]]

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.

Comment 5

[Dão Gottwald [:dao]]

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

Err, gnomestripe and pinstripe.

Comment 6

[Dão Gottwald [:dao]]

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.

Comment 7

[Dão Gottwald [:dao]]

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.

Comment 8

[Alfred Kayser]

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...

Comment 9

[Rob Arnold [:robarnold]]

(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.

Comment 10

[Dão Gottwald [:dao]]

(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.

Comment 11

[Rob Arnold [:robarnold]]

> 
> > 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.

Comment 12

[Alfred Kayser]

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 13

[Dão Gottwald [:dao]]

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.

Comment 14

[Rob Arnold [:robarnold]]

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

Comment 15

[Dão Gottwald [:dao]]

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

Comment 16

[Rob Arnold [:robarnold]]

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.

Comment 17

[Rob Arnold [:robarnold]]

Attachment: v2.0

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 18

[Dão Gottwald [:dao]]

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, ...

Comment 19

[Rob Arnold [:robarnold]]

Attachment: glass-panel fix (again)

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

Comment 20

[Rob Arnold [:robarnold]]

Attachment: v2.1

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.

Comment 21

[Rob Arnold [:robarnold]]

Attachment: glass-panel fix (again again)

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

Comment 22

[Rob Arnold [:robarnold]]

Attachment: v2.2

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

Comment 23

[Rob Arnold [:robarnold]]

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

Comment 24

[Tomas]

It is fixed?

Comment 25

[Rob Arnold [:robarnold]]

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

Comment 26

[Rob Arnold [:robarnold]]

Attachment: screenshot

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

Comment 27

[Marcia Knous [:marcia]]

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.

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Comparision between Firefox and application switcher

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?

Comment 29

[Rob Arnold [:robarnold]]

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.

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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?

Comment 31

[Rob Arnold [:robarnold]]

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.

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 33

[(no longer active)]

<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?

Comment 34

[Dão Gottwald [:dao]]

(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?

Comment 35

[Jim Mathies [:jimm]]

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.

Comment 36

[Dão Gottwald [:dao]]

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

Comment 37

[Jim Mathies [:jimm]]

(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.

Comment 38

[[not reading bugmail]]

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?