57115 - mime.properties file should contain correct HTML

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect, P3)

Description

[Henrik Gemal]

The "mime.properties" contains incorrect html, weird chars, etc..

Comment 1

[Henrik Gemal]

Attachment: Diff to fix incorrect HTML

Comment 2

[rhp (gone)]

Sorry, this isn't happening for rtm.

- rhp

Comment 3

[Henrik Gemal]

Perhaps this could be checked in now, since we're pasted rtm...:)

Comment 4

[scottputterman]

reassigning to ducarroz

Comment 5

[Henrik Gemal]

is this still nsbeta-

Comment 6

[:aceman]

Is this change wanted? Should I update it to current trunk?

Comment 7

[Ludovic Hirlimann [:Usul]]

(In reply to :aceman from comment #6)
> Is this change wanted? Should I update it to current trunk?

If something is still to be done : yes.

Comment 8

[:aceman]

You mean if that file wasn't changed meanwhile in some other bug?
It seems not, so it is still applicable:
http://hg.mozilla.org/comm-central/file/c38d6dd2c8b7/mail/locales/en-US/chrome/messenger/mime.properties

However I need somebody to agree to the content of the changes.
Tags are changed to lowercase. Is there a reason for that, any spec? If not, I'd like it better unchanged (so uppercase). Also, there is a <table> inside <center>. I am not sure that is the most correct HTML, but maybe we don't care here. Or should CSS be avoided there?

Comment 9

[Magnus Melin [:mkmelin]]

The properties in question may/should go away altogether soonish - see bug 492329.

Comment 10

[:aceman]

Could you please specify the relation to that bug more?
This one is mostly about the "truncated message" (because of limit on downloaded message size). The other one is about expired newsgroup message. To me that is completely unrelated. Is there any big overhaul of both these placeholders planned? I couldn't see something like that in those comments.

Comment 11

[Magnus Melin [:mkmelin]]

Yes, bug 492329 comment 12 - squib is planning on getting rid of DisplayHTMLInMessagePane which I think is where those strings are used.

Comment 12

[:aceman]

Ah, thanks. It isn't even filed yet. We'll see how soonish that one can be fixed. Thanks for catching that relation.

Comment 13

[Jim Porter (:squib)]

Hm, I'm not sure that bug is actually related. As far as I can tell, these strings aren't referenced in the two other places that DisplayHTMLInMessagePane is called. I expect these strings are used at a lower level, probably in one of the MIME emitters. If that's the case, removing DisplayHTMLInMessagePane wouldn't affect this.

Comment 14

[:aceman]

Magnus, what do you think?

Comment 15

[Magnus Melin [:mkmelin]]

Yeah, sorry i was mistaken.

Comment 16

[:aceman]

OK, I'll try to come up with something.

Comment 17

[:aceman]

Attachment: HTML updated to hopefully more modern style (HTML5)

This just changes the current design using <table> to one using <div>s and CSS. But the overal appearance of the placeholder is preserved.

If anybody has any suggestions to make the placeholder nicer, just comment while I am at it :)

Comment 18

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 578416 [details] [diff] [review]
HTML updated to hopefully more modern style (HTML5)

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

So, a couple of notes:

1) I'm gonna need a set of steps to see these strings, cause as it is, it's fairly hard to review.
2) I prefer lowercase tag and attribute names.
3) All the Localization Notes are wrong.
4) I suspect all the "%"s you added should really be "%%".

I'm going to clear out the ui-review flag until point #1 is addressed, since it's the main thing that's blocking me.

Thanks,
Blake.

Comment 19

[:aceman]

1. This "placeholder" is shown inside a message when it is downloaded incomplete (e.g. due to account settings->disk space->do not download messages larger than X Kb, or when "only download headers" is set). Receive a message larger than that and you'll see this code used (at least on POP3).
2. I asked for that in comment 8. No problem, I can do that. It just optically looks better to me, and it is easier to spot where is code and where is text.
3. Yes, will fix when the HTML code is agreed upon.
4. I am not sure. The patch works this way for me (with single %). The percentages are interpreted correctly in the real message. I have even checked that the old versions with %% made it into the real message and were still shown as %% in DOM Inspector (it allows to save the current DOM to a file and the %% are still there).

Comment 20

[:aceman]

Comment on attachment 578416 [details] [diff] [review]
HTML updated to hopefully more modern style (HTML5)

Maybe David will know if the % needs to be escaped (doubled).

Comment 21

[Magnus Melin [:mkmelin]]

You'd also have to change the property names (so localizers would pick up). Which in this case is way painful :( I believe there's a bug about that painfulness...

Comment 22

[:aceman]

Thanks, I always forget about that.

Comment 23

[:aceman]

Do you know how to rename the names here? They are just numbers. Should I choose new ones? How? Which numbers are free to use? Is there any increasing ID counter?

There are also some @name identifiers inside comments. Are those automatically used anywhere? Should they be changed?

I have only updated .dtd files so far. This is my first real .properties file. Please advice me. Thanks.

Comment 24

[Magnus Melin [:mkmelin]]

Need to look at the @name in the properties file, and find all references to it. Like @name MIME_MSG_PARTIAL_FMT_1. Then just choose the next available number.

Comment 25

[:aceman]

This will tell me how to change the 1037 number?

Comment 26

[Magnus Melin [:mkmelin]]

Yes, you need to change http://mxr.mozilla.org/comm-central/source/mailnews/mime/src/nsMimeStringResources.h#69 and similar for the other ones of course.

Comment 27

[:aceman]

Does this mean I can change 1037 to 1045? Are these number unique globally across Thunderbird? Or do they just need to be unique in this file (and each file has its own number range)?

Comment 28

[Magnus Melin [:mkmelin]]

They should be unique for MimeGetStringByID() usage, so yes unique to that file afaikt (ok to change to 1045).

Comment 29

[:aceman]

Thanks a lot.

Comment 30

[:aceman]

Comment on attachment 578416 [details] [diff] [review]
HTML updated to hopefully more modern style (HTML5)

Still needs to resolve the % escaping problem (comment 20).

Comment 31

[David :Bienvenu]

Comment on attachment 578416 [details] [diff] [review]
HTML updated to hopefully more modern style (HTML5)

I'm not the best person to review this, but it looks OK when I run with it.

Comment 32

[Ludovic Hirlimann [:Usul]]

(In reply to David :Bienvenu from comment #31)
> Comment on attachment 578416 [details] [diff] [review]
> HTML updated to hopefully more modern style (HTML5)
> 
> I'm not the best person to review this, but it looks OK when I run with it.

Who should it be ?

Comment 33

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 578416 [details] [diff] [review]
HTML updated to hopefully more modern style (HTML5)

If it's not bienvenu, I think it's :squib, since he's been doing a lot of mime-related attachment stuff...

To reiterate Blake's request before he gives UX-review, the localization notes are still explicitly calling out the old HTML.

Comment 34

[:aceman]

Comment on attachment 578416 [details] [diff] [review]
HTML updated to hopefully more modern style (HTML5)

Yes, the notes are wrong (and string IDs), that is why we did not request review. But we need somebody to decide if I should continue with the patch. If it will eventually be accepted.

Comment 35

[Jim Porter (:squib)]

Comment on attachment 578416 [details] [diff] [review]
HTML updated to hopefully more modern style (HTML5)

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

Overall, this looks ok. I think when you go about renaming the string names, you can probably rename them to actual names, provided you update the relevant places in libmime that call this. That'll make life easier for anyone in the future.

::: mail/locales/en-US/chrome/messenger/mime.properties
@@ +151,5 @@
>  # Partial Message Format 1
>  ## @name MIME_MSG_PARTIAL_FMT_1
>  ## @loc
>  # LOCALIZATION NOTE (1037): In the following line, translate only the word, "Truncated!".
> +1037=<DIV STYLE="margin: 1em auto; border: 1px solid black; width: 80%"><DIV STYLE="margin: 5px; padding: 10px; border: 1px solid gray; font-weight: bold; text-align: center;"><SPAN STYLE="font-size: 120%;">Truncated!</SPAN><HR>

I'd really like to see the styles be defined in an external stylesheet, but that might be out of the scope for this bug.

@@ +166,5 @@
>  ## @name MIME_MSG_PARTIAL_FMT_3
>  ## @loc
>  # LOCALIZATION NOTE (1039): This section must begin with the ">" sign and end with the tags,"</B></TD></TR></TABLE></CENTER>"
>  # Do not translate "</A>" tag.
> +1039=">Click here to download the rest of the message.</A></DIV></DIV>

If we're going to change what text is the link, I think we should just improve the text. How about "Download the rest of the message."? "Click here" is redundant for links.

@@ +199,5 @@
>  ## @name MIME_MSG_PARTIAL_FMT2_3
>  ## @loc
>  # LOCALIZATION NOTE (1044): This section must begin with the ">" sign and end with the tags,"</B></TD></TR></TABLE></CENTER>"
>  # Do not translate "</A>" tag.
> +1044=">Click here to download the rest of the message.</A></DIV></DIV>

Likewise, let's change this text too.

Comment 36

[:aceman]

Change the numbers to names as in other .properties files?
Can I then remove the defines from here: http://mxr.mozilla.org/comm-central/source/mailnews/mime/src/nsMimeStringResources.h#69 ? And just call the strings from source by the new name?

I would define the style in a .css but would not like it to be part of the theme. I think that would be needless duplication in all themes. Is there some TB private css file where this can be stuffed? Or where to create one?

Comment 37

[Jim Porter (:squib)]

(In reply to :aceman from comment #36)
> Change the numbers to names as in other .properties files?
> Can I then remove the defines from here:
> http://mxr.mozilla.org/comm-central/source/mailnews/mime/src/
> nsMimeStringResources.h#69 ? And just call the strings from source by the
> new name?

Correct.

> I would define the style in a .css but would not like it to be part of the
> theme. I think that would be needless duplication in all themes. Is there
> some TB private css file where this can be stuffed? Or where to create one?

Well, we're already well past that point for the theme code. We could* always make mail/themes/common and put common CSS there, but that's a pretty big undertaking. Again though, I wouldn't r- a patch that puts the styles in the strings like it is now.

Oh also, if it turns out to be easy, you could put the HTML in the .cpp files, but that's also totally optional.

* I think. Maybe it's not possible...

Comment 38

[:aceman]

(In reply to Jim Porter (:squib) from comment #37)
> Oh also, if it turns out to be easy, you could put the HTML in the .cpp
> files, but that's also totally optional.
> 
> * I think. Maybe it's not possible...

Good idea, I can look at that.

Comment 39

[:aceman]

Attachment: patch v3

What about this?
HTML included into code, string Names, HTML tags lowercased. Great for translators, no more notes needed. Duplicate strings of "Download the rest of the message" merged. Added the same strings for Seamonkey.
Anything else?

Comment 40

[Ian Neal]

Comment on attachment 594805 [details] [diff] [review]
patch v3

>+  partialMsgHtml.AppendLiteral("<div style='margin: 1em auto; border: 1px solid black; width: 80%'>");
>+  partialMsgHtml.AppendLiteral("<div style='margin: 5px; padding: 10px; border: 1px solid gray; font-weight: bold; text-align: center;'>");
>+
>+  partialMsgHtml.AppendLiteral("<span style='font-size: 120%;'>");
As a follow-up bug, as suggested elsewhere, could these styles be moved to themes.

>+++ b/suite/locales/en-US/chrome/mailnews/mime.properties
>+# Partial Message Truncated Explanation
>+## @name MIME_MSG_PARTIAL_TRUNCATED_EXPLANATION
> ## @loc
>+MIME_MSG_PARTIAL_TRUNCATED_EXPLANATION=This message exceeded the Maximum Message Size set in Account Settings, so we have only downloaded the first few lines from the mail server.
Nit: Is "we" usually used in messages? Alternate suggestion "so only the first few lines have been downloaded from the mail server."

>+# Partial Message Not Downloaded Explanation
>+## @name MIME_MSG_PARTIAL_NOT_DOWNLOADED_EXPLANATION
>+## @loc
>+MIME_MSG_PARTIAL_NOT_DOWNLOADED_EXPLANATION=Only the headers for this message were downloaded from the mail server.
Nit: Potentially this should be "have been" or the other alternate explanation changed to "were"

Comment 41

[:aceman]

(In reply to Ian Neal from comment #40)
> Comment on attachment 594805 [details] [diff] [review]
> patch v3
> 
> >+  partialMsgHtml.AppendLiteral("<div style='margin: 1em auto; border: 1px solid black; width: 80%'>");
> >+  partialMsgHtml.AppendLiteral("<div style='margin: 5px; padding: 10px; border: 1px solid gray; font-weight: bold; text-align: center;'>");
> >+
> >+  partialMsgHtml.AppendLiteral("<span style='font-size: 120%;'>");
> As a follow-up bug, as suggested elsewhere, could these styles be moved to
> themes.

Yes it would be nice but I think also squib concluded that there is not yet infrastructure for this. We'd like this to be in some css file but not in the theme. Do you know such a place?

Comment 42

[Jim Porter (:squib)]

Comment on attachment 594805 [details] [diff] [review]
patch v3

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

I just took a quick look at the code; I didn't compile with this. r=me, with the issues below fixed, and assuming iann made sure this works. :)

::: mailnews/mime/src/mimemsg.cpp
@@ +871,5 @@
>    if (gtPtr)
>      *gtPtr = 0;
>  
> +  bool msgBaseTruncated;
> +  msgBaseTruncated = (msg->bodyLength > MSG_LINEBREAK_LEN);

Just put the declaration/assignment on one line, please.

@@ +879,5 @@
>  
> +  partialMsgHtml.AppendLiteral("<div style='margin: 1em auto; border: 1px solid black; width: 80%'>");
> +  partialMsgHtml.AppendLiteral("<div style='margin: 5px; padding: 10px; border: 1px solid gray; font-weight: bold; text-align: center;'>");
> +
> +  partialMsgHtml.AppendLiteral("<span style='font-size: 120%;'>");

Use double quotes here for the style definition here and in other places. If you're unfamiliar with C++, you'll need to escape the quotes with backslashes like so: AppendLiteral("<div style=\"foo: bar;\">");

@@ +897,2 @@
>  
> +  partialMsgHtml.AppendLiteral("<a href='");

Double-quotes here.

@@ +915,5 @@
>  
>      partialMsgHtml.Append(item);
>    }
>  
> +  partialMsgHtml.AppendLiteral("'>");

Double-quotes here too.

Comment 43

[Magnus Melin [:mkmelin]]

(In reply to Jim Porter (:squib) from comment #42)
> > +  partialMsgHtml.AppendLiteral("<span style='font-size: 120%;'>");
> 
> Use double quotes here for the style definition here and in other places. 

Why do you want that? This seems like an ideal case for single quotes. 
(I think double quotes should be used when there is no reason to use single, but here it seems valid for readability.)

Comment 44

[Jim Porter (:squib)]

(In reply to Magnus Melin from comment #43)
> Why do you want that? This seems like an ideal case for single quotes. 
> (I think double quotes should be used when there is no reason to use single,
> but here it seems valid for readability.)

Most of the HTML we have in Thunderbird is actually formatted as XHTML, where double quotes are required. I think we should continue this habit.

Comment 45

[:aceman]

Attachment: patch v4

Thanks to all, carrying over reviews.
Does this still need ui-review? The look of the table is mostly unchanged and I addressed bwinton's nits.

Comment 46

[Magnus Melin [:mkmelin]]

(In reply to Jim Porter (:squib) from comment #44)
> Most of the HTML we have in Thunderbird is actually formatted as XHTML,
> where double quotes are required. 

XHTML doesn't require double quotes.

Comment 47

[Jim Porter (:squib)]

(In reply to Magnus Melin from comment #46)
> (In reply to Jim Porter (:squib) from comment #44)
> > Most of the HTML we have in Thunderbird is actually formatted as XHTML,
> > where double quotes are required. 
> 
> XHTML doesn't require double quotes.

Hm. Well, I'd still prefer it, since it's pretty universally-used in Mozilla, but I won't hold it against the patch otherwise.

Comment 48

[:aceman]

It is less readable, but I have already done it in v4.

Comment 49

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/03d5af18339f

Comment 50

[:aceman]

It looks like there are .css files in mail/base/content. Would it be OK to stuff the css in some of them?