591930 - Persona background is drawn too high in titlebar

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Jim Mathies [:jimm]]

Attachment: current vs. mock up

We need to shift the painting of the persona background-image down one or two pixels, it currently covers the window border. We have moz extensions that may be able to help:

https://developer.mozilla.org/en/css/background-image

Otherwise we might need to create a new extension.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: 5 different implementations

Here are 5 different implementations to adjust the window border and button position, based on the mockups here and the original bug 513170, and also an extra one as suggested to show the window glass resizer borders.

Shorlander, can you take a look at these and tell what you think is the most correct one?

Comment 2

[Jim Mathies [:jimm]]

My personal take is a or e, leaning toward e until we have the rest of the frame skinned.

Comment 3

[Filippo]

My vote is for A. Ciao!

Comment 4

[Karel Jára]

I’m for A as well. Maybe the transparency might be a bit less then 50% but definitely not solid white as in C.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

CC'ing faaborg for feedback on the screenshots

Comment 6

[Alex Faaborg [:faaborg] (Firefox UX)]

>CC'ing faaborg for feedback on the screenshots

stephen should make the call on this

Comment 7

[Stephen Horlander [:shorlander]]

Comment on attachment 471221 [details]
5 different implementations

A is correct. Actually 0.6 alpha is probably closer to what the system border does.

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch v1

Implemented style A with 0.6 alpha on the inner border.

Since the attempts of shifting the bg-color and -image wouldn't work in practice, the solution is to draw the top borders of the window ourselves.

I already wrote and tested this one on top of patch from bug 593950

Comment 9

[:Felipe Gomes (needinfo for replies!)]

FTR, I updated the patch comment to say:

-  /* Redraw borders */
+  /* Artificially draw window borders that are covered by lwtheme */

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 472845 [details] [diff] [review]
Patch v1

>+  #main-window[chromemargin^="0,"] {
>+    border-top: 1px solid transparent;
>+  }
>+
>+  #main-window[chromemargin^="0,"] > #titlebar > #titlebar-content {
>+    border-top: 1px solid transparent;
>+  }
>+
>+  #main-window[chromemargin^="0,"] > #titlebar > #titlebar-content > #appmenu-button-container {
>+    margin-top: -1px !important;
>+  }
>+
>+  #main-window[chromemargin^="0,"] > #titlebar {
>+    margin-bottom: -1px;
>+  }

Why is all this applied if there's no lwtheme?

>+  #main-window[sizemode="normal"]:-moz-lwtheme > #titlebar > #titlebar-content {
>+    border-top-color: rgba(255, 255, 255, 0.6);
>+  }

move :-moz-lwtheme to #titlebar-content

Comment 11

[Thomas Stache]

Is bug 547805 considered in this work, or does it not affect the positioning of the background?

Comment 12

[:Felipe Gomes (needinfo for replies!)]

(In reply to comment #10)
> Comment on attachment 472845 [details] [diff] [review]
> Patch v1
> 
> >+  #main-window[chromemargin^="0,"] {
> >+    border-top: 1px solid transparent;
> >+  }
> >+
> >+  #main-window[chromemargin^="0,"] > #titlebar > #titlebar-content {
> >+    border-top: 1px solid transparent;
> >+  }
> >+
> >+  #main-window[chromemargin^="0,"] > #titlebar > #titlebar-content > #appmenu-button-container {
> >+    margin-top: -1px !important;
> >+  }
> >+
> >+  #main-window[chromemargin^="0,"] > #titlebar {
> >+    margin-bottom: -1px;
> >+  }
> 
> Why is all this applied if there's no lwtheme?

Because the positioning of the elements is affected by the borders added, so I preferred to have a single final layout and make the borders transparent/not transparent, rather than having to shift things up/down when the lwtheme is applied.

> 
> >+  #main-window[sizemode="normal"]:-moz-lwtheme > #titlebar > #titlebar-content {
> >+    border-top-color: rgba(255, 255, 255, 0.6);
> >+  }
> 
> move :-moz-lwtheme to #titlebar-content

Ok

Comment 13

[Kai de Leeuw]

Attachment: Drawing in the XP title bar

Is the drawing error in the attachment related to this bug, or should I file a new bug?

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 472845 [details] [diff] [review]
Patch v1

In order to prevent accidental height increases or alignment changes (now or in the future), please apply your code only in the :-moz-lwtheme case.

Comment 15

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch v2

New patch, with a much better solution now. Before, to draw the two-colors border I was using the border of both the window and the titlebar-content. This lead to some tricky positioning issues, but now I'm using the -moz-border-top-colors to draw both of them to the titlebar-content, which simplifies things as it doesn't deal with the root element anymore.


(In reply to comment #14)
> In order to prevent accidental height increases or alignment changes (now or in
> the future), please apply your code only in the :-moz-lwtheme case.

I've applied everything to the :-moz-lwtheme case now. FWIW the original idea to apply the borders on both cases was exactly to prevent accidental changes (avoiding a different layout for lwtheme),  but I'm fine with it either way.

(In reply to comment #13)
> Created attachment 476512 [details]
> Drawing in the XP title bar
> 
> Is the drawing error in the attachment related to this bug, or should I file a
> new bug?
I took a look and tried to do it together here, but turns out that'll need some trickier changes so it's probably better done in a separate bug.

Comment 16

[Dão Gottwald [:dao]]

(In reply to comment #15)
> (In reply to comment #14)
> > In order to prevent accidental height increases or alignment changes (now or in
> > the future), please apply your code only in the :-moz-lwtheme case.
> 
> I've applied everything to the :-moz-lwtheme case now. FWIW the original idea
> to apply the borders on both cases was exactly to prevent accidental changes
> (avoiding a different layout for lwtheme),  but I'm fine with it either way.

The point is that the toolbar height should remain the same as it is now, not just consistent between lwtheme and non-lwtheme.

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 479622 [details] [diff] [review]
Patch v2

>--- a/browser/themes/winstripe/browser/browser-aero.css
>+++ b/browser/themes/winstripe/browser/browser-aero.css

> %define customToolbarColor hsl(214,44%,87%)
>+%define aeroActiveBorderColor rgb(37, 44, 51)
>+%define aeroInactiveBorderColor rgb(102, 102, 102)

s/aero/glass/

>+  /* Artificially draw window borders that are covered by lwtheme, see bug 591930. */
>+  #main-window[chromemargin^="0,"][sizemode="normal"] > #titlebar > #titlebar-content:-moz-lwtheme {
>+    border-top-width: 2px;
>+    border-top-style: solid;

border-top: 2px solid;

>+  #main-window[chromemargin^="0,"][sizemode="normal"] > #titlebar > #titlebar-content > #appmenu-button-container:-moz-lwtheme {
>+    margin-top: -1px;
>+  }

Won't this still push the toolbars down by one pixel?

Comment 18

[Dão Gottwald [:dao]]

(In reply to comment #17)
> Comment on attachment 479622 [details] [diff] [review]
> Patch v2
> 
> >--- a/browser/themes/winstripe/browser/browser-aero.css
> >+++ b/browser/themes/winstripe/browser/browser-aero.css
> 
> > %define customToolbarColor hsl(214,44%,87%)
> >+%define aeroActiveBorderColor rgb(37, 44, 51)
> >+%define aeroInactiveBorderColor rgb(102, 102, 102)
> 
> s/aero/glass/

Actually, "aero" might be correct as well, assuming it's the same border color for basic and glass. We only use it for glass, though.

Comment 19

[:Felipe Gomes (needinfo for replies!)]

> > s/aero/glass/
> 
> Actually, "aero" might be correct as well, assuming it's the same border color
> for basic and glass. We only use it for glass, though.

They're different colors; I'll change to glass
 {
> >+    border-top-width: 2px;
> >+    border-top-style: solid;
> 
> border-top: 2px solid;

ok

> 
> >+  #main-window[chromemargin^="0,"][sizemode="normal"] > #titlebar > #titlebar-content > #appmenu-button-container:-moz-lwtheme {
> >+    margin-top: -1px;
> >+  }
> 
> Won't this still push the toolbars down by one pixel?

No, the border-top isn't actually pushing anything, because the height of the #titlebar-content is 23 and #titlebar is 30, so any border <= 7 doesn't make a difference.
I could add a margin-bottom: -2px (to #titlebar-content) to guarantee this in case the #titlebar shrinks in the future.

This margin-top: -1px is just to reposition the appmenu button. It's -1px rather than -2px because it's natural position (without the borders) is 1px.

Comment 20

[:Felipe Gomes (needinfo for replies!)]

Attachment: Follow-up

Follow-up needed due to landing of bug 601603, as discussed.

I've realized though that we can remove the [chromemargin^="0,"] selector from these patches as the #titlebar element is not displayed at all when the menu bar is on. Do you want me to remove them from the patches?

Comment 21

[Dão Gottwald [:dao]]

(In reply to comment #20)
> Do you want me to remove them from the patches?

Yes please. Could you also merge the patches?

Comment 22

[:Felipe Gomes (needinfo for replies!)]

Attachment: Merged

Sure, here is the merged patch with chromemargin selectors removed

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 485061 [details] [diff] [review]
Merged

>+  #main-window[sizemode="normal"] > #titlebar > #titlebar-content:-moz-lwtheme {
>+    border-top: 2px solid;
>+    -moz-border-top-colors: @glassActiveBorderColor@ rgba(255,255,255,.6);
>+    margin-bottom: -2px;
>+  }

margin-bottom: -2px; doesn't seem to make a difference. r=me with this removed or explained.

Comment 24

[:Felipe Gomes (needinfo for replies!)]

I removed the margin-bottom: -2px. It was explained in comment 19, but it's not necessary so I kept that off.

http://hg.mozilla.org/mozilla-central/rev/28d6bc4f438a

Comment 25

[arni2033]

Huh, so that annoying white line is how it was designed???
Well, anyway, "border-top: 2px solid;" for Win7 causes bug 1199374