The askSendFormat code is really ancient. (I'd like to get rid of dialogs.css later, so this is one step). For the rtl dialogs.css changes: rtl is already handled by xul.

Fix problem with previous patch

Comment on attachment 391935 [details] [diff] [review] proposed fix, v2 >+pref("mail.asksendformat.default", 1); // 1=plaintext, 2=html, 3=both >+pref("mail.asksendformat.recommended_as_default", true); >+pref("mail.asksendformat.display_recommendation", true); These prefs are *only* used by askSendFormat.js, but nowhere else - noone ever changes their values, no config panel, nothing. CVS history claims they come from bug 51547 (which is still inaccessible for some strange reason without being security-related!) and were added in 2000... Actually, I'd say they are rather useless anyway (especially "display_recommendation") - just drop them and their handling. (I don't think it's very likely that we ever change the default from plain text to html, given the option "both" is in the field as well). And Bugs 44494 and 52222 didn't see much traction for some years as well. Neil, what do you think?

mail.asksendformat.default is actually set to 3 in Netscape. There is also a MozillaZine post where someone explains how to do that in Thunderbird. So, two hits on the entire Internet. Maybe we can get away with it ;-) Now a return question: would it be a problem to change the convertibleYes icon to be the message-icon rather than the question-icon?

Comment on attachment 391935 [details] [diff] [review] proposed fix, v2 Formally minusing.

Getting rid of all the unneeded prefs certainly makes the code easier to read

Comment on attachment 395918 [details] [diff] [review] proposed fix, v3 >+ * abort:false} Well, that's really an outparam ;-) >+ // If the user hits the close box, we will abort. >+ gParam.abort = true; The close box actually invokes Cancel(), so we can eliminate this line... >+ case msgCompConvertible.Plain: >+ // We shouldn't be here at all >+ mailSendFormatExplanation.textContent = bundle.getString("convertibleYes"); >+ icon.setAttribute("class", "message-icon"); >+ break; >+ case msgCompConvertible.Yes: This can be shortened to default: >+ icon.setAttribute("class", "message-icon"); Nit: could use .className >+ var radioButtons = group.getElementsByTagName("radio"); Missing var group = document.getElementById("mailDefaultHTMLAction"); ? >+ var format = gParam.action || msgCompSendFormat.PlainText; ^ Typo? >+ switch (format) I think it should be possible to replace this entire switch with var radio = group.getElementsByAttribute("value", format); or, after setting the value, var radio = group.selectedItem; >+ if (haveRecommendation) if (radio) > function Cancel() > { >- param.abort = true; >+ gParam.abort = true; > } ... or this method could be removed entirely, as we already set it earlier. >+ style="width: 40em;"> Would you mind changing this to a suitable ch value?

> >+ var format = gParam.action ||Â msgCompSendFormat.PlainText; ^ Typo? I don't see this in the patch, the other stuff i've fixed

Comment on attachment 396071 [details] [diff] [review] proposed fix, v4 >+ var radioButtons = group.getElementsByTagName("radio"); Unused? >+ group.value = gParam.action || msgCompSendFormat.PlainText; " " is a UTF-8 nonbreaking space.

Although you can't see that because Gecko submits them as ordinary spaces.

Comment on attachment 396071 [details] [diff] [review] proposed fix, v4 >+/** >+ * This dialog should be opened with arguments like e.g. >+ * {action:nsIMsgCompSendFormat.AskUser, convertible:nsIMsgCompConvertible.Yes} Nit: missing space behind ':'. > function Startup() ... >+ // Set the default radio array value and recommendation. >+ var group = document.getElementById("mailDefaultHTMLAction"); >+ var radioButtons = group.getElementsByTagName("radio"); >+ group.value = gParam.action ||Â msgCompSendFormat.PlainText; This is wrong. Given gParam.action was 4 (Ask), the old code knew that there isn't a radio with that value and chose the default action. Now the default (plain text) will only ever apply if somehow the action 0 (undefined!) is passed, which is not possible by current SM/TB code. The gParam.action 4 will select the highest radio value possible, which happens to be 3 (both) here... (My testcase: account with HTML compose; default "Ask"; mail to "a@b"; body text "a" with HTML colour red => action=4(Ask), convert=4(No).) BTW: "Â " are the UTF-8 representation bytes of a shift-space 0xA0. Please don't.

My editor don't show the odd spaces either, don't know how they ended up there. Anyway, erased and rewrote so they should be gone now. With rec: window.openDialog("chrome://messenger/content/messengercompose/askSendFormat.xul", "", "chrome,modal,titlebar,centerscreen", {action:Components.interfaces.nsIMsgCompSendFormat.HTML,convertible:Components.interfaces.nsIMsgCompConvertible.Altering}); window.openDialog("chrome://messenger/content/messengercompose/askSendFormat.xul", "", "chrome,modal,titlebar,centerscreen", {action:Components.interfaces.nsIMsgCompSendFormat.PlainText,convertible:Components.interfaces.nsIMsgCompConvertible.Altering}); window.openDialog("chrome://messenger/content/messengercompose/askSendFormat.xul", "", "chrome,modal,titlebar,centerscreen", {action:Components.interfaces.nsIMsgCompSendFormat.Both,convertible:Components.interfaces.nsIMsgCompConvertible.Altering}); window.openDialog("chrome://messenger/content/messengercompose/askSendFormat.xul", "", "chrome,modal,titlebar,centerscreen", {action:Components.interfaces.nsIMsgCompSendFormat.PlainText,convertible:Components.interfaces.nsIMsgCompConvertible.Yes}); window.openDialog("chrome://messenger/content/messengercompose/askSendFormat.xul", "", "chrome,modal,titlebar,centerscreen", {action:Components.interfaces.nsIMsgCompSendFormat.HTML,convertible:Components.interfaces.nsIMsgCompConvertible.Yes}); window.openDialog("chrome://messenger/content/messengercompose/askSendFormat.xul", "", "chrome,modal,titlebar,centerscreen", {action:Components.interfaces.nsIMsgCompSendFormat.Both,convertible:Components.interfaces.nsIMsgCompConvertible.Yes}); window.openDialog("chrome://messenger/content/messengercompose/askSendFormat.xul", "", "chrome,modal,titlebar,centerscreen", {action:Components.interfaces.nsIMsgCompSendFormat.PlainText,convertible:Components.interfaces.nsIMsgCompConvertible.No}); window.openDialog("chrome://messenger/content/messengercompose/askSendFormat.xul", "", "chrome,modal,titlebar,centerscreen", {action:Components.interfaces.nsIMsgCompSendFormat.HTML,convertible:Components.interfaces.nsIMsgCompConvertible.No}); window.openDialog("chrome://messenger/content/messengercompose/askSendFormat.xul", "", "chrome,modal,titlebar,centerscreen", {action:Components.interfaces.nsIMsgCompSendFormat.Both,convertible:Components.interfaces.nsIMsgCompConvertible.No}); --- No rec: window.openDialog("chrome://messenger/content/messengercompose/askSendFormat.xul", "", "chrome,modal,titlebar,centerscreen", {action:Components.interfaces.nsIMsgCompSendFormat.AskUser,convertible:Components.interfaces.nsIMsgCompConvertible.Altering}); window.openDialog("chrome://messenger/content/messengercompose/askSendFormat.xul", "", "chrome,modal,titlebar,centerscreen", {action:Components.interfaces.nsIMsgCompSendFormat.AskUser,convertible:Components.interfaces.nsIMsgCompConvertible.Yes}); window.openDialog("chrome://messenger/content/messengercompose/askSendFormat.xul", "", "chrome,modal,titlebar,centerscreen", {action:Components.interfaces.nsIMsgCompSendFormat.AskUser,convertible:Components.interfaces.nsIMsgCompConvertible.No});

Comment on attachment 396135 [details] [diff] [review] proposed fix, v5 >+ style="width: 67ch;"> To my eyes, 75ch looks nearest, however 80ch or simply omitting the width also works for me.

(In reply to comment #3) > Now a return question: would it be a problem to change the convertibleYes icon > to be the message-icon rather than the question-icon? So, it turns out message-icon has always been broken in winstripe :-( The trick with the radiogroup doesn't quite work because I forgot that I reviewed the patch that made radiogroups default to their first button...

Comment on attachment 396135 [details] [diff] [review] proposed fix, v5 >+ icon.className = "message-icon"; Have to use question-icon here, unless by some miracle bug 512173 gets fixed on 1.9.1. >+ var recRadio = group.getElementsByAttribute("value", gParam.action)[0]; >+ if (recRadio) >+ recRadio.label += " " + bundle.getString("recommended"); >+ group.value = gParam.action; Now that we know the action is valid, group.value = gParam.action; group.selectedItem.label += " " etc. should work. >+ group.value = msgCompSendFormat.PlainText; We could avoid this by adding value="1" to the group or selected="true" to the plain text radio.

For checkin.

Maybe thunderbird needs a sm2 approval flag too, for patches to shared code...

Given mailnews/compose/content/askSendFormat.js is the core of this patch, the actual SeaMonkey rule that can be applied here is: "Patches to mailnews/ that need or are only reasonable with changes to both mail/ and suite/ and are accepted for Thunderbird 3 are excepted for now and can land without the extra hassle of SeaMonkey approval." We know why we added this rule ;-)

changeset: 3547:c014663a4390 http://hg.mozilla.org/comm-central/rev/c014663a4390 ->FIXED