Closed
Bug 780425
Opened 12 years ago
Closed 12 years ago
Add Aero theming to attachment lists
Categories
(Thunderbird :: Theme, defect)
Tracking
(thunderbird16 fixed, thunderbird17 fixed)
RESOLVED
FIXED
Thunderbird 18.0
People
(Reporter: squib, Assigned: squib)
Details
Attachments
(7 files, 6 obsolete files)
9.73 KB,
image/png
|
Details | |
25.94 KB,
image/png
|
Details | |
6.41 KB,
text/css
|
Details | |
5.99 KB,
image/png
|
Details | |
5.32 KB,
image/png
|
Details | |
62.28 KB,
image/png
|
bwinton
:
ui-review+
|
Details |
9.97 KB,
patch
|
bwinton
:
review+
standard8
:
approval-comm-aurora+
standard8
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
Here is a patch to style like Squib's screenshot.
Attachment #649230 -
Flags: ui-review?(bwinton)
Attachment #649230 -
Flags: review?(bwinton)
Comment 2•12 years ago
|
||
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)
Comment 3•12 years ago
|
||
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•12 years ago
|
||
I already have a patch in progress for this (hence why I was assigned).
Assignee | ||
Comment 5•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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+
Comment 8•12 years ago
|
||
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•12 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.
Comment 10•12 years ago
|
||
(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•12 years ago
|
||
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 12•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 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 17•12 years ago
|
||
Comment on attachment 650764 [details] [diff] [review] Updated patch This looks good
Attachment #650764 -
Flags: feedback?(richard.marti) → feedback+
Comment 18•12 years ago
|
||
Comment on attachment 650441 [details]
Updated UI (for real)
Looks good to me. ui-r+!
Attachment #650441 -
Flags: ui-review?(bwinton) → ui-review+
Comment 19•12 years ago
|
||
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•12 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•12 years ago
|
||
Checked in: https://hg.mozilla.org/comm-central/rev/4cc9ac6460cf
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 17.0
Comment 22•12 years ago
|
||
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•12 years ago
|
||
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 24•12 years ago
|
||
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•12 years ago
|
||
Checked in: https://hg.mozilla.org/comm-central/rev/6392d78f3459
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 26•12 years ago
|
||
Fixed some bustage with a similar function in the compose tests: https://hg.mozilla.org/comm-central/rev/3be4b5bbe73c
Assignee | ||
Updated•12 years ago
|
Target Milestone: Thunderbird 17.0 → Thunderbird 18.0
Assignee | ||
Comment 27•12 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?
Updated•12 years ago
|
Attachment #654123 -
Flags: approval-comm-beta?
Attachment #654123 -
Flags: approval-comm-beta+
Attachment #654123 -
Flags: approval-comm-aurora?
Attachment #654123 -
Flags: approval-comm-aurora+
Comment 28•12 years ago
|
||
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.
Description
•