429143 - Migrate Composition prefpane

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Bruno 'Aqualon' Escherl]

This bug is for the migration of MailNews' Composition prefpane.

Comment 1

[Bruno 'Aqualon' Escherl]

Attachment: Migration of the prefpane v1

Comment 2

[Bruno 'Aqualon' Escherl]

Attachment: diff -w version of patch v1

Comment 3

[Ian Neal]

Comment on attachment 315787 [details] [diff] [review]
Migration of the prefpane v1

>Index: mailnews/compose/prefs/resources/content/pref-composing_messages.js
>===================================================================

>+function Startup()
>+{
>+  var value = document.getElementById("mail.compose.autosave").value;
>+  EnableTextbox("autoSaveInterval", value, false);
See below.
>+}
>+
>+function EnableMailComposeAutosaveInterval(aValue)
>+{
>+  var focus = (document.getElementById("autoSave") == document.commandDispatcher.focusedElement);
>+  EnableTextbox("autoSaveInterval", aValue, focus);
See below.
> }

>+function EnableTextbox(aTextboxId, aValue, aFocus)
As you are passing the same first argument in for both calls to this function, why bother? Just use "autoSaveInterval" within the function.
>+{
>+  var textbox = document.getElementById(aTextboxId);
>+  var pref = document.getElementById("mail.compose.autosave");
>+  var enable = aValue && !pref.locked;
>+
>+  textbox.disabled = !enable;
> 
>+  if (!textbox.disabled && aFocus)
As you have what !textbox.disabled is anyway (enable) why not just use that giving:
if (enable && aFocus)
>+    textbox.focus();
> }

>Index: mailnews/base/prefs/resources/content/mailPrefsOverlay.xul
>===================================================================
>@@ -61,16 +61,21 @@
>               id="mailnewsItem"
>               insertafter="navigator"
>               label="&mail.label;"
>               prefpane="mailnews_pane"
>               url="chrome://messenger/content/pref-mailnews.xul"
>               helpTopic="mail_prefs_general">
>       <treechildren id="messengerChildren">
>         <!-- XXX Move pref panes from below to here as they are migrated -->
>+        <treeitem id="composingItem"
>+                  label="&composingMessages.label;"
>+                  prefpane="composing_messages_pane"
>+                  url="chrome://messenger/content/messengercompose/pref-composing_messages.xul"
>+                  helpTopic="mail_prefs_messages"/>
You've not done the changes to the old entry that appears in the Legacy Prefs window.

Comment 4

[[e-mail]]

Comment on attachment 315787 [details] [diff] [review]
Migration of the prefpane v1

>-  aTextbox.disabled = (!aCheckbox.checked ||
>-                       parent.hPrefWindow.getPrefIsLocked(aTextbox.getAttribute("prefstring")));
>+  var pref = document.getElementById("mail.compose.autosave");
As I mentioned elsewhere you should use aTextbox.getAttribute("preference")

>         <!-- XXX Move pref panes from below to here as they are migrated -->
XXX Move!

Comment 5

[Bruno 'Aqualon' Escherl]

Attachment: Migration of the prefpane v2

Patch v2 using EnableTextbox from preferences.js and addressing review comments.

Comment 6

[Bruno 'Aqualon' Escherl]

Attachment: diff -w version of patch v2

Comment 7

[Ian Neal]

Comment on attachment 317104 [details] [diff] [review]
Migration of the prefpane v2

>Index: mailnews/compose/prefs/resources/content/pref-composing_messages.js
>===================================================================

>+function Startup()
>+{
>   if ("@mozilla.org/spellchecker;1" in Components.classes)
>     InitLanguageMenu();
>   else
>     document.getElementById("spellingGroup").hidden = true;
> 
>+  var value = document.getElementById("mail.compose.autosave").value;
Why "var" here and "let" in pref-viewing_messages?

>+  EnableTextbox("autoSaveInterval", value, false);
> }
> 
>+function EnableMailComposeAutosaveInterval(aValue)
>+{
>+  var focus = (document.getElementById("autoSave") == document.commandDispatcher.focusedElement);
"var" versus "let"?

>+  EnableTextbox("autoSaveInterval", aValue, focus);
> }

Comment 8

[Bruno 'Aqualon' Escherl]

Attachment: Migration of the prefpane v3

I forgot the call of PopulateFonts() in v2, so the font list was quite empty.

Comment 9

[Karsten Düsterloh]

Comment on attachment 319298 [details] [diff] [review]
Migration of the prefpane v3

>Index: mailnews/compose/prefs/resources/content/pref-composing_messages.js
>===================================================================
>-function Startup() {
>+function Startup()
>+{

Please adhere to the prevalent bracing style, even though it is disgusting. ;-)

>+  let value = document.getElementById("mail.compose.autosave").value;

(Just a sidenote: let vs. var seems rather esoteric on a function's main level, especially when most var uses could in fact even be consts, but with let in town I don't see any use of var anymore anyway...)

>Index: mailnews/compose/prefs/resources/content/pref-composing_messages.xul
>===================================================================
> <?xml-stylesheet href="chrome://communicator/skin/" type="text/css"?>

This is not needed, it's already included by preferences.xul.

>+      <radiogroup id="forwardMessageMode" orient="horizontal" align="center"
>+                  preference="mail.forward_message_mode">
>+        <label value="&forwardMsg.label;" control="forwardMessageMode"/>
>+        <radio value="2" label="&inline.label;" accesskey="&inline.accesskey;"/>
>+        <radio value="0" label="&asAttachment.label;"
>+               accesskey="&asAttachment.accesskey;"/>

Above and in all markup below:
Usually use one line per attribute (unless maybe there just two and they fit onto the line), align them with the first one and let accesskeys directly follow their labels, etc.

Comment 10

[Bruno 'Aqualon' Escherl]

Attachment: Migration of the prefpane v4

Carrying over sr, patch addresses review comments and I removed unnecessary attribute "prefattribute".

Comment 11

[Karsten Düsterloh]

Comment on attachment 319767 [details] [diff] [review]
Migration of the prefpane v4

>Index: mailnews/base/prefs/resources/content/mailPrefsOverlay.xul
>===================================================================
>+        <label value="&languagePopup.label;"
>+               control="languageMenuList"
>+               accesskey="&languagePopup.accessKey;"/>

The control attribute should usually be last and value and accesskey should be next to one another in a label element.

>+          <label control="fontSelect"
>+                 value="&font.label;"
>+                 accesskey="&font.accesskey;"/>
...
>+          <label control="fontSizeSelect"
>+                 value="&size.label;"
>+                 accesskey="&size.accesskey;"/>
...
>+          <label control="textColor"
>+                 value="&fontColor.label;"
>+                 accesskey="&fontColor.accesskey;"/>

See above.

>+          <colorpicker type="button"
>+                       id="textColor"
>+                       preference="msgcompose.text_color"/>

"id" should be the first attribute, if given.

>+          <label control="backgroundColor"
>+                 value="&bgColor.label;"
>+                 accesskey="&bgColor.accesskey;"/>

See above.

>+          <colorpicker type="button"
>+                       id="backgroundColor"
>+                       preference="msgcompose.background_color"/>

See above.

r=me with that.

Comment 12

[Ian Neal]

Comment on attachment 319767 [details] [diff] [review]
Migration of the prefpane v4

>Index: mailnews/compose/prefs/resources/content/pref-composing_messages.js
>===================================================================
>@@ -1,28 +1,61 @@
>+ * The Original Code is SeaMonkey project code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Ian Neal <[e-mail]>.
Could you change the above email address to [e-mail]

>Index: mailnews/compose/prefs/resources/content/pref-composing_messages.xul
>===================================================================
>@@ -1,129 +1,237 @@
>+        <checkbox id="autoSave" label="&autoSave.label;"
>+                  preference="mail.compose.autosave"
>+                  accesskey="&autoSave.accesskey;"
>+                  aria-labelledby="autoSave autoSaveInterval autoSaveEnd"/>
>+        <textbox id="autoSaveInterval"
>+                 size="2"
>+                 preference="mail.compose.autosaveinterval"
>+                 aria-labelledby="autoSave autoSaveInterval autoSaveEnd"/>
Should be looking to use textbox type="number" here.

>+        <label id="autoSaveEnd" value="&autoSaveEnd.label;"/>
>       </hbox>

>+        <label id="wrapOutMsg"
>+               value="&wrapOutMsg.label;"
>+               accesskey="&wrapOutMsg.accesskey;"
>+               control="wrapLength"/>
>+        <textbox id="wrapLength"
>+                 size="3"
>+                 preference="mailnews.wraplength"
>+                 aria-labelledby="wrapOutMsg wrapLength char"/>
Should be looking to use textbox type="number" here.

>+        <label id="char" value="&char.label;"/>
>+      </hbox>
Possibly look at implementing UI for new pref introduced in fix to bug 384599 here.
>+    </groupbox>

Comment 13

[Stefan [:stefanh]]

Maybe we could get the textbox type="number" stuff in a separate bug and change everything at once?

Comment 14

[Bruno 'Aqualon' Escherl]

Attachment: Migration of the prefpane v5

Patch integrates notes from comment #11 and comment #12. The gui should be done in a seperate bug (see bug 441659).

Comment 15

[Ian Neal]

Comment on attachment 326565 [details] [diff] [review]
Migration of the prefpane v5

>Index: mailnews/compose/prefs/resources/content/pref-composing_messages.js
>===================================================================
>+ * The Initial Developer of the Original Code is
>+ * Ian Neal <[e-mail]>.
I'm not sure I did so I should just be listed as a contributor and the above set to "Netscape Communications Corp."

Note: I am changing EnableTextbox in bug 441403 to EnableElementById

>Index: mailnews/compose/prefs/resources/content/pref-composing_messages.xul
>===================================================================

>+<!DOCTYPE overlay [
> <!ENTITY % pref-composing_messagesDTD SYSTEM "chrome://messenger/locale/messengercompose/pref-composing_messages.dtd" >
Nit: remove space before >

> %pref-composing_messagesDTD;
> <!ENTITY % editorOverlayDTD SYSTEM "chrome://editor/locale/editorOverlay.dtd" >
Nit: remove space before >

>+        <label id="wrapOutLabel"
>+               value="&wrapOutMsg.label;"
>+               accesskey="&wrapOutMsg.accesskey;"
>+               control="wrapLength"/>
>+        <textbox id="wrapLength"
>+                 type="number"
>+                 min="0"
>+                 max="999"
>+                 size="3"
>+                 preference="mailnews.wraplength"
>+                 aria-labelledby="wrapOutLabel wrapLength wrapOutEnd"/>
>+        <label id="wrapOutEnd" value="&char.label;"/>
Nit: Don't use aria-labelledby here, you can use control on second label and point it to the textbox element too. Then you do not need ids on either label.

>+        <menulist id="languageMenuList" preference="spellchecker.dictionary">
Nit: preference attribute should be on a separate line to menulist's id

>+          <menupopup onpopupshowing="InitLanguageMenu();">
>+            <!-- dynamic content populated by JS -->
>+            <menuseparator/>
>+            <menuitem value="more-cmd" label="&moreDictionaries.label;"/>
Nit: label should be on a separate line to menuitem's value

It would be good if the Font and Size lines could line up with each other (so menulists start inline with each other) - perhaps use a grid?
>+      <vbox>
>+        <hbox align="center">
>+          <label value="&font.label;"
>+                 accesskey="&font.accesskey;"
>+                 control="fontSelect"/>
>+          <menulist id="fontSelect" preference="msgcompose.font_face">
Nit: preference attribute should be on a separate line to the id.

>+            <menupopup>
>+              <menuitem value=""
>+                        label="&fontVarWidth.label;"/>
>+              <menuitem value="tt"
>+                        label="&fontFixedWidth.label;"/>
>+              <menuseparator/>
>+              <menuitem value="Helvetica, Arial, sans-serif"
>+                        label="&fontHelveticaFont.label;"/>
>+              <menuitem value="Times New Roman, Times, serif"
>+                        label="&fontTimes.label;"/>
>+              <menuitem value="Courier New, Courier, monospace"
>+                        label="&fontCourier.label;"/>
>+              <menuseparator/>
>+            </menupopup>
>+          </menulist>
>+        </hbox>
>+        <hbox align="center">
>+          <label value="&size.label;"
>+                 accesskey="&size.accesskey;"
>+                 control="fontSizeSelect"/>
>+          <menulist id="fontSizeSelect" preference="msgcompose.font_size">
Nit: preference attribute should be on a separate line to the id.

Note: this is just on code inspection only, not testing as yet.

Comment 16

[Karsten Düsterloh]

> >+        <label id="wrapOutLabel"
> >+               value="&wrapOutMsg.label;"
> >+               accesskey="&wrapOutMsg.accesskey;"
> >+               control="wrapLength"/>
> >+        <textbox id="wrapLength"
> >+                 type="number"
> >+                 min="0"
> >+                 max="999"
> >+                 size="3"
> >+                 preference="mailnews.wraplength"
> >+                 aria-labelledby="wrapOutLabel wrapLength wrapOutEnd"/>
> >+        <label id="wrapOutEnd" value="&char.label;"/>
> Nit: Don't use aria-labelledby here, you can use control on second label and
> point it to the textbox element too. Then you do not need ids on either label.

But that's what MarcoZ says here:
<http://wiki.mozilla.org/index.php?title=SeaMonkey:Toolkit_Transition:PrefwindowPanes:HowTo#Properly_labelling_controls>?

Comment 17

[Ian Neal]

(In reply to comment #16)
> > >+        <label id="wrapOutLabel"
> > >+               value="&wrapOutMsg.label;"
> > >+               accesskey="&wrapOutMsg.accesskey;"
> > >+               control="wrapLength"/>
> > >+        <textbox id="wrapLength"
> > >+                 type="number"
> > >+                 min="0"
> > >+                 max="999"
> > >+                 size="3"
> > >+                 preference="mailnews.wraplength"
> > >+                 aria-labelledby="wrapOutLabel wrapLength wrapOutEnd"/>
> > >+        <label id="wrapOutEnd" value="&char.label;"/>
> > Nit: Don't use aria-labelledby here, you can use control on second label and
> > point it to the textbox element too. Then you do not need ids on either label.
> 
> But that's what MarcoZ says here:
> <http://wiki.mozilla.org/index.php?title=SeaMonkey:Toolkit_Transition:PrefwindowPanes:HowTo#Properly_labelling_controls>?
> 

MarcoZ>	IanN: Well, you can accomplish the wanted without ARIA, so use conventionals. ARIA should be used as a last resort, or with funny constructs where the label of a checkbox is the label for an adjacent textbox at the same time. So, use the label first, then the textbox, then the description, and point both control attributes to the textbox.

Comment 18

[Marco Zehe (:MarcoZ)]

No, in this case aria-labelledby is, indeed, the correct approach, because both labels and the textbox form a single sentence. The word "characters" in the second label is not an additional description, it completes the sentence started by the first label.

Comment 19

[Bruno 'Aqualon' Escherl]

Attachment: Migration of the prefpane v6

This patch addresses Ian's nits from comment 15. I put the font and size menulists in a grid, so they're correctly aligned.

Note: the patch needs the change from bug 448107 to work.

Comment 20

[Karsten Düsterloh]

Comment on attachment 331575 [details] [diff] [review]
Migration of the prefpane v6

>+              <label value="&fontColor.label;"
>+                     accesskey="&fontColor.accesskey;"
>+                     control="textColor"/>
>+              <colorpicker id="textColor"
>+                           type="button"
>+                           preference="msgcompose.text_color"/>
>+              <label control="backgroundColor"
>+                     value="&bgColor.label;"
>+                     accesskey="&bgColor.accesskey;"/>
>+              <colorpicker id="backgroundColor"
>+                           type="button"
>+                           preference="msgcompose.background_color"/>

r=me with textColor and backgroundColor changed to something less generic.

Comment 21

[Bruno 'Aqualon' Escherl]

Attachment: Migration of the prefpane v7

I changed textColor to MsgComposeTextColor and backgroundColor to MsgComposeBackgroundColor.

Taking over r+/sr+

Comment 22

[Karsten Düsterloh]

Landed v7 on trunk, with msgComposeTextColor/msgComposeBackgroundColor instead of MsgComposeTextColor/MsgComposeBackgroundColor.