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.

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.

/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.

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.

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?

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).

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.

addressing drive-by comments in IRC. Should be no code changes, but typos, extraneous comments, extraneous debugging code removed.

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.

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.

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)

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.

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.

carrying forward UI review for these strings, and assigning to standard8 for early checkin.

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.

Comment on attachment 349794 [details] [diff] [review] [checked in] v8, strings String patch checked in: http://hg.mozilla.org/comm-central/rev/c6e9f82a0f43

Comment on attachment 349805 [details] [diff] [review] [checked in] missed string http://hg.mozilla.org/comm-central/rev/a23ff27acc06

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.

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

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.

Nits addressed. Carrying forward reviews. checkin at will.

Comment on attachment 350053 [details] [diff] [review] [checked in] v10! Checked in: http://hg.mozilla.org/comm-central/rev/ea449e297e95

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.

re-resolving fixed

Any chance of a warning next time you incompatibly change shared code?

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.

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?

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

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.