Closed Bug 455801 Opened 11 years ago Closed 11 years ago

Polish changes to message reader for b1

Categories

(Thunderbird :: Mail Window Front End, defect, P1, minor)

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0a3

People

(Reporter: davida, Assigned: davida)

References

(Blocks 1 open bug, )

Details

(Keywords: late-l10n, Whiteboard: [fixed?])

Attachments

(11 files, 18 obsolete files)

29.04 KB, image/png
Details
73.90 KB, patch
standard8
: review+
Details | Diff | Splinter Review
45.95 KB, image/png
Details
41.47 KB, image/png
Details
95.76 KB, image/png
Details
1.38 KB, patch
philor
: review+
Details | Diff | Splinter Review
19.65 KB, image/png
clarkbw
: ui-review+
Details
8.22 KB, image/png
Details
150.99 KB, image/png
Details
19.93 KB, patch
dmose
: review+
clarkbw
: ui-review+
Details | Diff | Splinter Review
7.84 KB, patch
davida
: review+
davida
: ui-review+
Details | Diff | Splinter Review
As a temporary measure on the way to higher polish and likely per-platform skins, we want to make the message reader view nicer to look at in nightlies/betas.

See https://wiki.mozilla.org/User:Dmose:Message_Reader_Polish for list of things that are to be done.

First patch is to make the buttons look less full-chrome on OS X, and better on Windows/Linux, as well as to fix link and tag colors on the darker background.

A few notes, summarizing discussions on IRC esp. w/ clarkbw:

 - "more actions" and "hide details" buttons have the button look show up on hover.
 - font sizes are deliberate (but only tested on OS X so far)
 - changes to /mailnews are to move the color setting of tags from direct color setting to class setting (letting us do background color setting in this theme).  We'd need changes to the tagColors.css file on all themes, and possibly different ones for Seamonkey.

I'll attach a first patch, and a screenshot.
Attached patch first pass (obsolete) — Splinter Review
Attachment #339147 - Flags: ui-review?(clarkbw)
Attached image screenshot showing tags, on OS X. (obsolete) —
Severity: normal → minor
Flags: blocking-thunderbird3+
Priority: -- → P1
Target Milestone: --- → Thunderbird 3.0b1
Attachment #339147 - Attachment is obsolete: true
Attachment #339147 - Flags: ui-review?(clarkbw)
Comment on attachment 339147 [details] [diff] [review]
first pass

New patch coming w/ images.
Attached patch patch with images added (obsolete) — Splinter Review
Attachment #339149 - Flags: ui-review?(clarkbw)
Comment on attachment 339149 [details] [diff] [review]
patch with images added

we already discussed this on IRC, looks good.
Attachment #339149 - Flags: ui-review?(clarkbw) → ui-review+
Patch updated so that marking as junk hides the mark as junk button, and unmarking brings it back.
Attachment #339149 - Attachment is obsolete: true
Attachment #339196 - Flags: review?(bugzilla)
Blocks: 455835
Duplicate of this bug: 455909
General comments:

- Mark as Junk button status doesn't get reset when the message being view changes, or when the junk status is changed by something else (e.g. click the junk icon in the list view).

- Mark as Junk is disabled for newsgroups... shouldn't we just hide it?

- Where the gradient is darkest for the background bothers me. Its just slightly low of centre between the top buttons and the other actions and doesn't feel quite right.

- Your text looks different to mine, mine isn't so bold. I'll attach a screenshot in a moment.

- Colour of labels is broken in SM (all platforms) and TB Linux/Windows.
Attached image Screenshot of Standard8's Mac. (obsolete) —
Blocks: 436615
Blocks: 449691
No longer blocks: 436615
Comment on attachment 339196 [details] [diff] [review]
Like last patch, but also toggles visibility of "mark as junk" button 

Two more general comments:

-  When I hover over the reply/forward/ etc buttons, they go white(ish). When I hover over the other actions and hide details buttons, they go grey.
- When I view all headers the background doesn't go all the way down to the bottom of the window, hence I can't read all the text (screenshot coming).


>diff --git a/mail/base/content/mailWindowOverlay.js b/mail/base/content/mailWindowOverlay.js
>--- a/mail/base/content/mailWindowOverlay.js
>+++ b/mail/base/content/mailWindowOverlay.js
>@@ -1756,6 +1756,20 @@
> {
>   MsgJunkMailInfo(true);
>   JunkSelectedMessages(!SelectedMessagesAreJunk());
>+}
>+
>+function junkViewedMessage()
>+{
>+  document.getElementById('junkButton').setAttribute('markedasjunk', 'true');
>+  document.getElementById('junkButton').disabled = true;
>+  goDoCommand('button_junk');
>+}
>+
>+function unjunkViewedMessage()
>+{
>+  document.getElementById('junkButton').removeAttribute('markedasjunk');
>+  document.getElementById('junkButton').disabled = false;
>+  JunkSelectedMessages(false);
> }

So this is the problem with the junk buttons. I've had a look at the code and its a bit strange. What I think is best to do, is in the messageHeaderSink object in the onEndHeaders function is to put an extra call in to update the status of the junk button similar to what you got here.

I think you should also use .removeAttribute('disabled') as well.

See also comment below about styles.

>diff --git a/mail/themes/pinstripe/mail/messageHeader.css b/mail/themes/pinstripe/mail/messageHeader.css
>--- a/mail/themes/pinstripe/mail/messageHeader.css
>+++ b/mail/themes/pinstripe/mail/messageHeader.css
>@@ -62,7 +71,46 @@
> }
>+.msgHeaderView-button:hover {
>+  opacity: 0.95;
>+  /*-webkit-border-image: url(smallWhiteButton2.png) 0 5 0 5 stretch stretch;*/
>+  /*-moz-border-image: url(smallWhiteButton2.png) 0 6 0 6 stretch stretch;*/
> }

What are these commented out bits for? If you're going to use them, add a comment to say when please.

>+#junkButton[markedasjunk="true"] {
>+  background: none;
>+  color: transparent;
>+}
>+

Why not do this on #junkButton[disabled="true"] ?

> .messageIdDisplayButton:hover {
>   cursor: pointer;
>   text-decoration: underline;
>   color: blue;
>+  background: url("chrome://messenger/skin/icons/arrow-dn-sepia.png") no-repeat center right;
> }
...
> .emailDisplayButton:hover {
>   cursor: pointer;
>   text-decoration: underline;
>-  color: blue;
>+  color: rgb(255,254,203);
>+  background: url("chrome://messenger/skin/icons/arrow-dn-sepia.png") no-repeat center right;
> }

Is the blue intentional? Is this to distinguish a menu popup from a link? (the email addresses go cream on hover).

>diff --git a/mail/themes/pinstripe/mail/tagColors.css b/mail/themes/pinstripe/mail/tagColors.css
>--- a/mail/themes/pinstripe/mail/tagColors.css
>+++ b/mail/themes/pinstripe/mail/tagColors.css
>+
>+
>+.blc-FFFFFF {
>+    background-color: FFFFFF;
>+}

This should have a # in front of it.

Dan & I don't quite understand why we really need 70 styles here. Obviously some of it is for theme purposes, but surely there is a better way?

>diff --git a/mailnews/base/resources/content/mailWidgets.xml b/mailnews/base/resources/content/mailWidgets.xml
>--- a/mailnews/base/resources/content/mailWidgets.xml
>+++ b/mailnews/base/resources/content/mailWidgets.xml
>@@ -699,8 +699,7 @@
>               // now create a label for the tag name, and set the color
>               var label = document.createElement("label");
>               label.setAttribute('value', tagName);
>-              label.style.color = color;
>-              label.className = "tagvalue";
>+              label.className = "tagvalue blc-" + color.substr(1);
>               headerValueNode.appendChild(label);
>             }
>         ]]>

Depending on what we want to do on the SeaMonkey front (and what SM want to do), then worst case, we could ifdef this for the time being (note, put a * in front of the lines including mailWidgets.xml in jar.mn to cause it to be preprocessed).
Attachment #339196 - Flags: review?(bugzilla) → review-
Attachment #339314 - Attachment description: Screenshot of Standard's Mac. → Screenshot of Standard8's Mac.
Standard8, thanks for the review.

1) I've switched to the lighter theme that bryan mentions in bug 455835.  It solves a lot of the problems with this one, and my guess is that we can fit the lighter one in b1, and worry post b1 about the dark, gradient one, given all the contrast stuff to figure out.  Bryan, your thoughts?

2) I've made the tagColors.css patch smaller (and applied it to the qute theme as well).  For now, I've ifdefed the change in mailWidgets.xml, with the preprocessor change in jar.mn, so that Seamonkey should not be affected. 

3) The issues with the junk button were fixed, as you suggested.

4) all headers problems go away w/ a background instead of a gradient.

I've tried to make the theme work on qute, but I haven't tested that for lack of a working linux/win dev env't.  I may have missed copying an image or two.

I'll attach a screenshot.
Attachment #339196 - Attachment is obsolete: true
Attachment #339378 - Flags: ui-review?(clarkbw)
Attachment #339378 - Flags: review?(bugzilla)
Attachment #339148 - Attachment is obsolete: true
Attachment #339314 - Attachment is obsolete: true
Attachment #339331 - Attachment is obsolete: true
Attachment #339379 - Flags: ui-review?(clarkbw)
I had a makefile bug in the last patch which broke the tags in Thunderbird, and I've changed the grey on the header fields as per IRC discussion w/ clarkbw.
Attachment #339378 - Attachment is obsolete: true
Attachment #339382 - Flags: review?(bugzilla)
Attachment #339378 - Flags: ui-review?(clarkbw)
Attachment #339378 - Flags: review?(bugzilla)
Attachment #339379 - Attachment is obsolete: true
Attachment #339383 - Flags: ui-review?(clarkbw)
Attachment #339379 - Flags: ui-review?(clarkbw)
the earlier screenshot was the wrong one.
Attachment #339383 - Attachment is obsolete: true
Attachment #339386 - Flags: ui-review?(clarkbw)
Attachment #339383 - Flags: ui-review?(clarkbw)
Attached patch Patch 6 (obsolete) — Splinter Review
Adjusted one grey level after talking w/ clarkbw.
Attachment #339382 - Attachment is obsolete: true
Attachment #339401 - Flags: review?(bugzilla)
Attachment #339382 - Flags: review?(bugzilla)
Comment on attachment 339401 [details] [diff] [review]
Patch 6

Certainly looks a lot better on Mac, though the collapsed view is looks broken - large fonts, and no background? (see also notes below).

If I view all headers, then we now display correctly, but the text goes over the status bar, is there an easy way to fix that? If we have to live with it for now, that's ok, but we should add it to the polish list.

>diff --git a/mail/base/content/mailWindowOverlay.js b/mail/base/content/mailWindowOverlay.js
>--- a/mail/base/content/mailWindowOverlay.js
>+++ b/mail/base/content/mailWindowOverlay.js
>@@ -1756,6 +1756,27 @@
> {
>   MsgJunkMailInfo(true);
>   JunkSelectedMessages(!SelectedMessagesAreJunk());
>+}
>+
>+function junkViewedMessage()
>+{
>+  document.getElementById('junkButton').disabled = true;
>+  goDoCommand('button_junk');
>+}
>+
>+function unjunkViewedMessage()
>+{
>+  document.getElementById('junkButton').disabled = false;
>+  JunkSelectedMessages(false);
>+}

As you've added the .disabled to UpdateJunkButton, you can just drop these two functions and undo the changes where you've made the function calls.

>+function UpdateJunkButton()
>+{
>+  var hdr = gDBView.hdrForFirstSelectedMessage;
>+  var junkScore = hdr.getStringProperty("junkscore");
>+  var isJunk = ((junkScore != "") && (junkScore != "0"));
>+  if (isNewsURI(hdr.folder.URI)) isJunk = true;

Please put the isJunk = true; on the next line.

>+  document.getElementById('junkButton').disabled = isJunk;
> }


>diff --git a/mail/themes/pinstripe/mail/messageHeader.css b/mail/themes/pinstripe/mail/messageHeader.css
>--- a/mail/themes/pinstripe/mail/messageHeader.css
>+++ b/mail/themes/pinstripe/mail/messageHeader.css
>@@ -43,16 +43,20 @@
> 
> /* ::::: for the entire area ::::: */
> .main-header-area {
>-  background-color: #6d84a2;
>-  color: #ffffff; 
>-  font-size: 12px;
>-  overflow: hidden;
> }

This could be the reason the collapsed view isn't right, but if really don't need this, then we should just remove it from the file.

> /* ::::: msg header toolbars ::::: */
>-#collapsedHeaderView,
>+#collapsedHeaderView {
>+  padding-top: 1em;
>+  color: black;
>+  font-size: 130%;
>+}

So now I read this, the collapsed view may be showing right, if it is, then it doesn't feel balanced. The drop arrow is right against the left edge, the date is right against the right edge and that just looks too close.

I think you could probably also get away with 0.5em as the top-padding, otherwise you're just taking up a lot of room.

You've also not got a background, is that intentional?

>-treechildren::-moz-tree-row(lc-FFFFFF, selected, focus) {
>+treechildren::-moz-tree-row(lc-FFFFFF, selected, focus), .blc-FFFFFF {
>   background-color: #FFFFFF;
> }

This is certainly a much better way.
>diff --git a/mail/themes/qute/mail/jar.mn b/mail/themes/qute/mail/jar.mn
>--- a/mail/themes/qute/mail/jar.mn
>+++ b/mail/themes/qute/mail/jar.mn
>@@ -166,5 +166,9 @@
>     skin/classic/messenger/accountcentral/manage-imap.png       (accountcentral/manage-imap.png)
>     skin/classic/messenger/accountcentral/manage-newsgroups.png (accountcentral/manage-newsgroups.png)
>     skin/classic/messenger/accountcentral/manage-rss.png        (accountcentral/manage-rss.png)
>+    skin/classic/messenger/icons/arrow-dn-grey.png              (icons/arrow-dn-grey.png)
>+    skin/classic/messenger/icons/arrow-dn-black.png             (icons/arrow-dn-black.png)
>+    skin/classic/messenger/icons/arrow-dn-blue.png              (icons/arrow-dn-blue.png)
>+    skin/classic/messenger/tagbg.png                            (tagbg.png)
>     icon.png
>     preview.png

tagbg.png for qute is missing from this patch. You're also missing all the css changes.
Attachment #339401 - Flags: review?(bugzilla) → review-
Sorry if this is bugspam, but can I throw out an idea for styling/chrome here? I can't find anywhere else where we're allowed to comment.

How about using something like ActiveCaption and CaptionText for the background (or maybe inactive captions)? That's dangerously close to being "native", but it provides a fallback for those with high contrast themes. The screenshot shows that on Windows XP with the media center theme.

I like the buttons being there too, but I think I'd rather a more Office2007 QuickAccess toolbar approach than a "other actions" button hiding out in the middle of nowhere. So I added a screenshot of Office in action, and a hacked Shredder theme for something similar (although I'm to lazy to move the "other actions" button).

I'm not sure why the image for delete should vary so much around the UI either, so I changed that to something more standard, although I kinda like the trash metaphor better.
This may be more a discussion for bug 449691, but I too would prefer using icons (possibly with tooltips when hovering) over the text-based buttons. They are less dominant, and we have the full buttons with labels already in the main toolbar, thus it should be unambiguous which functionality they imply.
Attachment #339401 - Attachment is obsolete: true
Attached patch WIP with untested qute theme (obsolete) — Splinter Review
Standard8: this patch is almost right for pinstripe, and hopefully ok for qute.  There's one issue that's still bugging me having to do with wrapping when the window width shrinks.  This patch wraps, but i'm not happy w/ the way the box sizing & placing happens, and it won't do the right thing in extreme cases.  

It's still better than what we have in tree, so I suggest we check this in (assuming you don't find major issues w/ it), w/ qute fixes if possible.

I'll be in transit for the next 5 hours, and then likely exhausted.
Attachment #339501 - Attachment is obsolete: true
This one should be pretty good.  I'll attach some screenshots for ui-review.
Attachment #339164 - Attachment is obsolete: true
Attachment #339541 - Attachment is obsolete: true
Attachment #339558 - Flags: review?(bugzilla)
Attachment #339560 - Flags: ui-review?(clarkbw)
the additional headers could use some styling work, but not today.
Attachment #339561 - Flags: ui-review?(clarkbw)
perhaps I missed it, but I don't see star/starred mentioned in any of the UX bugs or documents.
(In reply to comment #28)
> perhaps I missed it, but I don't see star/starred mentioned in any of the UX
> bugs or documents.

That is part of bug 450724 which is still WIP.
(In reply to comment #29)
> (In reply to comment #28)
> > perhaps I missed it, but I don't see star/starred mentioned in any of the UX
> > bugs or documents.
> 
> That is part of bug 450724 which is still WIP.

clarification - message marked as star (not addresses)
Comment on attachment 339558 [details] [diff] [review]
[checked in] Patch 8 - includes wrapping fixes, cleaned up paddings

>+    <vbox id="collapsedHeaderView" class="header-part1 headerContainer" flex="1" pack="start">
>+      <hbox flex="1">
>+        <hbox id="collapsedfromBox" align="baseline">
>+          <hbox>
>             <mail-multi-emailHeaderField id="collapsedfromValue"
>+                                         pack="start"
>                                          class="collapsedHeaderDisplayName"
>                                          label="&fromField2.label;" 
>-                                         collapsed="true" flex="1"/>
>+                                         />

nit: although it means an additional line change I think I'd prefer the /> on the end of the previous line.

I'm happy with this with this change, subject to the further changes I have done which will fix the qute theme (at least on Linux).
Attachment #339558 - Flags: review?(bugzilla) → review+
These changes should at least work on Linux.
Attached image Linux Screenshot
Attachment #339593 - Flags: ui-review?(clarkbw)
Note: the Linux changes need some more tweaking for font sizes and padding, but as a start they will do.
Attachment #339593 - Flags: ui-review?(clarkbw) → ui-review+
Comment on attachment 339386 [details]
updated os x tango theme screenshot showing tags (and darker labels)

Clearing old request
Attachment #339386 - Attachment is obsolete: true
Attachment #339386 - Flags: ui-review?(clarkbw)
Keywords: late-l10n
Given Bryan's acceptance of the Linux changes, I have gone ahead and checked these two patches in (I'm assuming Bryan is happy with the Mac changes as well), I will get post-checkin review for the qute theme changes.

Once the windows build has cycled, I'll download an hourly build and check the changes are ok there.

We can then decide on what needs to be done next.
Comment on attachment 339558 [details] [diff] [review]
[checked in] Patch 8 - includes wrapping fixes, cleaned up paddings

Checked in, changeset id: 390:b9680fefe0b4
Attachment #339558 - Attachment description: Patch 8 - includes wrapping fixes, cleaned up paddings → [checked in] Patch 8 - includes wrapping fixes, cleaned up paddings
Comment on attachment 339592 [details] [diff] [review]
[checked in] Fixes for qute theme.

Checked in changeset id: 391:2a274e619bcd, and requesting post-checkin review.
Attachment #339592 - Attachment description: Fixes for qute theme. → [checked in] Fixes for qute theme.
Attachment #339592 - Flags: review?(philringnalda)
Attached image Windows screenshot
I just got this screenshot from the latest windows hourly build. It looks reasonable so I'm not going to do any more changes until David/Bryan have taken a look.
Duplicate of this bug: 456289
Assignee: nobody → david.ascher
Whiteboard: [one patch landed][unix/windows update to go?]
Comment on attachment 339559 [details]
Screenshot OS X showing light tango theme, collapsed view

I don't really like the show details button like this.  It should copy the hide details button look and method.
Depends on: 456596
Here's a patch that addresses comment #41, as well as bad padding in the collapsed view on windows/linux, as well as bad font choices on windows/linux.

Note: I've removed the choice of arial for the expanded view, because it's subtly different from the rest of the UI, which looks very odd.  I've _added_ it to the collapsed view, because with the larger font the same clash doesn't happen, and we end up with antialiased fonts, which are more legible.

Also, I'm changing the "show details" to be a down arrow, with a tooltip saying "show details", as per a discussion w/ clarkbw.

I'll attach screenshots of OS X and Windows, and ask for ui-r.
Attachment #339988 - Flags: ui-review?(clarkbw)
Attachment #339988 - Flags: review?(bugzilla)
Attached patch updated patch (obsolete) — Splinter Review
I had some local changes not qrefreshed. this patch is the right one.
Attachment #339988 - Attachment is obsolete: true
Attachment #339990 - Flags: ui-review?(clarkbw)
Attachment #339990 - Flags: review?(bugzilla)
Attachment #339988 - Flags: ui-review?(clarkbw)
Attachment #339988 - Flags: review?(bugzilla)
using chevron, as per offline conv.
Attachment #339990 - Attachment is obsolete: true
Attachment #340019 - Flags: ui-review?(clarkbw)
Attachment #340019 - Flags: review?(bugzilla)
Attachment #339990 - Flags: ui-review?(clarkbw)
Attachment #339990 - Flags: review?(bugzilla)
Comment on attachment 340019 [details] [diff] [review]
updated patch w/ chevron instead of triangle

thanks for changing this!
Attachment #340019 - Flags: ui-review?(clarkbw) → ui-review+
Comment on attachment 340019 [details] [diff] [review]
updated patch w/ chevron instead of triangle

r=me if you add height: 22px; to #showDetailsButton as discussed on irc.

Though please also check the spacing of the arrow and text on the other details button on Linux - they seem a little close.
Attachment #340019 - Flags: review?(bugzilla) → review+
With my theming of the message pane, I inadvertently dropped the twisty-generation on long field names (esp. those mails with 100's of cc's).

The ideal design is one that shows a few lines, but stops short of showing lots of lines.  Unfortunately, I can't figure out how to make that happen in time, so I'm restoring the "single line vs all" behavior that was there before, as its failure mode is better.

The twisties in 2.0 are completely non-standard, so I'm using much more boring triangles which are like the os x tree folder triangles.  They're kinda like the icons in toolkit, except that there aren't cross-theme triangles handy.

I'll add a screenshot for ui-review.

(it'd be good to get this in today, as otherwise some emails won't be readable in b1)
Attachment #340049 - Flags: ui-review?(clarkbw)
Attachment #340049 - Flags: review?
Attachment #340049 - Flags: review? → review?(dmose)
Comment on attachment 340049 [details] [diff] [review]
[checked in] Patch to deal with long cc lines and other overflow situations

r=dmose
Attachment #340049 - Flags: review?(dmose) → review+
Updated patch with ifdef'ed font sizes to look good on windows _and_ linux using qute theme (this isn't going to work long term).

Also fixed other issues pointed out by Standard8 in Comment #47.

Carrying r+ and ui-r+.  If someone could checkin, that'd be good!
Attachment #340019 - Attachment is obsolete: true
Attachment #340058 - Flags: ui-review+
Attachment #340058 - Flags: review+
Comment on attachment 340058 [details] [diff] [review]
[checked in] Adjusted patch fixing linux issues

Pushed, http://hg.mozilla.org/comm-central/rev/37fa1b54be81
Attachment #340058 - Attachment description: Adjusted patch fixing linux issues [needs checkin] → Adjusted patch fixing linux issues [checked in]
Whiteboard: [one patch landed][unix/windows update to go?] → [fixed?]
Attachment #339559 - Flags: ui-review?(clarkbw)
Comment on attachment 339559 [details]
Screenshot OS X showing light tango theme, collapsed view

Clearing obsolete requests
Attachment #339560 - Flags: ui-review?(clarkbw)
Attachment #339561 - Flags: ui-review?(clarkbw)
Comment on attachment 340049 [details] [diff] [review]
[checked in] Patch to deal with long cc lines and other overflow situations

better not to regress this.  it's not the prettiest fix but we can track that change in bug 456596
Attachment #340049 - Flags: ui-review?(clarkbw) → ui-review+
Comment on attachment 340049 [details] [diff] [review]
[checked in] Patch to deal with long cc lines and other overflow situations

Checked in changeset id 424:5a4f971651a7
Attachment #340049 - Attachment description: Patch to deal with long cc lines and other overflow situations → [checked in] Patch to deal with long cc lines and other overflow situations
Attachment #340058 - Attachment description: Adjusted patch fixing linux issues [checked in] → [checked in] Adjusted patch fixing linux issues
The changes for b1 are done.  Other polish stuff can go on as dependent bugs of bug 456814.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Attachment #339592 - Flags: review?(philringnalda) → review+
You need to log in before you can comment on or make changes to this bug.