Last Comment Bug 780425 - Add Aero theming to attachment lists
: Add Aero theming to attachment lists
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Theme (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: Thunderbird 18.0
Assigned To: Jim Porter (:squib)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-04 16:44 PDT by Jim Porter (:squib)
Modified: 2012-09-25 13:22 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed


Attachments
Aero-style attachment highlighting (9.73 KB, image/png)
2012-08-04 16:44 PDT, Jim Porter (:squib)
no flags Details
Patch alternative 1 (3.96 KB, patch)
2012-08-06 03:52 PDT, Richard Marti (:Paenglab)
no flags Details | Diff | Splinter Review
Patch alternative 2 (4.47 KB, patch)
2012-08-06 03:56 PDT, Richard Marti (:Paenglab)
no flags Details | Diff | Splinter Review
Comparison of both alternatives (25.94 KB, image/png)
2012-08-06 04:01 PDT, Richard Marti (:Paenglab)
no flags Details
My patch (7.63 KB, patch)
2012-08-06 11:18 PDT, Jim Porter (:squib)
richard.marti: feedback+
Details | Diff | Splinter Review
My userChrome (6.41 KB, text/css)
2012-08-06 11:26 PDT, Jim Porter (:squib)
no flags Details
Abbreviated text (5.99 KB, image/png)
2012-08-06 12:43 PDT, Richard Marti (:Paenglab)
no flags Details
Comparison: left without right with border-box (5.32 KB, image/png)
2012-08-06 14:40 PDT, Richard Marti (:Paenglab)
no flags Details
What attachment 649326 looks like (62.28 KB, image/png)
2012-08-06 18:51 PDT, Jim Porter (:squib)
bwinton: ui‑review-
Details
Updated UI (63.36 KB, image/png)
2012-08-08 21:45 PDT, Jim Porter (:squib)
no flags Details
Updated UI (for real) (62.28 KB, image/png)
2012-08-08 22:05 PDT, Jim Porter (:squib)
bwinton: ui‑review+
Details
Updated patch (8.88 KB, patch)
2012-08-09 19:44 PDT, Jim Porter (:squib)
bwinton: review+
richard.marti: feedback+
Details | Diff | Splinter Review
FIx Mozmill tests (9.97 KB, patch)
2012-08-22 00:46 PDT, Jim Porter (:squib)
bwinton: review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Splinter Review

Description Jim Porter (:squib) 2012-08-04 16:44:40 PDT
Created attachment 649042 [details]
Aero-style attachment highlighting

Right now, the attachment list uses old-style highlighting for the items. We should make it look like Aero, like we did with the thread/folder panes. Attached is a screenshot of what it could look like.
Comment 1 Richard Marti (:Paenglab) 2012-08-06 03:52:25 PDT
Created attachment 649230 [details] [diff] [review]
Patch alternative 1

Here is a patch to style like Squib's screenshot.
Comment 2 Richard Marti (:Paenglab) 2012-08-06 03:56:27 PDT
Created attachment 649233 [details] [diff] [review]
Patch alternative 2

This patch highlights the whole item like Explorer does. On short attachment names it's also easier to select the item because now the whole item is selectable. I also ask for review of this patch, so you can choose which one you like more.
Comment 3 Richard Marti (:Paenglab) 2012-08-06 04:01:05 PDT
Created attachment 649236 [details]
Comparison of both alternatives

Three items are selected and IMG_0268.jpg is selected and current. It's only a slight difference like in Explorer and TB's lists.
Comment 4 Jim Porter (:squib) 2012-08-06 10:50:36 PDT
I already have a patch in progress for this (hence why I was assigned).
Comment 5 Jim Porter (:squib) 2012-08-06 11:18:33 PDT
Created attachment 649326 [details] [diff] [review]
My patch

Here's my patch. Significantly, this changes the selection style for the alternate attachment views (mailnews.attachments.display.view set to 1 or 2) to be closer to how Windows works. It also has a 1px margin between items so that the borders don't double up.
Comment 6 Jim Porter (:squib) 2012-08-06 11:26:52 PDT
Created attachment 649328 [details]
My userChrome

Also, for what it's worth, here's the userChrome I used to test this out, which may be a better base if we're going to override attachmentList.css instead of just replacing it.

One significant difference in our patches is how we handle margins for the items, which I think is important. I also made sure that, in the default view, the rows are exactly the same size as the XUL <tree> rows. That way, everything looks consistent.
Comment 7 Richard Marti (:Paenglab) 2012-08-06 12:40:28 PDT
Comment on attachment 649326 [details] [diff] [review]
My patch

Sorry, I overlooked it was assigned to you.

You're right the old file is to small to include it and override the rules. A new file with all rules is better.

About the partial highlight in mailnews.attachments.display.view=0 we should let Blake decide if this is okay or the whole item should be selected.

Two small things: when you add 'background-origin: border-box;' instead of 'background-size: 100% 100%;' on line 50 then the white inner border is visible.
When you apply the border directly to the attachmentitem then the width calculation isn't correct and the .attachmentcell-name will be abbreviated (I'll attach a screenshot). I had the same problem with alternative 2. When you apply the border to .attachmentcell-content then it works as before.

I cancel my two patches and let you do this.
Comment 8 Richard Marti (:Paenglab) 2012-08-06 12:43:24 PDT
Created attachment 649357 [details]
Abbreviated text

Example of the abbreviated text.

I let my comparison for Blake, if you are okay with it.
Comment 9 Jim Porter (:squib) 2012-08-06 13:47:07 PDT
(In reply to Richard Marti [:paenglab] from comment #7)
> You're right the old file is to small to include it and override the rules.
> A new file with all rules is better.

Well, should we *always* use Aero theming? Your patch only enables it when the OS is using Aero. That's the main difference between our approaches.

> Two small things: when you add 'background-origin: border-box;' instead of
> 'background-size: 100% 100%;' on line 50 then the white inner border is
> visible.

I'm not sure I see the difference.

(In reply to Richard Marti [:paenglab] from comment #8)
> Created attachment 649357 [details]
> Abbreviated text
> 
> Example of the abbreviated text.

With mailnews.attachments.display.view == 2 (tile view), we have a pretty small max-width, which is what you're seeing there. However, for ...view == 1 (large view), the abbreviation shouldn't happen. I think the way to fix this is in the Javascript that determines the size of the items. That way, anyone writing custom themes won't bump into this.
Comment 10 Richard Marti (:Paenglab) 2012-08-06 14:40:19 PDT
Created attachment 649425 [details]
Comparison: left without right with border-box

(In reply to Jim Porter (:squib) from comment #9)
> (In reply to Richard Marti [:paenglab] from comment #7)
> > You're right the old file is to small to include it and override the rules.
> > A new file with all rules is better.
> 
> Well, should we *always* use Aero theming? Your patch only enables it when
> the OS is using Aero. That's the main difference between our approaches.

The treechildren are only Aero styled when on default theme, not on Classic or High Contrast. Also Explorer don't use this styling on Classic or High Contrast.

> > Two small things: when you add 'background-origin: border-box;' instead of
> > 'background-size: 100% 100%;' on line 50 then the white inner border is
> > visible.
> 
> I'm not sure I see the difference.

It's very slightly. You can see it only on the top border. On bug 774090 it is better visible.

> With mailnews.attachments.display.view == 2 (tile view), we have a pretty
> small max-width, which is what you're seeing there. However, for ...view ==
> 1 (large view), the abbreviation shouldn't happen. I think the way to fix
> this is in the Javascript that determines the size of the items. That way,
> anyone writing custom themes won't bump into this.

It is only the difference of the 4px for the border. On the right on the screenshot I've applied my proposal. But I agree it would be best to fix this in the JavaScript function.
Comment 11 Jim Porter (:squib) 2012-08-06 18:51:33 PDT
Created attachment 649519 [details]
What attachment 649326 [details] [diff] [review] looks like

Here's what my patch looks like in the reader and composer with all the available view options. Blake, what do you think? Specifically, do you like the top left/right images as is, or should the selection cover the entire item? (The current UI only selects the name.)
Comment 12 Blake Winton (:bwinton) (:☕️) 2012-08-08 11:42:15 PDT
Comment on attachment 649519 [details]
What attachment 649326 [details] [diff] [review] looks like

I think it should cover the entire item, because the addition of the size makes those views feel more like Windows Explorer's Details view than like the List view, and Details view highlights the entire line.

(But aside from that, I really like how the new styling fits in with the rest of Windows 7.)
Comment 13 Jim Porter (:squib) 2012-08-08 21:45:53 PDT
Created attachment 650440 [details]
Updated UI

Here's an updated view of what it looks like. Note that there's no left/right margins on the vertical attachment list. This is intentional, and is designed to match the thread pane exactly.
Comment 14 Jim Porter (:squib) 2012-08-08 22:05:06 PDT
Created attachment 650441 [details]
Updated UI (for real)

Actually, *this* is what it looks like now. The margins are a bit different on the bottom two images. Note that the icon looks off-center because that's just how the icon is drawn; most icons fill the space properly.
Comment 15 Jim Porter (:squib) 2012-08-09 19:44:15 PDT
Created attachment 650764 [details] [diff] [review]
Updated patch

Ok, here's the patch.
Comment 16 Jim Porter (:squib) 2012-08-09 19:46:15 PDT
Comment on attachment 650764 [details] [diff] [review]
Updated patch

Let's try to get this into 16 to line up with bug 686959 and bug 774090.
Comment 17 Richard Marti (:Paenglab) 2012-08-10 01:25:13 PDT
Comment on attachment 650764 [details] [diff] [review]
Updated patch

This looks good
Comment 18 Blake Winton (:bwinton) (:☕️) 2012-08-20 07:41:43 PDT
Comment on attachment 650441 [details]
Updated UI (for real)

Looks good to me.  ui-r+!
Comment 19 Blake Winton (:bwinton) (:☕️) 2012-08-20 07:56:09 PDT
Comment on attachment 650764 [details] [diff] [review]
Updated patch

>+++ b/mail/base/content/mailWidgets.xml
>@@ -358,21 +362,27 @@
>       <method name="setOptimumWidth">
>         <body><![CDATA[
>+          if (this._childNodes.length == 0)
>+            return;

Won't that give us the same width as we had previously?  But I guess that doesn't matter since in this case the element won't be shown?

>+++ b/mail/themes/qute/mail/attachmentList-aero.css
>@@ -0,0 +1,107 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Mmmmm, MPL 2, how I love your tiny, tiny header.  :)

And I've hit the end, so I guess that means r=me, if my question above is true.

Thanks,
Blake.
Comment 20 Jim Porter (:squib) 2012-08-20 11:35:23 PDT
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #19)
> Comment on attachment 650764 [details] [diff] [review]
> Updated patch
> 
> >+++ b/mail/base/content/mailWidgets.xml
> >@@ -358,21 +362,27 @@
> >       <method name="setOptimumWidth">
> >         <body><![CDATA[
> >+          if (this._childNodes.length == 0)
> >+            return;
> 
> Won't that give us the same width as we had previously?  But I guess that
> doesn't matter since in this case the element won't be shown?

Right. If there aren't any child nodes, then there's nothing for us to apply the calculated width to, so we can just bail out early.
Comment 21 Jim Porter (:squib) 2012-08-21 01:08:19 PDT
Checked in: https://hg.mozilla.org/comm-central/rev/4cc9ac6460cf
Comment 22 Mark Banner (:standard8) 2012-08-21 05:28:16 PDT
Backed out due to unit test failures: https://hg.mozilla.org/comm-central/rev/658238405a6b

https://tbpl.mozilla.org/php/getParsedLog.php?id=14555484&tree=Thunderbird-Trunk#error0

Test Failure: Attachment size should not be displayed!
TEST-UNEXPECTED-FAIL | C:\talos-slave\test\build\mozmill\attachment\test-attachment-size.js | test-attachment-size.js::test_detached_attachment_with_missing_file
TEST-START | C:\talos-slave\test\build\mozmill\attachment\test-attachment-size.js | test_deleted_attachment
Test Failure: Attachment size should not be displayed!
TEST-UNEXPECTED-FAIL | C:\talos-slave\test\build\mozmill\attachment\test-attachment-size.js | test-attachment-size.js::test_deleted_attachment
...
Test Failure: Attachment size should not be displayed!
TEST-UNEXPECTED-FAIL | C:\talos-slave\test\build\mozmill\attachment\test-attachment-size.js | test-attachment-size.js::test_multiple_attachments_one_detached_with_missing_file
TEST-START | C:\talos-slave\test\build\mozmill\attachment\test-attachment-size.js | test_multiple_attachments_one_deleted
Test Failure: Attachment size should not be displayed!
TEST-UNEXPECTED-FAIL | C:\talos-slave\test\build\mozmill\attachment\test-attachment-size.js | test-attachment-size.js::test_multiple_attachments_one_deleted
...
TEST-START | C:\talos-slave\test\build\mozmill\composition\test-attachment.js | test_webpage_attachment
Step Pass: {"function": "Controller.keypress()"}
search "ldap" not found - skipping.
search "ldap" not found - skipping.
Test Failure: Attachment size should not be displayed!
search "ldap" not found - skipping.
TEST-UNEXPECTED-FAIL | C:\talos-slave\test\build\mozmill\composition\test-attachment.js | test-attachment.js::test_webpage_attachment
Comment 23 Jim Porter (:squib) 2012-08-22 00:46:15 PDT
Created attachment 654123 [details] [diff] [review]
FIx Mozmill tests

Whoops, I forgot to update that test. Here's a simple fix for that; I've confirmed that the test passes locally now.
Comment 24 Blake Winton (:bwinton) (:☕️) 2012-08-22 06:45:55 PDT
Comment on attachment 654123 [details] [diff] [review]
FIx Mozmill tests

r=me, based on manual inspection of the code change and the test change.
Comment 25 Jim Porter (:squib) 2012-09-03 11:46:35 PDT
Checked in: https://hg.mozilla.org/comm-central/rev/6392d78f3459
Comment 26 Jim Porter (:squib) 2012-09-03 13:30:34 PDT
Fixed some bustage with a similar function in the compose tests: https://hg.mozilla.org/comm-central/rev/3be4b5bbe73c
Comment 27 Jim Porter (:squib) 2012-09-07 02:20:57 PDT
Comment on attachment 654123 [details] [diff] [review]
FIx Mozmill tests

[Approval Request Comment]
Regression caused by (bug #): N/A
User impact if declined: Slightly inconsistent UI
Testing completed (on c-c, etc.): Tested on C-C and baked for a few days
Risk to taking this patch (and alternatives if risky): Risk is low, but it would be nice to get this into 16 to line up with bug 686959 and bug 774090.

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