Closed Bug 456818 Opened 11 years ago Closed 11 years ago

messagereader: Collapsed view should show "to" information

Categories

(Thunderbird :: Mail Window Front End, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b1

People

(Reporter: davida, Assigned: davida)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: late-l10n)

Attachments

(5 files, 9 obsolete files)

520 bytes, patch
standard8
: review+
davida
: ui-review+
Details | Diff | Splinter Review
547 bytes, patch
standard8
: review+
Details | Diff | Splinter Review
45.49 KB, patch
davida
: review+
standard8
: ui-review+
Details | Diff | Splinter Review
1.61 KB, patch
Bienvenu
: review+
Details | Diff | Splinter Review
331 bytes, patch
standard8
: review+
Details | Diff | Splinter Review
No description provided.
Component: Message Compose Window → Mail Window Front End
QA Contact: message-compose → front-end
Duplicate of this bug: 383539
Making a note that if a message is sent to: you and we show your name as "You" if a message is sent from: you then we should also show your name as "You" in the from field.
Assignee: nobody → dmose
Target Milestone: --- → Thunderbird 3.0b1
Flags: blocking-thunderbird3+
Whiteboard: [ready for review today]
Whiteboard: [ready for review today] → [ready for review tues evening pacific]
Whiteboard: [ready for review tues evening pacific] → [ready for review thurs evening pacific]
Whiteboard: [ready for review thurs evening pacific] → [ready for review fri eve pacific]
Assignee: dmose → david.ascher
Attached patch WIP patch, v1 (obsolete) — Splinter Review
Attachment #349221 - Attachment is patch: true
Attachment #349221 - Attachment mime type: application/octet-stream → text/plain
Attached patch v2 (obsolete) — Splinter Review
I think this is about it, modulo making it work on qute, removing dump statements, and style checks.

One issue is that I currently use "more..." to show that there are more email addresses to show, which is a string addition.  Probably need to change that to just "..." or a symbol for b1, due to string freeze, and followup w/ something else.

Also, note that i hate <descriptions> more than ever.
Attachment #349221 - Attachment is obsolete: true
Duplicate of this bug: 466174
/me loves -> name:"toCcBcc"

thanks for fixing this!
From a UX point of view, I'm not actually sure whether Bcc wants to be included there (it'll only exist on mails that the sender themselves sent or drafts, generally).  Bcc-ed address maybe want some sort of visual affordance to distinguish themselves, eg greyed out or italicized.  Not critical for b1, though.
Whiteboard: [ready for review fri eve pacific] → [ready for review sat eve pacific]
Attached file v3 (obsolete) —
Much better version, with context menu fixes, removal of extra triangles, and comma squeezing, and a much better understanding on my part of the CSS involved in this.

Still to do: port to qute, and I have some ideas about the "more..." location that I want to tweak, but I'm not going to block this patch for b1 on that bit.
Attached patch v4 (obsolete) — Splinter Review
This version works as well on windows.  I'll do a linux build as well, but won't get to test it all tonight.  It'd be good to start the review process, as it's not the smallest of patches.

Two strings: 1) You, as per plan, and 2) "more", to indicate that the email addresses have wrapped.

dan, if you don't want to review it, find someone else?
Attachment #349427 - Attachment is obsolete: true
Attachment #349615 - Attachment is obsolete: true
Attachment #349624 - Flags: review?(dmose)
Whiteboard: [ready for review sat eve pacific] → [needs review dmose]
Attached patch v5 (obsolete) — Splinter Review
version that works on linux as well (the CSS for the class change from junkButton to hdrJunkButton had been missed, making it look weird on newsgroup messages).
Attachment #349624 - Attachment is obsolete: true
Attachment #349651 - Flags: review?(dmose)
Attachment #349624 - Flags: review?(dmose)
oops.  i didn't mean to leave my helper ddumpObj() function and friends in there.

Other changes already done thanks to drive-bys in IRC:
 - inlined getButtonBox() function to avoid YAGlobal.
 - insted -> instead.
Comment on attachment 349651 [details] [diff] [review]
v5

>--- a/mail/locales/en-US/chrome/messenger/messenger.properties
>+++ b/mail/locales/en-US/chrome/messenger/messenger.properties
>@@ -450,3 +450,7 @@
> # junkCommands.js
> junkAnalysisPercentComplete=Junk analysis %S complete
> processingJunkMessages=Processing Junk Messages
>+
>+# second person direct object pronoun; used in the collapsed header view if
>+# the user is in the To or Cc field of a message
>+headerFieldYou=You

This string may not work from an i18n standpoint since it is very English-centric. Other languages have different translations for "you" based on gender, politeness rate, etc. I guess we'll have to find out what localizers say to this.
(In reply to comment #12)

> This string may not work from an i18n standpoint since it is very
> English-centric. Other languages have different translations for "you" based on
> gender, politeness rate, etc. I guess we'll have to find out what localizers
> say to this.

Agreed.  I wonder if using "me" forms might be easier to translate.  FWIW, it seems to be what gmail uses in a l10n email context.
"Me" was I was thinking as well, but I know a lot of thinking as gone on about this for this bug.
Attached patch v6 (obsolete) — Splinter Review
addressing drive-by comments in IRC.  Should be no code changes, but typos, extraneous comments, extraneous debugging code removed.
Attachment #349651 - Attachment is obsolete: true
Attachment #349665 - Flags: review?(dmose)
Attachment #349651 - Flags: review?(dmose)
Comment on attachment 349665 [details] [diff] [review]
v6

dmose correctly points out that i should get ui-review.  bryan, I can show you on all three OSes tomorrow.
Attachment #349665 - Flags: ui-review?(clarkbw)
Please push the strings as the first thing tomorrow morning, add the late-l10n keyword to this bug when you do the pushing and post an explanation of the strings either to mozilla.dev.l10n or dev-l10n@lists.mozilla.org as detailed in https://wiki.mozilla.org/Thunderbird:Localization#Breaking_the_string_freeze

Let me add, that at least the four header-strings seem to sound familiar, but as noted in the discussions of bug 452890, moving/renaming strings during a string freeze is exactly the same as introducing totally new strings and is an absolute no-go. It *must* never happen. 

If such a move/rename action is necessary, it *must* be postponed until after the affected the release and the old/existing strings should be used until that time.

And I see only added strings in the relevant v5 patch of that bug. If those strings are indeed moved/renamed, then please remove the old strings in their old location as well (*after* the release), so that we don't have to go through the stuff (hundreds of obsolete strings) cleaned up in bug 461112 and bug 463869 again.
Would it be possible to use the existing strings for these, instead of adding new ones? Or do they need to be differently localized for b1?

+<!ENTITY hdrReplyButton.label "reply">
+<!ENTITY hdrForwardButton.label "forward">
+<!ENTITY hdrJunkButton.label "mark as junk">
+<!ENTITY hdrTrashButton.tooltiptext "delete">

If we need new strings, does that mean the old ones can be removed?
We want the ellipses to go away, but we can use the existing entities for b1, and do the de-ellipsification and move after b1.

I'll generate a new version of the patch that leaves the 4 old strings alone, and a new patch for post-b1 that does the move and de-ellipsification.
Attached patch v7 (obsolete) — Splinter Review
Version that doesn't use the moved entities (and thus still have the ellipses.

I've filed Bug 466397 for that follow-on string work.

(note: we shouldn't really have multiple strings with the same entity name under /mail, that's a recipe for disaster)
Attachment #349679 - Flags: ui-review?(clarkbw)
Attachment #349679 - Flags: review?(dmose)
Attachment #349665 - Attachment is obsolete: true
Attachment #349665 - Flags: ui-review?(clarkbw)
Attachment #349665 - Flags: review?(dmose)
Comment on attachment 349679 [details] [diff] [review]
v7

saw a demo this morning.

Hopefully the "more" placement will improve to be at the end of the text in the future.

The colors look pretty good.

Overall I like it.
Attachment #349679 - Flags: ui-review?(clarkbw) → ui-review+
Attached patch v8, code (obsolete) — Splinter Review
Better version, which puts the context menu on the thing with the background color, and cancels the mousedown on the star instead (that's why i had moved the context menu "in").

I've also split out the strings, which will be in a separate attachment, for early checkin, as per sipaq's instructions.

Carrying forward ui-review, as it hasn't changed.
Attachment #349679 - Attachment is obsolete: true
Attachment #349792 - Flags: ui-review+
Attachment #349679 - Flags: review?(dmose)
carrying forward UI review for these strings, and assigning to standard8 for early checkin.
Attachment #349794 - Flags: ui-review+
Attachment #349794 - Flags: review?(bugzilla)
Attachment #349792 - Flags: review?(mkmelin+mozilla)
Comment on attachment 349794 [details] [diff] [review]
[checked in] v8, strings

+<!-- Message Header Button Box -->

nit: I think this should be:

+<!-- Message Header Button Box (to show hidden email addresses) -->

I'll change this as I land it.
Attachment #349794 - Flags: review?(bugzilla) → review+
Comment on attachment 349794 [details] [diff] [review]
[checked in] v8, strings

String patch checked in: http://hg.mozilla.org/comm-central/rev/c6e9f82a0f43
Attachment #349794 - Attachment description: v8, strings → [checked in] v8, strings
Keywords: late-l10n
Attachment #349805 - Flags: review?(bugzilla)
Attachment #349805 - Flags: review?(bugzilla) → review+
Comment on attachment 349805 [details] [diff] [review]
[checked in] missed string

http://hg.mozilla.org/comm-central/rev/a23ff27acc06
Attachment #349805 - Attachment description: missed string → [checked in] missed string
Comment on attachment 349792 [details] [diff] [review]
v8, code

All in all it's a good improvement. Somehow I'd very much like to see "From" and "To" in the collapsed view too. It's just odd listing all names out of context.

I too got the 
Error: emailAddressNode.getPart is not a function
Source File: chrome://messenger/content/msgHdrViewOverlay.js
Line: 1157

... not sure where.

Some more comments/nits:

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
@@ -1678,7 +1678,15 @@
   let hideJunk = (junkScore != "") && (junkScore != "0");
   if (isNewsURI(hdr.folder.URI))
     hideJunk = true;
>-  document.getElementById('junkButton').disabled = hideJunk;
>+  // which DOM node is the current junk button in the
>+  // message reader depends on whether it's the collapsed or
>+  // expanded header
>+  let buttonBox;
>+  if (gCollapsedHeaderViewMode)
>+    buttonBox = document.getElementById("collapsedButtonBox");
>+  else
>+    buttonBox = document.getElementById("expandedButtonBox");

   let buttonBox = document.getElementById(gCollapsedHeaderViewMode ?
                     "collapsedButtonBox" : "expandedButtonBox");
 
     if (gCollapsedHeaderViewMode && !gBuiltCollapsedView)
     {
>-      if (headerName in gCollapsedHeaderView)
>-      headerEntry = gCollapsedHeaderView[headerName];
>+      if (headerName =="cc" || headerName == "to" || headerName == "bcc") { 
>+        headerEntry = gCollapsedHeaderView["toCcBcc"];
>+      } else if (headerName in gCollapsedHeaderView) {
>+        headerEntry = gCollapsedHeaderView[headerName];
>+      }

No need for the braces here (and trailing space).

>+function OutputDate(headerEntry, headerValue)
>+{
>+  try {
>+    headerEntry.textNode.value = headerValue;
>+    // the date field is a textbox, and we want it to be as small as possible to
>+    // not take space from the subject textbox.  This is a bit bigger than
>+    // necessary, but guaranteed to fit.  We should move to not using textboxes
>+    // because of these sizing problems, and instead figure out selectable
>+    // labels.
>+    headerEntry.textNode.setAttribute("size", Math.round(headerValue.length * 1.2)); 

Trailing space (also please capitalize the sentence and remove the double spaces.)

>+  } catch (e) {
>+    dump("headerEntry = " + headerEntry + " and headerValue = " + headerValue + '\n')
>+  }

When do we need this try/catch? If we really need it, also dump out the e so we know what happened.


>-    document.getElementById("editContactItem").label);
>-
>+  

Non-empty emtpy line.

>-  var displayName = cardDetails.card.displayName;
>+  var displayName;
>+  var allIdentities = accountManager.allIdentities;
>+  identity = getBestIdentity(allIdentities);
>+  if (emailAddress == identity.email) {
>+    displayName = gMessengerBundle.getString("headerFieldYou");
>+  } else {

Don't need the tmp allIdentities here, and identity should be declared.
(While you're at it, brace style is else on a new line)

>+    if (!cardDetails.card) {
>+      return;
>+    }

No need for braces for this.

>+    displayName = cardDetails.card.displayName;
>+  }
 
   if (!displayName)
     return;
@@ -1154,11 +1184,17 @@
   }
 }
 
>+function hideEmailAddressPopup(emailAddressNode)
>+{
>+  // highlight the emailBox
>+  emailAddressNode.removeAttribute('selected');
>+}
>+
 function setupEmailAddressPopup(emailAddressNode)
 {
   var emailAddressPlaceHolder = document.getElementById('emailAddressPlaceHolder');
>-  var emailAddress = emailAddressNode.getAttribute('emailAddress');
>-
>+  var emailAddress = emailAddressNode.getPart("emaillabel").value;
>+  emailAddressNode.setAttribute('selected', "true");

Please choose one quote style per line;)

>+        </hbox>
>+        <hbox flex="0" id="collapseddateBox">

Please move the id first.

>+          <textbox id="collapseddateValue" class="plain collapsedHeaderValue"
>+                   flex="0" readonly="true"/>
         </hbox>
       </hbox>
>-      <hbox align="baseline" flex="1">
>-        <hbox id="collapsedsubjectBox" flex="1">
>-          <hbox flex="1" align="center">
>-            <textbox flex="1" id="collapsedsubjectValue"
>-                     class="collapsedHeaderValue plain" readonly="true"/>
>-          </hbox>
>-          <vbox>
>-            <button align="center"
>-                    id="showDetailsButton"
>-                    tooltiptext="&showDetailsButton.label;"
>-                    onclick="ToggleHeaderView();"
>-                    class="msgHeaderView-button"/>
>-          </vbox>
>+
>+      <hbox flex="1" Xstyle="border: solid 1px red;" align="center">
>+        <hbox id="collapsedtoCcBccBox"
>+              align="center" flex="1" Xstyle="border: solid 1px blue;">

Remove the debug.

>+          <mail-multi-emailHeaderField id="collapsedtoCcBccValue"
>+                                       flex="1"
>+                                       align="baseline"
>+                                       class="collapsedHeaderDisplayName"/>
         </hbox>
>+        <button Xstyle="border: solid 1px green;"

... here too.

>+          <hbox flex="1" align="center">
             <mail-multi-emailHeaderField id="expandedfromBox"
                                          label="&fromField2.label;"
>+                                         align="baseline"
                                          collapsed="false"
                                          flex="1"/>
           </hbox>

Don't need the align="center" when it's only one element.

diff --git a/mail/locales/en-US/chrome/messenger/messenger.properties b/mail/locales/en-US/chrome/messenger/messenger.properties
>--- a/mail/locales/en-US/chrome/messenger/messenger.properties
>+++ b/mail/locales/en-US/chrome/messenger/messenger.properties
@@ -450,3 +450,7 @@
 # junkCommands.js
 junkAnalysisPercentComplete=Junk analysis %S complete
 processingJunkMessages=Processing Junk Messages
>+
>+# second person direct object pronoun; used in the collapsed header view if
>+# the user is in the To or Cc field of a message
>+headerFieldYou=You

This part should have gone in the string patch, no?

   background-color: transparent;
   padding: 0px;
   margin-top: 0;
>+  margin-right: 0;
>+}
>+
>+

Extra emtpy line

 #collapsedHeaderView {
>-  padding: .5em;
   min-width: 1px;
>+  margin-left: 0.5em;
   color: black;
>-  font-size: 125%;
>+  font-size: 110%;
>+  padding-bottom: .5em;
>+  padding-top: .5em;
 }

Please choose either decimal format for consistency.

 #expandedHeaderView {
   color: #2e3436;
>-  font-size: 100%;
>+  padding-top: .5em;
   overflow-y: auto;
   overflow-x: hidden;
   max-height: 14em;
@@ -75,7 +77,6 @@
 .headerContainer
 {
   min-width: 1px;
>-  background-color: transparent;
 }
 
 #otherActionsButton {
@@ -124,18 +125,24 @@
   border: 2px solid #C0C3C6;
 }
 
>-
 #collapsedsubjectValue {
>+  margin-left: 3px !important;
   -moz-box-align: stretch;
   font-weight: bold;
 }
 
>+#collapseddateValue {
>+  -moz-box-align: stretch;
>+  text-align: right;
>+  padding-right: .5em !important;
>+}
 
 #showDetailsButton {
   -moz-appearance: none !important;
   border: 2px solid transparent;
   background-color: transparent;
   width: 22px;
>+  padding-bottom: 0px !important;
   height: 22px;
   background-image: url("chrome://messenger/skin/icons/chevron.png");
   background-repeat: no-repeat;
@@ -147,11 +154,6 @@
   border: 2px solid #C0C3C6;
 }
 
>-#trashButton {
>-  -moz-box-orient: vertical;
>- list-style-image: url("chrome://messenger/skin/icons/folder.png");
>-  -moz-image-region: rect(0 144px 16px 128px);
>-}
 
 /* ::::: expanded header pane ::::: */
 
@@ -254,7 +256,7 @@
 }
 
 .msgHeaderView-button {
>-  min-width: 1px;
>+  min-width: 1px !important;
   -moz-appearance: none;
   font-size: 95%;
   color: black;
@@ -272,6 +274,14 @@
   -moz-border-left-colors: none;
 }
 
>+/* had to set a silly attribute on the XUL element to make it so that this
>+  rule overrides the rule above.  Better fix wanted! */
>+.hdrTrashButton[trash="true"] { 
>+  -moz-box-orient: vertical !important;
>+  list-style-image: url("chrome://messenger/skin/icons/folder.png") !important;
>+  -moz-image-region: rect(1px 142px 15px 126px);
>+}

Does .hdrTrashButton.msgHeaderView-button work?


 
 .headerName {
>-  margin: 0 .5em 5px 0;
>+  color: #888a85; /* lower contrast */
   text-align: right;
>-  color: #888a85; /* lower contrast */
>+  background-color: transparent;
>+  padding: 0px;
>+  margin-top: 0;
>+  margin-right: 0;
>+  /*margin: 0 .5em 5px 0;*/
>+}

Remove thh commented out margin.

 <bindings   id="mailBindings"
             xmlns="http://www.mozilla.org/xbl"
@@ -155,17 +162,18 @@
 
   <!-- Message Pane Widgets -->
 
>-  <!-- mail-toggle-headerfield: non email addrss headers which have a toggle associated with them (i.e. the subject).
>-       use label to set the header name.
>-       use headerValue to set the header value. -->
>+  <!-- mail-toggle-headerfield: non email addrs headers which have a toggle
>+       associated with them (i.e. the subject).
>+         use label to set the header name.
>+         use headerValue to set the header value. -->

Please capitalize sentenses if it otherwise looks like a sentence.
>+        <xul:hbox class="headerNameBox" align="baseline" pack="end">
>+          <xul:label class="headerName" xbl:inherits="value=label"/>
>+        </xul:hbox>
>+  

Spaces on this emtpy line.

>+        <xul:hbox class="headerValueBox" anonid="longEmailAddresses" flex="1"
>+                  singleline="true"
>+                  align="baseline"
>+                  onoverflow="this.parentNode.overflow(event)"
>+                  onunderflow="this.parentNode.underflow(event)">
>+          <xul:description class="headerValue" containsEmail="true" anonid="emailAddresses" flex="1"
>+                  orient="vertical" pack="start" />

Wrap earlier to keep line lenght under control


                 var textNode = document.createElement("text");
>-                textNode.setAttribute("value", ", ");
>+                textNode.setAttribute("value", ",");

Is this needed? I wonder if there aren't cases it will look odd, and then perhaps
screen readers aren't too fond of it?


   </binding>
>+  
>+  <binding id="header-view-button-box">

Spaces on emtpy line

>+    <content>
>+      <!-- It might be nice to allow a UI for customization of these buttons -->
>+      <xul:hbox align="start">
>+ 

Trailing space.

>+        <!-- XXXdmose need to move these commands to a controller, either
>+             on the header view, the message pane, or the default
>+             controller -->
>+
>+        <!-- XXXdmose need to audit our shortcut/a11y setup and make sure
>+             these buttons are doing the right thing -->
>+
>+       <xul:button anonid="hdrReplyButton" label="&replyButton.label;"
>+                   oncommand="MsgReplyMessage(event);" observes="button_reply"
>+                   class="msgHeaderView-button hdrReplyButton"/>
>+       <xul:button anonid="hdrForwardButton" label="&forwardButton.label;"
>+                   oncommand="MsgForwardMessage(event);"
>+                   observes="button_forward" class="msgHeaderView-button hdrForwardButton"/>
>+       <xul:button anonid="hdrJunkButton" label="&junkButton.label;"
>+                   observes="button_junk" class="msgHeaderView-button hdrJunkButton"
>+                   oncommand="goDoCommand('button_junk')"/>
>+       <xul:button anonid="hdrTrashButton" tooltiptext="&trashButton.tooltiptext;"
>+                   observes="button_delete" trash="true"
>+                   class="msgHeaderView-button hdrTrashButton" 
>+                   oncommand="goDoCommand(event.shiftKey ? 'cmd_shiftDelete' : 'cmd_delete')"/>
>+      </xul:hbox>
>+    </content>
>+
>+    <implementation>
>+      <method name="getButton">
>+        <parameter name="id"/>
>+        <body>
>+          <![CDATA[
>+          return document.getAnonymousElementByAttribute(this, 'anonid', id);
>+          ]]>
>+        </body>
>+      </method>
>+    </implementation>
>+  </binding>
>+  
>+  

Trailing spaces, and I don't think we need two empty lines.

For the theme changes I'm a bit concerned about all the !important usage, but it's hard to say without testing what is needed when and when not.

There was also a placeholder label="foo" in the patch, is that needed? 

Oh, and could you put something like
diff =-p -U 8 in your ~/.hgrc? More context makes it easier to see what you're changing.
Attached patch v9 (obsolete) — Splinter Review
Ok, this version addresses all of the comments in Comment #28, with a few exceptions noted below.  I also deal with Standard8's comment in IRC about the bubbles on the second line showing up on hover at times.

The getPart exception you both saw was due to the fact that the popup/context menus set the popupNode to the node clicked, even if the menu is set on the parent.  There's a workaround in the patch, which is OK but not great.  I can't find a better fix at this stage.

Things I didn't address (and why):

>+/* had to set a silly attribute on the XUL element to make it so that this
>+  rule overrides the rule above.  Better fix wanted! */
>+.hdrTrashButton[trash="true"] { 
>+  -moz-box-orient: vertical !important;
>+  list-style-image: url("chrome://messenger/skin/icons/folder.png") !important;
>+  -moz-image-region: rect(1px 142px 15px 126px);
>+}

> Does .hdrTrashButton.msgHeaderView-button work?

No, it didn't seem to work for me.  CSS precedence rules baffle me at times.

                 var textNode = document.createElement("text");
>-                textNode.setAttribute("value", ", ");
>+                textNode.setAttribute("value", ",");

> Is this needed? I wonder if there aren't cases it will look odd, and then
> perhaps
> screen readers aren't too fond of it?

I'm going as per bryan's design.  It seems to look ok in testing so far.  If screen readers have an issue, I'm sure we'll find out.

carrying forward ui-review
Attachment #349792 - Attachment is obsolete: true
Attachment #349855 - Flags: ui-review+
Attachment #349855 - Flags: review?(mkmelin+mozilla)
Attachment #349792 - Flags: review?(mkmelin+mozilla)
Comment on attachment 349855 [details] [diff] [review]
v9

+function OutputDate(headerEntry, headerValue)
+{
+headerEntry.textNode.value = headerValue;

Nit: indent

-    return;
+  } else {
+    documentNode.setAttribute("hascard", "true");

Nit: else on new row.

 function setupEmailAddressPopup(emailAddressNode)
 {
+    dump("emailAddressNode.tagName = " + emailAddressNode.tagName + '\n');

Remove the dump.

+        <button Xstyle="border: solid 1px green;"
+                id="showDetailsButton"

Remove debug.

+/* YYY ok to use this selector? */
+#collapsedtoCcBccValue > .headerNameBox {

Should be ok. (That's only one element up the path that has to be checked.)

+          <xul:description class="headerValue" containsEmail="true" anonid="emailAddresses" flex="1"
+                  orient="vertical" pack="start" />

Wrap and align.

Haven't had a chance to play with it yet, but the functional changes from the last patch should be minimal I assume. So r=mkmelin with the nits fixed.
Attachment #349855 - Flags: review?(mkmelin+mozilla) → review+
Whiteboard: [needs review dmose] → [needs nitpicked patch]
Nits addressed.  Carrying forward reviews.

checkin at will.
Attachment #349855 - Attachment is obsolete: true
Attachment #350053 - Flags: superreview+
Attachment #350053 - Flags: review+
Keywords: checkin-needed
Whiteboard: [needs nitpicked patch] → [needs checkin]
Attachment #350053 - Attachment is patch: true
Attachment #350053 - Attachment mime type: application/octet-stream → text/plain
Attachment #350053 - Flags: superreview+ → ui-review+
Attachment #350053 - Attachment description: v10! → [checked in] v10!
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs checkin]
Testing post landing identified that in the expanded header view on windows, the "more" text link showed up all the time.

In addition, that text is too big on Windows at least, so got rid of the font size setting, and it looks better.

In addition, mkmelin was wrong in comment #28 about: "Don't need the align="center" when it's only one element", given that the parent is flex.  That looked goofy on the mac in expanded view.  Restored it.
Attachment #350080 - Flags: review?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #350080 - Attachment description: tweaks. → checked in - tweaks.
Attachment #350080 - Flags: review? → review+
re-resolving fixed
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Any chance of a warning next time you incompatibly change shared code?
Depends on: 466795
Neil: sorry, mea culpa.  Next time, will definitely try to give a warning.
Is it possible that the changes from this bug caused this?
http://forums.mozillazine.org/viewtopic.php?p=5085785#p5085785
re #37: yes.  Standard8 has reported the same behavior on OS X.  I can't as yet repro it.  Will dig some more.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
this seems to fix it, although i don't understand the underlying css math that would cause overflow in the expanded view and not the collapsed view.  chromebug, where art thou?
Attachment #350198 - Flags: review?(bugzilla)
Blocks: 466880
Comment on attachment 350198 [details] [diff] [review]
[checked in] fix for spurious "more" showing up unnecessarily on OS X

This seems to fix it for me.

I've checked it in as well:
http://hg.mozilla.org/comm-central/rev/86e78aa854b6
Attachment #350198 - Attachment description: fix for spurious "more" showing up unnecessarily on OS X → [checked in] fix for spurious "more" showing up unnecessarily on OS X
Attachment #350198 - Flags: review?(bugzilla) → review+
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Can confirm it, this fixed it also for me. Thanks :)
The "more" twinkles shortly if I switch between detailed header and slim header. But this is fortunately not very noticable.
I hope this is the right place for the following (it looks like it is, and I was directed here from http://forums.mozillazine.org/viewtopic.php?f=29&t=987335&start=45&st=0&sk=t&sd=a&sid=5cebe5a5deed1a6844b1126adcba505a).

I am running Thunderbird 3.0b1 under MacOS 10.5.6 and I am seeing the spurious "more" discussed above.  I have tried starting in "safe mode" and still see the "more" indications, so I assume it is not a result of any add-ons etc.
Blocks: 481966
No longer blocks: 481966
Depends on: me-header
You need to log in before you can comment on or make changes to this bug.