Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Add Aero theming to attachment lists

RESOLVED FIXED in Thunderbird 18.0

Status

Thunderbird
Theme
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: squib, Assigned: squib)

Tracking

Trunk
Thunderbird 18.0
x86_64
Windows 7

Thunderbird Tracking Flags

(thunderbird16 fixed, thunderbird17 fixed)

Details

Attachments

(7 attachments, 6 obsolete attachments)

(Assignee)

Description

5 years ago
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.
Created attachment 649230 [details] [diff] [review]
Patch alternative 1

Here is a patch to style like Squib's screenshot.
Attachment #649230 - Flags: ui-review?(bwinton)
Attachment #649230 - Flags: review?(bwinton)
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.
Attachment #649233 - Flags: ui-review?(bwinton)
Attachment #649233 - Flags: review?(bwinton)
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.
(Assignee)

Comment 4

5 years ago
I already have a patch in progress for this (hence why I was assigned).
(Assignee)

Comment 5

5 years ago
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.
Attachment #649326 - Flags: feedback?(richard.marti)
(Assignee)

Comment 6

5 years ago
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 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.
Attachment #649326 - Flags: feedback?(richard.marti) → feedback+
Created attachment 649357 [details]
Abbreviated text

Example of the abbreviated text.

I let my comparison for Blake, if you are okay with it.
Attachment #649230 - Attachment is obsolete: true
Attachment #649233 - Attachment is obsolete: true
Attachment #649230 - Flags: ui-review?(bwinton)
Attachment #649230 - Flags: review?(bwinton)
Attachment #649233 - Flags: ui-review?(bwinton)
Attachment #649233 - Flags: review?(bwinton)
(Assignee)

Comment 9

5 years ago
(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.
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.
(Assignee)

Comment 11

5 years ago
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.)
Attachment #649519 - Flags: ui-review?(bwinton)
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.)
Attachment #649519 - Flags: ui-review?(bwinton) → ui-review-
(Assignee)

Comment 13

5 years ago
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.
Attachment #649519 - Attachment is obsolete: true
Attachment #650440 - Flags: ui-review?(bwinton)
(Assignee)

Comment 14

5 years ago
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.
Attachment #650440 - Attachment is obsolete: true
Attachment #650440 - Flags: ui-review?(bwinton)
Attachment #650441 - Flags: ui-review?(bwinton)
(Assignee)

Comment 15

5 years ago
Created attachment 650764 [details] [diff] [review]
Updated patch

Ok, here's the patch.
Attachment #649326 - Attachment is obsolete: true
Attachment #650764 - Flags: review?(bwinton)
Attachment #650764 - Flags: feedback?(richard.marti)
(Assignee)

Comment 16

5 years ago
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.
Attachment #650764 - Flags: approval-comm-aurora?
Comment on attachment 650764 [details] [diff] [review]
Updated patch

This looks good
Attachment #650764 - Flags: feedback?(richard.marti) → feedback+
Comment on attachment 650441 [details]
Updated UI (for real)

Looks good to me.  ui-r+!
Attachment #650441 - Flags: ui-review?(bwinton) → ui-review+
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.
Attachment #650764 - Flags: review?(bwinton) → review+
(Assignee)

Comment 20

5 years ago
(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.
(Assignee)

Comment 21

5 years ago
Checked in: https://hg.mozilla.org/comm-central/rev/4cc9ac6460cf
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 17.0
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 23

5 years ago
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.
Attachment #650764 - Attachment is obsolete: true
Attachment #650764 - Flags: approval-comm-aurora?
Attachment #654123 - Flags: review?(bwinton)
Comment on attachment 654123 [details] [diff] [review]
FIx Mozmill tests

r=me, based on manual inspection of the code change and the test change.
Attachment #654123 - Flags: review?(bwinton) → review+
(Assignee)

Comment 25

5 years ago
Checked in: https://hg.mozilla.org/comm-central/rev/6392d78f3459
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 26

5 years ago
Fixed some bustage with a similar function in the compose tests: https://hg.mozilla.org/comm-central/rev/3be4b5bbe73c
(Assignee)

Updated

5 years ago
Target Milestone: Thunderbird 17.0 → Thunderbird 18.0
(Assignee)

Comment 27

5 years ago
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.
Attachment #654123 - Flags: approval-comm-beta?
Attachment #654123 - Flags: approval-comm-aurora?
Attachment #654123 - Flags: approval-comm-beta?
Attachment #654123 - Flags: approval-comm-beta+
Attachment #654123 - Flags: approval-comm-aurora?
Attachment #654123 - Flags: approval-comm-aurora+
Checked in:

https://hg.mozilla.org/releases/comm-aurora/rev/2211cfff40d0
https://hg.mozilla.org/releases/comm-aurora/rev/5bb7955cea1f
https://hg.mozilla.org/releases/comm-beta/rev/76979fc2eb59
https://hg.mozilla.org/releases/comm-beta/rev/c0afc4d4dc63
status-thunderbird16: --- → fixed
status-thunderbird17: --- → fixed
You need to log in before you can comment on or make changes to this bug.