Content area needs a grey border around it

RESOLVED FIXED in Firefox 4.0b8

Status

()

defect
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: josh.tumath+bugzilla, Assigned: dao)

Tracking

(Blocks 1 bug)

Trunk
Firefox 4.0b8
All
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

()

Attachments

(4 attachments, 6 obsolete attachments)

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
I'll request for this to be a blocker for 2.0, because it might look strange for user's without this border.
Blocks: 544820
blocking2.0: --- → ?
Depends on: 554982
Version: unspecified → Trunk
(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?
Summary: Content needs a grey border around it → Content area needs a grey border around it
The problem is I don't know. I reported the same issue there, because it is the same border.
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.
Duplicate of this bug: 588974
Shorlander: I'm guessing you're supporting this as a theme blocker?
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 ?
Posted image Missing shadow (obsolete) —
This shadow is also missing from toolbars background with tabs on bottom.
(In reply to comment #8)
> Shorlander: I'm guessing you're supporting this as a theme blocker?

Yes.
Summary: Content area needs a grey border around it → Content area needs a grey border and shadow around it
Blocks: 594824
Posted image Border as in Windows Explorer (obsolete) —
With Photoshop put the frame of Windows Explorer on top Minefield.
Clearly keeping the proportions and color pixels.
Per shorlander.
blocking2.0: ? → betaN+
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;
}
I don't see any difference.
(In reply to comment #15)
> I don't see any difference.

before there wasn't border in panorama view
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.
replacing ThreedShadow with ThreeDDarkShadow the border become more dark
"suppose"... Are you sure that will be done?
there is some bug for that?
I dunno.
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;
}
Posted patch add border (obsolete) — Splinter Review
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.
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #476793 - Flags: review?(gavin.sharp)
Posted patch add border v2 (obsolete) — Splinter Review
realized that the border shouldn't be there when the window is maximized
Attachment #476793 - Attachment is obsolete: true
Attachment #476796 - Flags: review?(gavin.sharp)
Attachment #476793 - Flags: review?(gavin.sharp)
Actually I think even an opaque border might regress bug 590468. We'll see if talos complains.
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).
I can take a look at how your patch changes the layer tree next time I boot into Windows.
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.
We're currently using -moz-win-borderless-glass (bug 554982).
Assignee: dao → nobody
Status: ASSIGNED → NEW
Component: Theme → Widget: Win32
Product: Firefox → Core
QA Contact: theme → win32
Attachment #476796 - Flags: review?(gavin.sharp)
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?
Timothy said he was going to do some stuff. He should do that stuff and then reassign to Dão!
Assignee: nobody → tnikkel
Comment 28 supersedes what I was going to do.
Assignee: tnikkel → nobody
Assignee: nobody → dao
I'm not working on this since it's a widget bug now.
Assignee: dao → nobody
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?
(In reply to comment #34)
>  If we're following Stephen's new mockups...
Which new mockups?
(In reply to comment #35)
> Which new mockups?

I was referring to comment 30.
Component: Widget: Win32 → Theme
Product: Core → Firefox
QA Contact: win32 → theme
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?
Summary: Content area needs a grey border and shadow around it → Content area (and addons bar) needs a glass border and around it
(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.
Summary: Content area (and addons bar) needs a glass border and around it → Content area needs a grey border and shadow around it
Component: Theme → Widget
Product: Firefox → Core
QA Contact: theme → general
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?
Assignee: nobody → jmathies
felipe, maybe you can take this?
(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.
(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.)
Actually, just adding a new theme value and painting it normally in DrawWidgetBackground would regress bug 590468. We need another approach.
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).
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.
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.
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).
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.
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
(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?
Assignee: jmathies → nobody
Component: Widget → Theme
Product: Core → Firefox
QA Contact: general → theme
(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.
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.
It's actually fine to do whatever you want in those elements, as long as you keep them small.
Posted file 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
Posted patch WIP patch (obsolete) — Splinter Review
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?
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.
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.
Blocks: 601304
(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?
The new Addon manager magically creates a good looking border around the content, why?
Posted patch patchSplinter Review
Assignee: nobody → dao
Attachment #471123 - Attachment is obsolete: true
Attachment #475057 - Attachment is obsolete: true
Attachment #476796 - Attachment is obsolete: true
Attachment #479464 - Attachment is obsolete: true
Attachment #485474 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #493505 - Flags: review?(gavin.sharp)
Attachment #493505 - Flags: review?(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.
Attachment #493505 - Flags: review?(gavin.sharp)
Attachment #493505 - Flags: review?(dolske)
Attachment #493505 - Flags: review+
(In reply to comment #62)
> Out of curiosity, what's |layer="true"| do?

See bug 590468
http://hg.mozilla.org/mozilla-central/rev/a4544a4b3224
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b8
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;"
i think this has given the border above the address and below the bookmarks bar the glass effect.
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
Isn't this border way too light? Looks really bad in my eyes. Looks hardly better than before. :(
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
re-opening because this isn't right yet.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
We're not going to implement the shadow for reasons discussed in this bug. As for the border itself, file new bugs as needed.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Summary: Content area needs a grey border and shadow around it → Content area needs a grey border around it
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.
(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
I've filed Bug 618399 on the basic "it's too light" problem
(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.
Depends on: 622163
Depends on: 618353
No longer depends on: 622163
Blocks: 629629
No longer blocks: 601304
You need to log in before you can comment on or make changes to this bug.