Last Comment Bug 727689 - Clean up borders for attachment lists
: Clean up borders for attachment lists
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Theme (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: Thunderbird 13.0
Assigned To: Jim Porter (:squib)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-15 18:47 PST by Jim Porter (:squib)
Modified: 2012-03-27 01:07 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
Fix the borders (4.59 KB, patch)
2012-02-15 18:47 PST, Jim Porter (:squib)
bugs: ui‑review-
Details | Diff | Review
Before/after on Linux (16.27 KB, image/png)
2012-02-15 18:48 PST, Jim Porter (:squib)
no flags Details
Before/after on XP (15.96 KB, image/png)
2012-02-15 18:48 PST, Jim Porter (:squib)
no flags Details
Before/after on Aero (12.95 KB, image/png)
2012-02-15 18:48 PST, Jim Porter (:squib)
no flags Details
Fix Aero theme (4.79 KB, patch)
2012-02-16 17:27 PST, Jim Porter (:squib)
bwinton: review+
bugs: ui‑review+
standard8: approval‑comm‑aurora-
standard8: approval‑comm‑beta-
Details | Diff | Review
Fix XP theme (424 bytes, patch)
2012-02-24 19:08 PST, Jim Porter (:squib)
bugs: review+
bugs: ui‑review+
standard8: approval‑comm‑aurora+
Details | Diff | Review
attachmentbucket (5.07 KB, image/png)
2012-02-25 13:49 PST, Joe Sabash [:JoeS1]
no flags Details
win7 screenshot (8.15 KB, image/png)
2012-02-26 12:05 PST, Joe Sabash [:JoeS1]
no flags Details
n7.2 (9.30 KB, image/png)
2012-02-26 15:21 PST, Joe Sabash [:JoeS1]
no flags Details

Description Jim Porter (:squib) 2012-02-15 18:47:34 PST
Created attachment 597643 [details] [diff] [review]
Fix the borders

The borders on the attachment lists in the reader and composer are a bit ugly looking. We should fix this. Here's a patch, since I'm sick of looking at the ugliness every day. :)

I also made the total attachment size line up with each attachment's size in the composer.
Comment 1 Jim Porter (:squib) 2012-02-15 18:48:24 PST
Created attachment 597644 [details]
Before/after on Linux
Comment 2 Jim Porter (:squib) 2012-02-15 18:48:42 PST
Created attachment 597645 [details]
Before/after on XP
Comment 3 Jim Porter (:squib) 2012-02-15 18:48:58 PST
Created attachment 597646 [details]
Before/after on Aero
Comment 4 Andreas Nilsson (:andreasn) 2012-02-16 06:01:51 PST
Looks good on Linux. Will make sure it also works for all theme variants on Windows before I give ui-r+
Comment 5 Andreas Nilsson (:andreasn) 2012-02-16 15:11:55 PST
For some reason I keep getting a double border on Windows in the compose window still when I build this. I get the same results as in your screenshot only if I set the splitter to 1px and change the #FormatToolbar top-width to 0px

The attachment in a message looks nice so it the patch did bite.
Comment 6 Jim Porter (:squib) 2012-02-16 15:13:36 PST
Whoops, I probably screwed up when I merged all the patches together. I'll post a better one tonight.
Comment 7 Andreas Nilsson (:andreasn) 2012-02-16 15:16:13 PST
Upon closer inspection I noticed that you had the formatting bar turned off in your screenshot and this explains why I get a double border on the bottom and you don't. If you want to fix that in the code as well while you're poking around in that file it would be great!
Comment 8 Andreas Nilsson (:andreasn) 2012-02-16 15:16:55 PST
Comment on attachment 597643 [details] [diff] [review]
Fix the borders

setting ui-r minus since due to issues with the Windows theme part.
Comment 9 Jim Porter (:squib) 2012-02-16 17:27:19 PST
Created attachment 598073 [details] [diff] [review]
Fix Aero theme

This should resolve the issues with the Aero theme, including the format toolbar border.
Comment 10 Andreas Nilsson (:andreasn) 2012-02-16 17:51:05 PST
Comment on attachment 598073 [details] [diff] [review]
Fix Aero theme

Works as expected now!
ui-review+
Comment 11 Blake Winton (:bwinton) (:☕️) 2012-02-22 12:53:39 PST
Comment on attachment 598073 [details] [diff] [review]
Fix Aero theme

>+++ b/mail/themes/qute/mail/compose/messengercompose-aero.css
>@@ -418,12 +422,16 @@
>+#attachmentBucketSize {
>+  color: #888a85;
>+}

Where did this colour come from?

That's a pretty trivial question, so I'm going to say r=me.

Thanks,
Blake.
Comment 12 Jim Porter (:squib) 2012-02-22 13:30:11 PST
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #11)
> Comment on attachment 598073 [details] [diff] [review]
> Fix Aero theme
> 
> >+++ b/mail/themes/qute/mail/compose/messengercompose-aero.css
> >@@ -418,12 +422,16 @@
> >+#attachmentBucketSize {
> >+  color: #888a85;
> >+}
> 
> Where did this colour come from?

It originally came from the message header fields ("From" and the like). It's pretty commonly-used in our code (everywhere but XP): http://mxr.mozilla.org/comm-central/search?string=888a85

I do think we should turn that rule into "color: windowtext; opacity: 0.5;", which is what XP does. That's a bug for a different day though, but I did at least make XP consistent in its definitions here.
Comment 13 Jim Porter (:squib) 2012-02-23 23:11:35 PST
Checked in: http://hg.mozilla.org/comm-central/rev/dcc10c245fc7
Comment 14 Jim Porter (:squib) 2012-02-23 23:13:26 PST
Comment on attachment 598073 [details] [diff] [review]
Fix Aero theme

Should this go into aurora/beta? It's just a handful of minor CSS changes to clean things up visually. It's about as low-risk as it gets, and I think a little more prettiness always helps.
Comment 15 Jim Porter (:squib) 2012-02-24 19:08:54 PST
Created attachment 600602 [details] [diff] [review]
Fix XP theme

Whoops. I missed a single character in one of the selectors that caused the top border on the attachment pane in the reader to be invisible when it's collapsed on XP. Here's the fix.
Comment 16 Joe Sabash [:JoeS1] 2012-02-25 13:49:50 PST
Created attachment 600717 [details]
attachmentbucket

This is with xp classic default theme.
Needs more visible indication of a sizer on left border.
Easy to miss, if you don't know it's there.
Comment 17 Jim Porter (:squib) 2012-02-25 18:11:27 PST
That's intentional and has ui-review from andreasn.
Comment 18 Joe Sabash [:JoeS1] 2012-02-25 18:54:27 PST
(In reply to Jim Porter (:squib) from comment #17)
> That's intentional and has ui-review from andreasn.

OK, but it seems inconsistent with other panes that are re-sizable.
All those have a double border that defines the panes extent.
Still, even that is not very discoverable to the new user.
We do have the tooltip that expands long file names, but no visual cue that the pane can be expanded.
Comment 19 Bozz 2012-02-26 01:53:12 PST
I'm going to have to agree with Joe. The sizer should remain visible and consistent with the other panes so that users can easily 'discover' that the attachment bucket is resizable in order to see long file names.
Comment 20 Joe Sabash [:JoeS1] 2012-02-26 12:05:03 PST
Created attachment 600798 [details]
win7 screenshot

So..I have been reminded (elsewhere) that the winxp theme was the only theme that seemed to accentuate the sizer to the user.
Anyhow, here is what the attachmentbucket looks like in win7.
At the very least, the winxp rendering should at least delineate the expandable pane in the vertical.

@andreasn
Do we need a bug filed on the general discover ability of windows that can be re-sized.
Comment 21 Joe Sabash [:JoeS1] 2012-02-26 15:21:03 PST
Created attachment 600819 [details]
n7.2

Listen, I'm not meaning to spam this bug, but when we talk about "cleanup" and intuitive UI design, maybe we should look to the past sometimes and compare our current paradigm to that. I'm sure you will agree that the left border insinuates something different than a normal border (It's draggable) How do we convey that today. The clip is from Netscape 7.2
Comment 22 Mark Banner (:standard8) 2012-02-28 08:26:05 PST
Comment on attachment 598073 [details] [diff] [review]
Fix Aero theme

Looks like there's still issues here, so not taking for beta at this time.
Comment 23 Andreas Nilsson (:andreasn) 2012-03-01 08:39:48 PST
(In reply to Joe Sabash from comment #20)

> @andreasn
> Do we need a bug filed on the general discover ability of windows that can
> be re-sized.

Looking at some other Win7 applications (File Explorer and Windows Media Player, but also Firefox) it seems like the <-> cursor on hover to indicate resizing is a pretty well established pattern, so I think we're ok.
Comment 24 Andreas Nilsson (:andreasn) 2012-03-02 04:19:52 PST
Comment on attachment 600602 [details] [diff] [review]
Fix XP theme

Looks good!
Comment 25 Mark Banner (:standard8) 2012-03-12 06:36:06 PDT
So is this ready to land? Should we be getting the reviewed attachment landed before merge?
Comment 26 Jim Porter (:squib) 2012-03-18 13:01:31 PDT
Whoops, forgot about this. Checked in:

http://hg.mozilla.org/comm-central/rev/6af3f1722764
Comment 27 Jim Porter (:squib) 2012-03-18 13:02:08 PDT
Comment on attachment 600602 [details] [diff] [review]
Fix XP theme

This should go onto aurora, since the previous checkin happened before the merge.
Comment 28 Mark Banner (:standard8) 2012-03-19 06:40:24 PDT
Comment on attachment 598073 [details] [diff] [review]
Fix Aero theme

so this patch is now on aurora already, I'm not sure its that advantageous to take across to beta, unless Blake disagrees with me.
Comment 29 Mark Banner (:standard8) 2012-03-19 06:40:45 PDT
Comment on attachment 600602 [details] [diff] [review]
Fix XP theme

but yes, this patch should land on aurora now.
Comment 30 Mark Banner (:standard8) 2012-03-27 01:07:49 PDT
Checked in:

http://hg.mozilla.org/releases/comm-aurora/rev/a247b5a311e0

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