588764 - Content area needs a grey border around it

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Josh Tumath]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b5pre) Gecko/20100819 Minefield/4.0b5pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b5pre) Gecko/20100819 Minefield/4.0b5pre

Due to the landing of bug 554982, the content area and status bar of browser.xul now have no border. The mockups have a 1px semi-transparent border around them. Also, correct me if I'm wrong, but they have a box-shadow as well.

Reproducible: Always

Comment 1

[Josh Tumath]

I'll request for this to be a blocker for 2.0, because it might look strange for user's without this border.

Comment 2

[Peter Henkel [:Terepin]]

https://bugzilla.mozilla.org/show_bug.cgi?id=571662#c5

Comment 3

[Josh Tumath]

(In reply to comment #2)
I don't understand. Are you saying that this should depend on your bug, or that this is a duplicate?

Comment 4

[Peter Henkel [:Terepin]]

The problem is I don't know. I reported the same issue there, because it is the same border.

Comment 5

[Josh Tumath]

When I read your bug, I assumed that this would be unrelated, because that relates to the toolbars, where as this relates to the actual content area. :) We'll just have to see whether the devs would rather handle them separately or in the same patch.

Comment 7

[Stephen Horlander [:shorlander]]

Attachment: Content/Toolbar Shadow

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Shorlander: I'm guessing you're supporting this as a theme blocker?

Comment 9

[pino_sb]

I think patch should be:

  #main-window[sizemode="normal"] #browser  {
    border-left:1px solid ThreeDShadow;
    border-right:1px solid ThreeDShadow;
  }
  #main-window[sizemode="normal"] #browser-bottombox {
    border-left:1px solid ThreeDShadow;
    border-right:1px solid ThreeDShadow;
    border-bottom:1px solid ThreeDShadow;
  }

This adds a border to the window with the same color as the toolbar border when in windowed mode. It doesn't seem possible to add a shadow that expands into the border area, which comment 7 seems to suggest.
I don't know how to create a patch, but it seems that these lines should be added to themes/winstripe/browser/browser-aero.css ?

Comment 10

[Peter Henkel [:Terepin]]

Attachment: Missing shadow

This shadow is also missing from toolbars background with tabs on bottom.

Comment 11

[Stephen Horlander [:shorlander]]

(In reply to comment #8)
> Shorlander: I'm guessing you're supporting this as a theme blocker?

Yes.

Comment 12

[VeRtex]

Attachment: Border as in Windows Explorer

With Photoshop put the frame of Windows Explorer on top Minefield.
Clearly keeping the proportions and color pixels.

Comment 13

[Johnathan Nightingale [:johnath]]

Per shorlander.

Comment 14

[Valerio]

A possible solution to this bug should be:

#main-window[sizemode="normal"] #browser, #browser-bottombox {
   border-left:1px solid ThreeDShadow !important;
   border-right:1px solid ThreeDShadow !important;
}

#main-window[sizemode="normal"] #browser-bottombox {
   border-bottom:1px solid ThreeDShadow !important;
}

#main-window[sizemode="normal"] #tab-view {
   border: 1px solid ThreeDShadow !important;  
}

#main-window[sizemode="maximized"] #tab-view {
   border-top: 1px solid ThreeDShadow !important;
}

Comment 15

[Peter Henkel [:Terepin]]

I don't see any difference.

Comment 16

[Valerio]

(In reply to comment #15)
> I don't see any difference.

before there wasn't border in panorama view

Comment 17

[redsign]

Before there wasn't any border at all. It is quiet light-colored and that's why it is not very visible. The mockups show a more dark-colored line and a shadow which looks much better I think.

Comment 18

[Valerio]

replacing ThreedShadow with ThreeDDarkShadow the border become more dark

Comment 19

[Peter Henkel [:Terepin]]

This is how is Panorama suppose to look:
http://www.mozilla.sk/wp-content/uploads/2010/05/Firefox-4-Mockup-i06-Win7-Aero-InContentUI-TabCandy-Glass.png

Comment 20

[Valerio]

"suppose"... Are you sure that will be done?
there is some bug for that?

Comment 21

[Peter Henkel [:Terepin]]

I dunno.

Comment 22

[Valerio]

This draw a border around toolbar when tabs are on bottom:

#main-window[sizemode="normal"] #navigator-toolbox:not([tabsontop="true"]) > #nav-bar + #customToolbars + #PersonalToolbar[collapsed="false"] +
#TabsToolbar:last-child:not(:-moz-lwtheme) {
   border-left:1px solid ThreeDShadow !important;
   border-right:1px solid ThreeDShadow !important;
}

#main-window[sizemode="normal"] [tabsontop="false"] #PersonalToolbar {
  border-left: 1px solid ThreeDShadow !important;
  border-right: 1px solid ThreeDShadow !important;
}

[tabsontop="false"] #PersonalToolbar {
  border-top: 1px solid ThreeDShadow !important;
}

Comment 23

[Dão Gottwald [:dao]]

Attachment: add border

This adds an opaque border. A transparent one would regress bug 590468, so we don't want that.

Comment 9 is right regarding the shadow. Obviously it would regress bug 590468 as well.

Comment 24

[Dão Gottwald [:dao]]

Attachment: add border v2

realized that the border shouldn't be there when the window is maximized

Comment 25

[Dão Gottwald [:dao]]

Actually I think even an opaque border might regress bug 590468. We'll see if talos complains.

Comment 26

[Timothy Nikkel (:tnikkel)]

I think you're unlikely to see any change in talos numbers. Tp4 runs without browser chrome, and bug 590468 had no noticeable effect on the Tp4 numbers (that I remember).

Comment 27

[Timothy Nikkel (:tnikkel)]

I can take a look at how your patch changes the layer tree next time I boot into Windows.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm sure this will regress bug 590468.

If we really have to do this, I suggest we hardcode the border drawing into nsWindow painting code when there is a glass background.

Comment 29

[Dão Gottwald [:dao]]

We're currently using -moz-win-borderless-glass (bug 554982).

Comment 30

[Stephen Horlander [:shorlander]]

Attachment: Design with default Windows etch/border

If it isn't possible to get a proper translucent shadow around the content and the chrome it would be best to just use the default Windows 7 etch/border around the content; then use a proper shadow around the toolbars and other UI.

Would the design in this mockup be possible?

Comment 31

[Joe Drew (not getting mail)]

Timothy said he was going to do some stuff. He should do that stuff and then reassign to Dão!

Comment 32

[Timothy Nikkel (:tnikkel)]

Comment 28 supersedes what I was going to do.

Comment 33

[Dão Gottwald [:dao]]

I'm not working on this since it's a widget bug now.

Comment 34

[Josh Tumath]

I thought it was now a theme bug? If we're following Stephen's new mockups, then we're basically just undoing bug 554982 and giving it some polish, aren't we?

Comment 35

[Peter Henkel [:Terepin]]

(In reply to comment #34)
>  If we're following Stephen's new mockups...
Which new mockups?

Comment 36

[Josh Tumath]

(In reply to comment #35)
> Which new mockups?

I was referring to comment 30.

Comment 37

[hjalte]

Fixing Bug 590945 should make the possibility of having an alternatively themed glass window border (one with a shadow in it) real, should it not?

Comment 38

[Dão Gottwald [:dao]]

(In reply to comment #30)
> Created attachment 479464 [details]
> Design with default Windows etch/border
> 
> If it isn't possible to get a proper translucent shadow around the content and
> the chrome it would be best to just use the default Windows 7 etch/border
> around the content; then use a proper shadow around the toolbars and other UI.
> 
> Would the design in this mockup be possible?

The etch is outside of the client area, so the toolbar border can't be drawn there.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I suggest we use a new Windows theme value to express the border look we want, and draw it in the theme engine. This will prevent large layers from needing to be created.

Jim, I know you're kinda swamped, but can you take this?

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

felipe, maybe you can take this?

Comment 41

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #39)
> I suggest we use a new Windows theme value to express the border look we want,
> and draw it in the theme engine. This will prevent large layers from needing to
> be created.

I agree, this seems like the best approach.

Comment 42

[Jim Mathies [:jimm]]

(In reply to comment #39)
> I suggest we use a new Windows theme value to express the border look we want,
> and draw it in the theme engine. This will prevent large layers from needing to
> be created.
> 
> Jim, I know you're kinda swamped, but can you take this?

Hmm, maybe. I'm not sure how we would target just the content area. Looking through theme constants we have NS_THEME_WINDOW, NS_THEME_WIN_BORDERLESS_GLASS, and NS_THEME_WIN_GLASS. Would 1) adding padding to one of these plus 2)rendering a border in DrawWidgetBackground suffice?

(I think adding padding might actually shrink the drawing surface so I'm not sure this would work, have to test it.)

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually, just adding a new theme value and painting it normally in DrawWidgetBackground would regress bug 590468. We need another approach.

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

One possibility would be to put one or more colored rectangles under or around the content area, using XUL elements with background-color and the "layer" attribute we added in bug 590468. Those should be converted to ColorLayers, which don't take up much memory, draw fast, and shouldn't cause bug 590468 to regress --- although I'd like to see a layer tree dump with the resulting patch, to confirm (set gDumpPaintList=1 in a debug build in the debugger).

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If colored rectangles are too clumsy, then three elements, one for each piece of the border, each with a "layer" attribute, should work fine too.

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Or we could just go crazy and implement supermagical optimizations for almost-entirely-transparent ThebesLayers, but I really don't want to do that for Firefox 4.

Comment 47

[:Felipe Gomes (needinfo for replies!)]

Why the current window style with -moz-win-glass doesn't regress bug 590468? Maybe we could add a margin to the browser <hbox> and draw the border as part of DrawWidgetBackground of the window.

But I don't know how we could pass the content-area dimensions info so we don't draw this border over the toolbars/addon-bar area (i.e. only to the sides of the content area).

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Er yeah, I meant to mention that. I realized this morning that bug 601603 probably *did* just regress bug 590468. Before that bug, we pretended we didn't support -moz-win-glass for drawing, so nothing was drawn. I'll follow up in bug 601603.

Comment 49

[Stephen Horlander [:shorlander]]

For clarity the theme/appearance design preferences for this are:

1) Seamless border/shadow around content and toolbars (attachment 467570 [details])

2) Use default Aero border around content and overlap the top 2px (attachment 479464 [details])

3) Some other type of border if either of those two aren't possible

Comment 50

[Peter Henkel [:Terepin]]

(In reply to comment #49)
> 2) Use default Aero border around content and overlap the top 2px (attachment
> 479464 [details])

Then why removing them in the first place?

Comment 51

[Jim Mathies [:jimm]]

(In reply to comment #44)
> One possibility would be to put one or more colored rectangles under or around
> the content area, using XUL elements with background-color and the "layer"
> attribute we added in bug 590468. Those should be converted to ColorLayers,
> which don't take up much memory, draw fast, and shouldn't cause bug 590468 to
> regress --- although I'd like to see a layer tree dump with the resulting
> patch, to confirm (set gDumpPaintList=1 in a debug build in the debugger).

Switched this back to theme assuming we're going to try what roc suggested. I can put this together unless someone else takes it before I get back here.

Comment 52

[:Felipe Gomes (needinfo for replies!)]

I sent a patch to tryserver (http://hg.mozilla.org/try/rev/97829721007b) to get talos numbers using roc's suggestion, as I'm always scared of browser.xul changes.

http://bit.ly/aEUMQm
It seems it didn't cause any problems. I took a look at the layer tree and IIRC it is what is expected, but I'll confirm this soon.

Roc, I have a question: this was done using a solid background-color for the "border" elements, as these become ColorLayers which you mentioned are fast. Will we be able to do a more elaborate look here ("grey border and shadow")? Likely a rgba color will be needed at minimum, and maybe a border or shadow style. Unless we go for a simple solid border look.

Comment 53

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It's actually fine to do whatever you want in those elements, as long as you keep them small.

Comment 54

[:Felipe Gomes (needinfo for replies!)]

Attachment: Layer tree

Attached is the resulting layer tree with this approach implemented.

Note that the layer for the bottom bar will always be present (even if the add-on bar is not visible) because we also need a border on the bottom

Comment 55

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, that looks good. Thanks!

Comment 56

[:Felipe Gomes (needinfo for replies!)]

Attachment: WIP patch

Posting my WIP patch that implements a simple solid border and that I have used to test the resulting layer tree.

Dao, could you pick this up from here to tweak the appearance as you're proficient with how the CSS shadows on the toolbars are implemented?

Comment 57

[Dão Gottwald [:dao]]

Is there a way to reduce the window border? Any border added in the client area currently cuts away from the content area and visually makes the window border fatter.

Comment 58

[:Felipe Gomes (needinfo for replies!)]

It's possible to change the chromemargin left/right values from -1 (applies the default value) to something else. The default is 8 so a value of 6 or 7 probably gets the intended look.

The problem is that this overrides the system settings if the user chose a different setting.

Comment 59

[Dão Gottwald [:dao]]

(In reply to comment #58)
> It's possible to change the chromemargin left/right values from -1 (applies the
> default value) to something else. The default is 8 so a value of 6 or 7
> probably gets the intended look.
> 
> The problem is that this overrides the system settings if the user chose a
> different setting.

Rather than setting absolute numbers, could the native margins be reduced by one or two pixels for -moz-win-borderless-glass?

Comment 60

[Norman Paschke]

The new Addon manager magically creates a good looking border around the content, why?

Comment 61

[Dão Gottwald [:dao]]

Attachment: patch

Comment 62

[Justin Dolske [:Dolske]]

Comment on attachment 493505 [details] [diff] [review]
patch

Out of curiosity, what's |layer="true"| do?

But patch looks fine, checked it on my Win7 box.

Comment 63

[Dão Gottwald [:dao]]

(In reply to comment #62)
> Out of curiosity, what's |layer="true"| do?

See bug 590468

Comment 64

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/a4544a4b3224

Comment 65

[Valerio]

With this patch the border's color of the toolbar and the content area is not the same because in the toolbar border code there is "background-clip: padding-box;"

Comment 66

[James [silentj]]

i think this has given the border above the address and below the bookmarks bar the glass effect.

Comment 67

[Valerio]

Replacing "background-clip:padding-box;" with "background-clip: border-box;" like this :

  #navigator-toolbox,
   #navigator-toolbox > toolbar {
     border-color: @glassToolbarBorderColor@ !important;
     background-clip: border-box !important;
   }

then the border of the toolbar and the content area have the same color

Comment 68

[redsign]

Isn't this border way too light? Looks really bad in my eyes. Looks hardly better than before. :(

Comment 69

[andrew]

This border is the wrong color.  Not only does it abruptly change from the navigation bar above it, but on a white background its just a different enough color to create a blur effect.

If its going to be white, it needs to be completely white (to at least avoid that blur effect on white pages).

But it should really be much darker to match the mockups

Comment 70

[Asa Dotzler [:asa]]

Attachment: screenshot showing content area border in mock and in currently nightly

Comment 71

[Asa Dotzler [:asa]]

re-opening because this isn't right yet.

Comment 72

[Dão Gottwald [:dao]]

We're not going to implement the shadow for reasons discussed in this bug. As for the border itself, file new bugs as needed.

Comment 73

[Jonathan Haas]

Note that both colors are wrong. The border around the top area is way to blueish (is there a separate bug for that?) and around the content it's to bright, while he border between the content and the toolbars needs to be brighter according to the mockups.

Additionally the area for the bottom-left and bottom-right window resizer seems to be now smaller than those of native windows applications, but I'm not sure this is related to this bug.

Comment 74

[Dão Gottwald [:dao]]

(In reply to comment #73)
> Note that both colors are wrong. The border around the top area is way to
> blueish (is there a separate bug for that?)

bug 617506 more or less

Comment 75

[Asa Dotzler [:asa]]

I've filed Bug 618399 on the basic "it's too light" problem

Comment 76

[Jonathan Haas]

(In reply to comment #73)
> Additionally the area for the bottom-left and bottom-right window resizer seems
> to be now smaller than those of native windows applications, but I'm not sure
> this is related to this bug.

Filed bug 618420 on this.