Closed Bug 898860 Opened 6 years ago Closed 5 years ago

expandeddateLabel misspelled "date" value

Categories

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

17 Branch
defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 31.0

People

(Reporter: milongal, Assigned: aceman)

References

Details

Attachments

(5 files, 4 obsolete files)

Attached image date-header-misspelled.jpg (obsolete) —
User Agent: Mozilla/5.0 (Windows NT 5.1; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130618035212

Steps to reproduce:

Inside message window, tab and pane.
Inspection with DOMi:
id: expandeddateLabel, class: headerName, value: date
Click "edit" to fix missing capital "D". 


Actual results:

It fixed properly displays "Date".

All other headers are correctly spelled with capitals "To", "From", "Subject".


Expected results:

Could someone please correct this in the interface?
Here's the full view.
Attachment #782255 - Attachment is obsolete: true
I see a capitalized "Date" on TB 17.0.7 and it is like this since landing of bug 478468 (9. Sep 2011) for me.

Which version of TB are you using? Please also check the label again with all extensions disabled.
Flags: needinfo?(milongal)
As seen in 17.0.6, with all extensions disabled.
Thanks for the quick reply, Richard.
I was on TB 17.0.6. Even with all extensions disabled, I still see "date".
I also just updated to 17.0.7, and it didn't fix the problem.

Sorry. The attachment got saved and posted, before this message.
Flags: needinfo?(milongal)
Strange, is it the en-US locale you are using? Here with built-in en-US it is correctly "Date".

Maybe someone other has a solution.
I use the FR and ENG versions of TB 2 (old habits die hard) and English TB 17 (to keep up to date). Both are on several different computers, all running French XP SP3.

In the French TB2, all the headers are correctly capitalized, black and bold, with colons after them. Tiny details, yes, but they do improve recognition and readability. I haven't tried TB 17 in FR yet. Need to sort out the quirks in ENG first.

In both English TB 2 & TB 17 no capitalized "date". (Strangely, I never noticed it until now.)
However, English TB 2 has nice black, bold headers with colons. 

Let's hope someone can sort it out.
I can confirm the bug on TB25 trunk.
Assuming you got the Date header to display using the pref mailnews.headers.extraExpandedHeaders and setting it to "date".
When the pref is not set, using DOM inspector, the label has the correct value of "Date" and is collapsed=true.

When setting the pref to "date", the row appears (after Tb restart), with a label of "date". But when you analyse the document tree, you notice this element got added to the end of the list of row elements in the msg head. And there are now two rows having id="expandeddateRow" . So I think somehow the wanted extra header does not use the proper already created xul elements but creates a new one.

Try to follow the code from here:
http://mxr.mozilla.org/comm-central/source/mail/base/content/msgHdrViewOverlay.js#166
Status: UNCONFIRMED → NEW
Ever confirmed: true
So if you put "Date" into the pref, the label is correct. It takes the label value verbatim from the pref. But when you put it in uppercase, then the id of the element is also using the uppercase value so you get expandedDateRow, which is not in line with the other elements in the list (which are like expandeddateRow).

So the code at 
http://mxr.mozilla.org/comm-central/source/mail/base/content/msgHdrViewOverlay.js#183 should probably be 
new createNewHeaderView(extraHeader.ToLowerCase(), extraHeader);

But that still does not answer why it is not reusing the already existing element with expandeddateRow but instead creates a new one. Maybe Date is not supposed to be included in mailnews.headers.extraExpandedHeaders but there are standard options for it?
@:aceman You aced it!! ;-) Thanks.

Yes, I did use about:config to display the headers, and typed in "date" instead of "Date".

On a deeper level, I think these headers are useful and essential elements, especially in Pane view. If not set by default, then at least keep the "+" to roll them down. 

Also, yes, there are in fact 2 "date" sections. See new attachment. The one at the right just looks odd and lonely by itself, especially on a wide screen. One clean visual block at the left would seem sufficient.
Problem solved, but why two date fields?
if you don't want 2 date fields, why do you explicitly enable the second one via the mailnews.headers.extraExpandedHeaders ?

Should we make TB so clever, that when you include the Date in mailnews.headers.extraExpandedHeaders it should hide the date on the right?

But there are serious inconsistencies here. If the pref mailnews.headers.extraExpandedHeaders distinguishes lowercase and uppercase forms of the field labels, why did it still display the value of the Date field if you input "date" into it? I think it should have been blank so that you notice the mistake. If it accepts both "date" and "Date" and shows the same content, then maybe it should automatically uppercase the label.

I'd like some comment from squib and maybe I could take this bug.

What I meant by finding 2 elements with id="expandeddateRow" in DOM inspector is that there are 2 <row> elements generated with such an id, not that there is another date display on the right (that one surely has a unique id).
Flags: needinfo?(squibblyflabbetydoo)
(In reply to :aceman from comment #11)
> Should we make TB so clever, that when you include the Date in
> mailnews.headers.extraExpandedHeaders it should hide the date on the right?

Certainly not. If someone really hates the date on the right and is modifying prefs to show it elsewhere, they can also modify userChrome.css to hide the date on the right.

> But there are serious inconsistencies here. If the pref
> mailnews.headers.extraExpandedHeaders distinguishes lowercase and uppercase
> forms of the field labels, why did it still display the value of the Date
> field if you input "date" into it? I think it should have been blank so that
> you notice the mistake. If it accepts both "date" and "Date" and shows the
> same content, then maybe it should automatically uppercase the label.

MIME headers are case-insensitive, which is why this is happening. Neither "date" nor "Date" are more correct; just use the form you like ("dAtE" is also acceptable, of course).

> What I meant by finding 2 elements with id="expandeddateRow" in DOM
> inspector is that there are 2 <row> elements generated with such an id, not
> that there is another date display on the right (that one surely has a
> unique id).

We could probably remove the expandeddateRow element, since it doesn't appear to be used anywhere else.
Flags: needinfo?(squibblyflabbetydoo)
(In reply to Jim Porter (:squib) from comment #12)
> (In reply to :aceman from comment #11)
> MIME headers are case-insensitive, which is why this is happening. Neither
> "date" nor "Date" are more correct; just use the form you like ("dAtE" is
> also acceptable, of course).

I'll ask the UI guys if we want the label to be cleaned up somehow so it fits the rest of the UI. E.g. making the first letter uppercase and the rest lowercase, regardless how the user set it in the pref.

> > What I meant by finding 2 elements with id="expandeddateRow" in DOM
> > inspector is that there are 2 <row> elements generated with such an id, not
> > that there is another date display on the right (that one surely has a
> > unique id).
> 
> We could probably remove the expandeddateRow element, since it doesn't
> appear to be used anywhere else.

There are many such collapsed rows for other fields and I don't know when (if) they are used. I can look at this. Thanks for the comment.
Assignee: nobody → acelists
Flags: needinfo?(bwinton)
Sorry for the delay in replying.

Please see new attachment. My suggestion is:
1. group the relevant message header info together on the left like it was in the past
2. Remove lonely date on the right
3. Put the "+" sign back to give user back the choice to display or not.

Personally, I didn't look for a way to remove the date at the right but will check it out. But remember, I am a geek. Most users and none of my clients are. The simpler the better.


Thanks to all for input and help.
Comparison of open/closed pane view TB2 vs TB17.
(In reply to :aceman from comment #13)
> (In reply to Jim Porter (:squib) from comment #12)
> > (In reply to :aceman from comment #11)
> > MIME headers are case-insensitive, which is why this is happening. Neither
> > "date" nor "Date" are more correct; just use the form you like ("dAtE" is
> > also acceptable, of course).
> 
> I'll ask the UI guys if we want the label to be cleaned up somehow so it
> fits the rest of the UI. E.g. making the first letter uppercase and the rest
> lowercase, regardless how the user set it in the pref.

Can we get the case of the header from the current message?

If not, I'm fine with making it Title Case.  Just be sure to handle stuff like "List-Id" and "Authentication-Results" correctly.

Later,
Blake.
Flags: needinfo?(bwinton)
(In reply to Blake Winton (:bwinton) from comment #16)
> Can we get the case of the header from the current message?

No, we add the headers from mailnews.headers.extraExpandedHeaders when the message pane is first loaded, before we actually put a message in there. Theoretically, we could fill it with a dummy value and then fix it up when we load a message, but I'd rather we keep this simple.
Title-Case it is, then!
Attached patch patch (obsolete) — Splinter Review
So this reuses existing elements even for the user supplied headers and titlecases the header name. In the preexisting elements we hope the name is already in the proper format (the *Field*.label entities).
Attachment #8402402 - Flags: ui-review?(bwinton)
Attachment #8402402 - Flags: review?(squibblyflabbetydoo)
Severity: normal → minor
So…  I think I might go back on my decision above…
It seems to me that showing what people type in is a better idea than trying to be clever, and title-casing everything.

Thoughts?
(In reply to Blake Winton (:bwinton) from comment #20)
> So…  I think I might go back on my decision above…
> It seems to me that showing what people type in is a better idea than trying
> to be clever, and title-casing everything.

I think that's the current behavior without the patch. I'm totally fine with it, and it would make it possible to have non-title-cased headers show up correctly, e.g. MIME-version or DKIM-Signature (provided you type them in correctly in the prefs editor).
That would basically make this bug invalid. And the patch would be reduced to just reusing the existing elements to not produce elements with duplicate IDs.
Can we title case it only when the value in the pref is all lowercase? Otherwise do not touch it.
Flags: needinfo?(squibblyflabbetydoo)
Flags: needinfo?(bwinton)
no, because that would prevent people from reverting to the one true label style, all-lowercase.  ;)

(But seriously, no, that would just be confusing.)
Flags: needinfo?(squibblyflabbetydoo)
Flags: needinfo?(bwinton)
Attached patch patch v2 (obsolete) — Splinter Review
OK, this removes the title case. In the reporter's case, 'date' in pref will still come up as Date because the existing element with dateField4.label will be used. But other headers not found in the xul file will be created from the pref headers unchanged.
Attachment #8402402 - Attachment is obsolete: true
Attachment #8402402 - Flags: ui-review?(bwinton)
Attachment #8402402 - Flags: review?(squibblyflabbetydoo)
Attachment #8407023 - Flags: review?(squibblyflabbetydoo)
Attachment #8407023 - Flags: review?(bwinton)
Comment on attachment 8407023 [details] [diff] [review]
patch v2

Review of attachment 8407023 [details] [diff] [review]:
-----------------------------------------------------------------

r-, but overall I like what this does. I just think we should use this chance to fix the interface to this code a bit as well.

::: mail/base/content/msgHdrViewOverlay.js
@@ +996,5 @@
>   * @param {String} headerName  name of the header we're adding, all lower-case;
>   *                             used to construct element ids
>   * @param {String} label       name of the header as displayed in the UI
>   */
>  function createNewHeaderView(headerName, label)

I think we should rename this to "HeaderView". Saying "new createNewHeaderView(...)" is kind of weird looking.

@@ +1003,5 @@
> +  let idName = "expanded" + headerName + "Box";
> +  let topViewNode = document.getElementById("expandedHeader2Rows");
> +  let newHeaderNode;
> +  // If a row for this header already exists, do not create another one.
> +  let newRowNode = topViewNode.querySelector("#" + rowId);

Last I checked, getElementById was *substantially* faster than querySelector, and since we've fixed the issue of non-unique element IDs, we shouldn't need to explicitly check a subtree of the document, right?

@@ +1004,5 @@
> +  let topViewNode = document.getElementById("expandedHeader2Rows");
> +  let newHeaderNode;
> +  // If a row for this header already exists, do not create another one.
> +  let newRowNode = topViewNode.querySelector("#" + rowId);
> +  if (!newRowNode) {

I think it would make more sense for this function to always create a new HeaderView, but to give it a reset() method that resets its internal state to how it was when it was first created.

Maybe add a utility function to "create a new HeaderView or reset the existing one". I know that's adding more code to do basically the same thing, but I think it separates things more cleanly.

@@ +1005,5 @@
> +  let newHeaderNode;
> +  // If a row for this header already exists, do not create another one.
> +  let newRowNode = topViewNode.querySelector("#" + rowId);
> +  if (!newRowNode) {
> +    // create new collapsed row

While you're here, could you turn these comments into proper sentences? (Capitalize the first letter and add punctuation.)
Attachment #8407023 - Flags: review?(squibblyflabbetydoo) → review-
(In reply to Jim Porter (:squib) from comment #26)
> @@ +1004,5 @@
> > +  let topViewNode = document.getElementById("expandedHeader2Rows");
> > +  let newHeaderNode;
> > +  // If a row for this header already exists, do not create another one.
> > +  let newRowNode = topViewNode.querySelector("#" + rowId);
> > +  if (!newRowNode) {
> 
> I think it would make more sense for this function to always create a new
> HeaderView, but to give it a reset() method that resets its internal state
> to how it was when it was first created.
> 
> Maybe add a utility function to "create a new HeaderView or reset the
> existing one". I know that's adding more code to do basically the same
> thing, but I think it separates things more cleanly.
I do not understand this one. Why would we create the element when it already exists? It comes from the xul file. Or do you mean some HeaderView object, not xul element? Also, storing the data to be able to reset it would really add some complication. Actually, is the xul reparsed for each message so the elements are reset automatically?
Attached patch patch v3 (obsolete) — Splinter Review
Or you meant this?
Attachment #8407023 - Attachment is obsolete: true
Attachment #8407023 - Flags: review?(bwinton)
Attachment #8408948 - Flags: review?(squibblyflabbetydoo)
(In reply to :aceman from comment #27)
> (In reply to Jim Porter (:squib) from comment #26)
> > @@ +1004,5 @@
> > > +  let topViewNode = document.getElementById("expandedHeader2Rows");
> > > +  let newHeaderNode;
> > > +  // If a row for this header already exists, do not create another one.
> > > +  let newRowNode = topViewNode.querySelector("#" + rowId);
> > > +  if (!newRowNode) {
> > 
> > I think it would make more sense for this function to always create a new
> > HeaderView, but to give it a reset() method that resets its internal state
> > to how it was when it was first created.
> > 
> I do not understand this one. Why would we create the element when it
> already exists? It comes from the xul file. Or do you mean some HeaderView
> object, not xul element? Also, storing the data to be able to reset it would
> really add some complication. Actually, is the xul reparsed for each message
> so the elements are reset automatically?

Or do you mean to remove the elements from the xul file, make HeaderView create necessary elements at first use and then keep them until TB exits?
What I mean is something like this:

function HeaderView(headerName, label) {
  // Create and append the label which contains the header name.
  // [[Note this isn't inside an if statement]]
  // ...

  this.reset();
}

HeaderView.prototype = {
  reset: function() {
    this.valid = false;
    this.useToggle = false;
    this.outputFunction = updateHeaderValue;
  },
};

function getHeaderView(headerName, label) {
  let rowId = "expanded" + headerName + "Row";
  let newRowNode = document.getElementById(rowId);
  if (newRowNode) {
    newRowNode.reset();
    return newRowNode;
  }
  return new HeaderView(headerName, label);
}
(In reply to Jim Porter (:squib) from comment #26)
> I think it would make more sense for this function to always create a new
> HeaderView, but to give it a reset() method that resets its internal state
> to how it was when it was first created.
> 
> Maybe add a utility function to "create a new HeaderView or reset the
> existing one". I know that's adding more code to do basically the same
> thing, but I think it separates things more cleanly.

I'm going to retract this review comment. The part I was commenting on is fine. The other comments still apply though.
Thanks, then I think patch v3 includes the other comments. Can you look at it?
Comment on attachment 8408948 [details] [diff] [review]
patch v3

Review of attachment 8408948 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/msgHdrViewOverlay.js
@@ +854,5 @@
> +      enclosingBox.clearHeaderValues();
> +    else {
> +      while (enclosingBox.hasChildNodes())
> +        enclosingBox.lastChild.remove();
> +    }

I don't think we need this change.
Comment on attachment 8408948 [details] [diff] [review]
patch v3

r=me with comment 34 addressed.
Attachment #8408948 - Flags: review?(squibblyflabbetydoo) → review+
Attached patch patch v 3.1Splinter Review
OK, thanks.
Attachment #8408948 - Attachment is obsolete: true
Attachment #8413571 - Flags: review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
OS: Windows XP → All
Hardware: x86 → All
https://hg.mozilla.org/comm-central/rev/35badd55a546
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 31.0
Depends on: 1003525
Depends on: 1003693
You need to log in before you can comment on or make changes to this bug.