Closed Bug 426000 Opened 16 years ago Closed 16 years ago

Splitters for the bookmarks and history sidebars should be 1 pixel wide on Vista

Categories

(Firefox :: Theme, defect)

x86
Windows Vista
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 3

People

(Reporter: faaborg, Assigned: ehsan.akhgari)

References

Details

Attachments

(3 files, 2 obsolete files)

The splitters for the bookmarks and history sidebars should be 1 pixel wide on Vista.  This is similar to the changes made in the Library window (bug 403138).  Note that the splitters should be visually 1 pixel wide, their actual width can remain larger for usability. The color of the 1 pixel splitter should be #a9b7c9
The CSS fix should be this, tried using Stylish on Firefox3b4:

#sidebar-splitter{
 border: 0 !important;
 -moz-border-right-colors: none !important;
 border-right: 1px solid #a9b7c9 !important;
 background-color: transparent !important;
 min-width: 0 !important;
}

This applies to *any* sidebar of the browser content area. Might affect
sidebar-related extensions as well.
From an email conversation with beltzner:

>>-style vista sidebars
>
>blocking: this is just the CSS fixes?

Yes, and one image change (bug 425998)
Flags: blocking-firefox3?
Flags: blocking-firefox3? → blocking-firefox3+
Note that we need to fall back to a system color for accessibility, setting to depend on 425598
Depends on: 425598
(In reply to comment #3)
> Note that we need to fall back to a system color for accessibility

Which system color?
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
For a theme that isn't Aero, we need to use ThreeDFace, but this will result in black on black for a high contrast theme, so for the accessibility pref we need to use ThreeDHighlight, which I believe will result in white on black.  The two related bugs are bug 425598 and bug 426660
We should use ThreeDLightShadow, ThreedShadow or ThreedDarkShadow. Neither ThreeDFace nor ThreeDHighlight are right for this.
Sure, fwiw Vista's Windows explorer uses ThreeDFace in classic mode for splitters, but it certainly doesn't look like they put too much thought into it.  I just went with what matched the file system view in classic mode.
It's possible that ThreeDLightShadow looks just like ThreeDFace; probably depends on the theme. That said, it's also entirely possible that they messed it up in Explorer.
Depends on: 426660
Here is what explorer looks like in classic mode.
Oh, right, so it's not 1 pixel wide. I this case, it's debatable whether this is a border or part of the face.
I guess with our fancy new selectors we can make the splitter wider if we we are not in Aero.  Perhaps matching our style on XP would look the best on classic in Vista.
Or the style of the splitters for the sidebars right now, library window on XP is using collapsible splitters at the moment.
(In reply to comment #10)
With high contrast themes in mind, I think I'd still prefer to see it as a border and use ThreeDLightShadow.

(In reply to comment #12)
> Or the style of the splitters for the sidebars right now, library window on XP
> is using collapsible splitters at the moment.

In that case, we'd probably just have to add this:

#sidebar-splitter:-moz-system-metric(windows-default-theme) {
  border: none !important;
  min-width: 1px !important;
  background-color: #a9b7c9 !important;
}

(Styling based on comment 1. Not sure if the "important" flags are needed.)
(In reply to comment #0)
> their actual width can remain larger for usability.

Without this, there would really be usability problem.
But I imagine this would produce a visible gap between the splitter and the sidebar header, or between the splitter and the tab bar / content area.
(In reply to comment #11)
> I guess with our fancy new selectors we can make the splitter wider if we we
> are not in Aero.  Perhaps matching our style on XP would look the best on
> classic in Vista.

I'm really wondering if this new selectors thing can work on-the-fly? I mean say, I'm using Aero and the splitter is 1px wide. While doing that, I change to Classic, the splitter changes to 3-4px wide?
Ehsan, do you have a plan for addressing the usability problem? If not, I think blocking needs to be reconsidered.
My impression was that the userChrome in comment #1 keeps the actual target area the same, with a one pixel visual target area, which (if it works) should be fine from a usability perspective.

>I'm really wondering if this new selectors thing can work on-the-fly? I mean
>say

Theme changing in windows doesn't really work on the fly for most apps, it is usually accepted that closing and reopening the app after a theme change is required to pick up the new appearance, especially on Vista.  On linux you have to reopen Firefox as well for a theme change, so I don't think that alone should stop us from making a better default appearance.
(In reply to comment #17)
> My impression was that the userChrome in comment #1 keeps the actual target
> area the same, with a one pixel visual target area, which (if it works) should
> be fine from a usability perspective.

It makes the actual target area one pixel wide, which I find easy to miss.
(In reply to comment #17)
> My impression was that the userChrome in comment #1 keeps the actual target
> area the same, with a one pixel visual target area, which (if it works) should
> be fine from a usability perspective.

I don't see how it would be possible to make the visual target area 1px wide while keeping the actual target area >1px wide in CSS...  If the splitter element is supposed to be more than 1px wide, then rendering it to 1px only would leave some left-over space unrendered (possibly with the background color, which is white by default) and it could be very ugly, unless we do something like this:

set the width of the element to 5px (for example)
set the right border to 1px solid ThreeDLightShadow (or whatever)
set the background color to transparent
make sure the splitter is positioned on top of the other elements (for example by setting its position to absolute, and adjusting its position and z-index)

This should imitate the Vista splitter in Explorer pretty closely I suppose.
(In reply to comment #19)
> make sure the splitter is positioned on top of the other elements (for example
> by setting its position to absolute, and adjusting its position and z-index)

Even if that's possible with XUL elements (I don't think it is), it would probably prevent the splitter from functioning.
(In reply to comment #20)
> (In reply to comment #19)
> > make sure the splitter is positioned on top of the other elements (for example
> > by setting its position to absolute, and adjusting its position and z-index)
> 
> Even if that's possible with XUL elements (I don't think it is)

I'm also not sure how to do it...

> it would
> probably prevent the splitter from functioning.

Why?
(In reply to comment #21)

> > it would
> > probably prevent the splitter from functioning.
> 
> Why?

I've tried what you said. And the splitter can't function. By setting margin-left: -[X]px;, it moves the splitter a bit but the sidebar overlaps on it. The larger the [X], the splitter goes 'behind' the sidebar. I've tried margin-right, but the splitter goes 'behind' the web page content area as well.
(In reply to comment #22)
> I've tried what you said. And the splitter can't function. By setting
> margin-left: -[X]px;, it moves the splitter a bit but the sidebar overlaps on
> it. The larger the [X], the splitter goes 'behind' the sidebar. I've tried
> margin-right, but the splitter goes 'behind' the web page content area as well.

Can you share the stylesheet you're using?  Have you tried z-index: some_large_number on the splitter?
(In reply to comment #23)
> (In reply to comment #22)
> > I've tried what you said. And the splitter can't function. By setting
> > margin-left: -[X]px;, it moves the splitter a bit but the sidebar overlaps on
> > it. The larger the [X], the splitter goes 'behind' the sidebar. I've tried
> > margin-right, but the splitter goes 'behind' the web page content area as well.
> 
> Can you share the stylesheet you're using?  Have you tried z-index:
> some_large_number on the splitter?

I've tried z-index, but to no avail. Example:

#sidebar-splitter{
 border: 0 !important;
 -moz-border-right-colors: none !important;
 border-right: 1px solid #a9b7c9 !important;
 margin-left: -10px;
}
(In reply to comment #24)
> I've tried z-index, but to no avail. Example:
> 
> #sidebar-splitter{
>  border: 0 !important;
>  -moz-border-right-colors: none !important;
>  border-right: 1px solid #a9b7c9 !important;
>  margin-left: -10px;
> }

I'm not sure what you've tried exactly, since your example doesn't include z-index, but the following userChrome.css changes work beautifully for me:

#sidebar-splitter{
 border: 0 !important;
 -moz-border-right-colors: none !important;
 border-right: 1px solid #a9b7c9 !important;
 min-width: 0 !important;
 width: 3px !important;
 background-color: transparent !important;
 margin-left: -3px;
 position: relative;
 z-index: 10;
}

I'll attach some screenshots.  The splitter remains fully functional, and the visual target area is just a 1px wide border-right, while the actual target area is 3px wide.  Wider target areas (such as 5px) interfere with the operation of the scrollbar when the history sidebar is expanded, but we could easily try them if desired.
z-index works with position:fixed|absolute|relative only.

(In reply to comment #21)
> > it would
> > probably prevent the splitter from functioning.
> 
> Why?

I suppose the code that resizes the siblings assumes that the splitter is laid out between rather than on top of them.
Neither GMIP nor PrintScreen could grab screenshots including the mouse pointer, but this should be trivial to test in order to get a feel of the target area.
(In reply to comment #26)
> (In reply to comment #21)
> > > it would
> > > probably prevent the splitter from functioning.
> > 
> > Why?
> 
> I suppose the code that resizes the siblings assumes that the splitter is laid
> out between rather than on top of them.

Note that this applies to absolute/fixed positioning. If relative works, that's probably fine.
(In reply to comment #26)
> z-index works with position:fixed|absolute|relative only.

No, it only works with positioned elements (that is, any element with position set to anything other than auto I guess).

> (In reply to comment #21)
> > > it would
> > > probably prevent the splitter from functioning.
> > 
> > Why?
> 
> I suppose the code that resizes the siblings assumes that the splitter is laid
> out between rather than on top of them.

Have you tried the code I posted in comment 25?  It works perfectly.  The splitter is fully functional...
(In reply to comment #29)
> (In reply to comment #26)
> > z-index works with position:fixed|absolute|relative only.
> 
> No, it only works with positioned elements (that is, any element with position
> set to anything other than auto I guess).

It doesn't work with position:static.

> Have you tried the code I posted in comment 25?  It works perfectly.  The
> splitter is fully functional...

see comment 28. position:relative leaves the surrounding layout in place as if the element was positioned statically.
(In reply to comment #25)

> I'm not sure what you've tried exactly, since your example doesn't include
> z-index, but the following userChrome.css changes work beautifully for me:
> 
> #sidebar-splitter{
>  border: 0 !important;
>  -moz-border-right-colors: none !important;
>  border-right: 1px solid #a9b7c9 !important;
>  min-width: 0 !important;
>  width: 3px !important;
>  background-color: transparent !important;
>  margin-left: -3px;
>  position: relative;
>  z-index: 10;
> }

Alright, I tried this and it surprisingly works. But, 'position: relative' seems dangerous...
(In reply to comment #30)
> see comment 28. position:relative leaves the surrounding layout in place as if
> the element was positioned statically.

So I guess you agree that this approach is fine, then.
(In reply to comment #31)
> Alright, I tried this and it surprisingly works. But, 'position: relative'
> seems dangerous...

How come?
From my understanding, XUL UI is grid-based layout, so relative positioning doesn't sound right, at least for me.
(In reply to comment #32)
> (In reply to comment #30)
> > see comment 28. position:relative leaves the surrounding layout in place as if
> > the element was positioned statically.
> 
> So I guess you agree that this approach is fine, then.

Yes, although I also agree that it looks fragile / potentially dangerous. But if you tested it and get it (ui-)reviewed, that should be fine.
Comment on attachment 313583 [details]
Screenshot of the code in comment 25

Requesting ui-review on both the look of the implementation and its behavior.  Once it gets ui-r+ and bug 426660 lands, I'll provide a patch for review.
Attachment #313583 - Flags: ui-review?(faaborg)
Whiteboard: [has patch][needs ui-review faaborg]
Blocks: 405605
I think whoever does the UI review needs a patch in order to test if the target are is big enough, doesn't steal space from the scroll bar, etc.
Attached patch Patch (v1) (obsolete) — Splinter Review
(In reply to comment #37)
> I think whoever does the UI review needs a patch in order to test if the target
> are is big enough, doesn't steal space from the scroll bar, etc.

You're right.  Here's the patch to evaluate this.
No longer depends on: 425598
Comment on attachment 313583 [details]
Screenshot of the code in comment 25

Switching ui-review to beltzner to get his feedback.
Attachment #313583 - Flags: ui-review?(faaborg) → ui-review?(beltzner)
Attachment #313913 - Flags: ui-review?(beltzner)
Attachment #313583 - Flags: ui-review?(beltzner)
Whiteboard: [has patch][needs ui-review faaborg] → [has patch][needs ui-review beltzner]
I moved the ui-review request to the patch to make it clear that the behavior needs to be tested.
Comment on attachment 313913 [details] [diff] [review]
Patch (v1)

This is fine, yes, with two nits:

Nit1: On Vista there's actually a 1px pad between the keyline splitter and any adjescent UI. Can we get that here, as well?

Nit2: This needs to also be used for the splitters used in the Library, which I think are a slightly different widget?
Attachment #313913 - Flags: ui-review?(beltzner) → ui-review+
Whiteboard: [has patch][needs ui-review beltzner] → [has patch]
Attachment #313913 - Flags: review+
Attachment #313913 - Flags: approval1.9+
Whiteboard: [has patch] → [has patch][has reviews][has approval]
(In reply to comment #41)
> Nit2: This needs to also be used for the splitters used in the Library, which I
> think are a slightly different widget?

i've merged this splitter code in my Library experimental patch for Vista (bug 403147) and feels like working fine (apart putting margin-left: -4px; feels like more coherent with the OS, with 3px there's too much distance between scrollbar and the splitter line). code is already there so can be easily plugged off, moved, merged, as you will.

Whiteboard: [has patch][has reviews][has approval] → [patch needs to address nits]
Looks like a new patch is needed here?
Keywords: checkin-needed
Attached patch Patch (v1.1) (obsolete) — Splinter Review
(In reply to comment #41)
> Nit1: On Vista there's actually a 1px pad between the keyline splitter and any
> adjescent UI. Can we get that here, as well?

I don't know how that can be done.

> Nit2: This needs to also be used for the splitters used in the Library, which I
> think are a slightly different widget?

Nope, it's the same widget.

The new patch addresses the splitter in the Library as well, and also makes the code RTL-friendly.
Attachment #313913 - Attachment is obsolete: true
Attachment #317483 - Flags: ui-review?(beltzner)
Attachment #317483 - Flags: review?(mconnor)
Whiteboard: [patch needs to address nits] → [has patch][needs review mconnor][needs ui-review beltzner]
should not you use
 
#placesView > splitter:-moz-system-metric(windows-default-theme) {

and maybe also in the sidebar... Are we going to hardcode color and one line also for classic theme?
Let's have Beltzner's idea on this.
Comment on attachment 317483 [details] [diff] [review]
Patch (v1.1)

Yeah, we need -moz-metric(windows-default-theme) here. On classic (/me shakes fist!) the sidebars are as they ever were.
Attachment #317483 - Flags: ui-review?(beltzner) → ui-review-
Whiteboard: [has patch][needs review mconnor][needs ui-review beltzner] → [needs small patch update]
Attached patch Patch (v1.2)Splinter Review
(In reply to comment #47)
> (From update of attachment 317483 [details] [diff] [review])
> Yeah, we need -moz-metric(windows-default-theme) here. On classic (/me shakes
> fist!) the sidebars are as they ever were.

Done.
Attachment #317483 - Attachment is obsolete: true
Attachment #317562 - Flags: ui-review?(beltzner)
Attachment #317562 - Flags: review?(mconnor)
Attachment #317483 - Flags: review?(mconnor)
Whiteboard: [needs small patch update] → [has patch][needs review mconnor][needs ui-review beltzner]
Comment on attachment 317562 [details] [diff] [review]
Patch (v1.2)

r+uir+a=beltzner
Attachment #317562 - Flags: ui-review?(beltzner)
Attachment #317562 - Flags: ui-review+
Attachment #317562 - Flags: review?(mconnor)
Attachment #317562 - Flags: review+
Attachment #317562 - Flags: approval1.9+
Whiteboard: [has patch][needs review mconnor][needs ui-review beltzner] → [has patch][has reviews][has approval]
Keywords: checkin-needed
mozilla/browser/themes/winstripe/browser/browser-aero.css 	1.8
mozilla/browser/themes/winstripe/browser/jar.mn 	1.92
mozilla/browser/themes/winstripe/browser/places/organizer-aero.css 	1.1 
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch][has reviews][has approval]
Target Milestone: --- → Firefox 3
Blocks: 430860
Verified with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008050606 Minefield/3.0pre ID:2008050606
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.