Last Comment Bug 777175 - Social side bar splitter should be styled like the standard side bar splitter
: Social side bar splitter should be styled like the standard side bar splitter
Status: RESOLVED FIXED
[Fx16]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-24 17:14 PDT by Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
Modified: 2012-08-01 19:40 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
slitter css (2.04 KB, patch)
2012-07-24 17:28 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
jaws: feedback+
Details | Diff | Review
slitter css (1.94 KB, patch)
2012-07-24 18:36 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
jaws: review+
dao+bmo: review-
Details | Diff | Review
new splitter css patch (4.16 KB, patch)
2012-07-26 13:56 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
dao+bmo: review-
Details | Diff | Review
new splitter css patch (3.93 KB, patch)
2012-07-31 10:57 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
no flags Details | Diff | Review
osx splitter (deleted)
2012-07-31 16:30 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
no flags Details
windows splitter (100.03 KB, image/png)
2012-07-31 16:31 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
jboriss: ui‑review+
Details
linux default splitter (48.11 KB, image/png)
2012-07-31 16:36 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
jboriss: ui‑review+
Details
osx splitter (94.71 KB, image/png)
2012-07-31 16:54 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
jboriss: ui‑review+
Details
new splitter css patch (4.06 KB, patch)
2012-07-31 16:57 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
dao+bmo: review-
Details | Diff | Review
new splitter css patch (4.18 KB, patch)
2012-07-31 17:28 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
no flags Details | Diff | Review
new splitter css patch (4.10 KB, patch)
2012-07-31 17:39 PDT, Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16]
dao+bmo: review+
Details | Diff | Review

Description Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-07-24 17:14:29 PDT
sidebar uses default xul css on the splitter, need to make it appear like the left sidebars.
Comment 1 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-07-24 17:24:12 PDT
we basically need to copy the styles for #devtools-side-splitter on all platforms.
Comment 2 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-07-24 17:28:12 PDT
Created attachment 645599 [details] [diff] [review]
slitter css
Comment 3 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-07-24 17:37:44 PDT
Comment on attachment 645599 [details] [diff] [review]
slitter css

Review of attachment 645599 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/winstripe/browser.css
@@ +3422,5 @@
>    max-height: 600px;
>    max-width: 400px;
>  }
>  
> +#social-sidebar-splitter {

Could you add the #social-sidebar-splitter selector to the rule for #devtools-sidebar-splitter for this and the other theme CSS files?
Comment 4 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-07-24 18:36:34 PDT
Created attachment 645617 [details] [diff] [review]
slitter css
Comment 5 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-07-24 22:53:34 PDT
Comment on attachment 645617 [details] [diff] [review]
slitter css

Review of attachment 645617 [details] [diff] [review]:
-----------------------------------------------------------------

Still get the review from Dao, but I think this is fine from my point of view.
Comment 6 Dão Gottwald [:dao] 2012-07-25 01:56:09 PDT
Comment on attachment 645617 [details] [diff] [review]
slitter css

#devtools-side-splitter uses a color that doesn't make sense here. You're also burying the #social-sidebar-splitter styling deep in devtools style code, which is undesirable.
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-25 08:46:45 PDT
We can instead just add a .sidebar-splitter class in browser.js, and use it for both splitters.
Comment 8 Dão Gottwald [:dao] 2012-07-25 09:51:32 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #7)
> We can instead just add a .sidebar-splitter class in browser.js, and use it
> for both splitters.

We couldn't use it for the generic side bar, though, since the positioning is reversed on the left side of the browser.
Comment 9 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-07-26 13:56:38 PDT
Created attachment 646319 [details] [diff] [review]
new splitter css patch

created new class for right side splitter, using devtools css but with corrected colors.  devtools class overrides the color to leave as-is for them.  not using a class on linux as the left sidebar splitter doesn't seem to have any custom css.

I've verified working on osx, but lack of build VMs on travel laptop leaves me unable to verify on other platforms.
Comment 10 Dão Gottwald [:dao] 2012-07-27 04:59:04 PDT
Comment on attachment 646319 [details] [diff] [review]
new splitter css patch

>+.sidebar-splitter-right {

This name doesn't make sense in RTL settings.

>+  -moz-border-start: 1px solid #404040;

Where did you get this color value from? Why not use a system color?

You didn't patch gnomestripe. Not sure if this is intentional.

On Windows, the generic side bar uses the default splitter styling except for aero glass and aero basic. Not sure why this splitter should be different.
Comment 11 Jennifer Morrow [:Boriss] (UX) 2012-07-28 20:20:39 PDT
This should get a UI review too.  Shane, it'd be awesome if you could post screenshots and/or screenshare with me when the patches are ready.
Comment 12 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-07-30 11:19:44 PDT
(In reply to Dão Gottwald [:dao] from comment #10)
> Comment on attachment 646319 [details] [diff] [review]
> new splitter css patch
> 
> >+.sidebar-splitter-right {
> 
> This name doesn't make sense in RTL settings.

I'm not clear how RTL will affect this sidebar.  There is the sidebar-splitter class for sidebars appearing on the left, this has tweaks to provide the same appearance of a sidebar on the right.

> >+  -moz-border-start: 1px solid #404040;
> 
> Where did you get this color value from? Why not use a system color?

It matches the left sidebar splitter:

https://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/browser.css#1654

> You didn't patch gnomestripe. Not sure if this is intentional.

There is no sidebar-splitter class for the left, I am assuming that the left uses the default xul styling for splitters.  I'm trying to simply match the right-side sidebar to the left.

> On Windows, the generic side bar uses the default splitter styling except
> for aero glass and aero basic. Not sure why this splitter should be
> different.

I'll change this to do the same.
Comment 13 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-07-30 11:22:11 PDT
(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #11)
> This should get a UI review too.  Shane, it'd be awesome if you could post
> screenshots and/or screenshare with me when the patches are ready.

We're matching the right sidebar splitter to that of the left (splitter for bookmarks and history sidebars) for each platform.  On OSX and aero that is the thin dark line.  Otherwise it is the default splitter style.  Does this need UI review and screenshots?
Comment 14 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-07-31 10:57:02 PDT
Created attachment 647597 [details] [diff] [review]
new splitter css patch

This patch only changes the css for OSX and Aero.  The colors and style match the left sidebar splitter.

I have opted to leave the developer tools splitter as-is since it does not match the splitters on a per-platform basis.

Non-aero and linux are left with the default style for splitters, which is how the left sidebar splitter is styled.

Boriss, do you need pictures, or is matching the left sidebar splitter fine?
Comment 15 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-07-31 16:30:56 PDT
Created attachment 647757 [details]
osx splitter
Comment 16 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-07-31 16:31:21 PDT
Created attachment 647758 [details]
windows splitter
Comment 17 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-07-31 16:32:15 PDT
posted the wrong pic, so made this confidential
Comment 18 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-07-31 16:36:11 PDT
Created attachment 647762 [details]
linux default splitter
Comment 19 Dave Miller [:justdave] (justdave@bugzilla.org) 2012-07-31 16:43:16 PDT
The content of attachment 647757 [details] has been deleted by
    Dave Miller [:justdave] <justdave@mozilla.com>
who provided the following reason:

Uploader requested deletion of unsanitized data

The token used to delete this attachment was generated at 2012-07-31 16:43:02 PDT.
Comment 20 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-07-31 16:54:21 PDT
Created attachment 647768 [details]
osx splitter
Comment 21 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-07-31 16:57:58 PDT
Created attachment 647769 [details] [diff] [review]
new splitter css patch

adding 3px width to make grabbing easier, matches what is done on windows
Comment 22 Dão Gottwald [:dao] 2012-07-31 17:09:35 PDT
Comment on attachment 647769 [details] [diff] [review]
new splitter css patch

You can use ".sidebar-splitter" and "#appcontent ~ .sidebar-splitter" to discern left and right splitters.
Comment 23 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-07-31 17:28:05 PDT
Created attachment 647779 [details] [diff] [review]
new splitter css patch

cool, updated
Comment 24 Dão Gottwald [:dao] 2012-07-31 17:31:47 PDT
Comment on attachment 647779 [details] [diff] [review]
new splitter css patch

>+  .sidebar-splitter {
>     border: 0;
>+    -moz-border-start: none;
>     -moz-border-end: 1px solid #A9B7C9;
>     min-width: 0;
>     width: 3px;
>     background-color: transparent;
>     -moz-margin-start: -3px;
>+    -moz-margin-end: 0;
>     position: relative;
>   }

What's the point of these added lines?
Comment 25 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-07-31 17:39:44 PDT
Created attachment 647789 [details] [diff] [review]
new splitter css patch

removed extra unnecessary css
Comment 26 Jennifer Morrow [:Boriss] (UX) 2012-07-31 17:45:18 PDT
Comment on attachment 647768 [details]
osx splitter

+'d, and thank you for attaching screenshots!
Comment 27 Shane Caraveo (:mixedpuppy) [on leave 5/16-7/16] 2012-07-31 17:58:46 PDT
passing to gavin for landing
Comment 28 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-01 12:08:05 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c835504961b
Comment 29 Ryan VanderMeulen [:RyanVM] 2012-08-01 19:40:42 PDT
https://hg.mozilla.org/mozilla-central/rev/9c835504961b

Note You need to log in before you can comment on or make changes to this bug.