This bug is for the migration of MailNews' message display prefpane.

The patch migrates the message display pane to the new prefwindow. The license block from pref-viewing_messages.xul is missing cause I didn't know what exactly to write for initial developer and contributors. I asked about that in the mozilla.legal newsgroup and will include it when there's an answer to my question (see http://groups.google.com/group/mozilla.legal/browse_thread/thread/649f8c7eb336792f#)

Comment on attachment 313927 [details] [diff] [review] diff -w version of patch v1 >+let checkbox; >+let textbox; Does that even make sense here? But I wouldn't bother with saving them anyway. >+ textbox.disabled = (!checkbox.checked || isPrefLocked(textbox)); Note: you'll need to modify this to switch back to Startup. >+ if (!textbox.disabled && !startingUp) >+ textbox.focus(); Sadly this is unlikely to be safe in instantApply mode. >+ if (!elem.hasAttribute("preference")) >+ return false; I'd rather hope that we could be sure that this would never happen ;-) >- function Startup() { I'd prefer to keep Startup if possible. >+ oncommand="enableMarkMessagesReadAfter()" This doesn't belong here. See the paragraph I just added to the How To ;-)

(In reply to comment #3) > (From update of attachment 313927 [details] [diff] [review]) > >+ if (!textbox.disabled && !startingUp) > >+ textbox.focus(); > Sadly this is unlikely to be safe in instantApply mode. The code looks slightly different now, but it works in instantApply mode.

(In reply to comment #3) >(From update of attachment 313927 [details] [diff] [review]) >>+ if (!textbox.disabled && !startingUp) >>+ textbox.focus(); >Sadly this is unlikely to be safe in instantApply mode. Steps to reproduce problem: 1. Open about:config 2. If necessary, set instant apply to true 3. Set mailnews.mark_message_read.delay to false (and leave it selected) 4. Open Preferences and view message display preferences (do not close Preferences!) 5. Switch back to the about:config window and press Enter to toggle the preference Note that the focus is lost from about:config - if you switch back to Preferences you'll find that the focus went to the delay in seconds textbox (even if you selected a different panel but in that case it's harder to tell!)

This patch additionally checks, if the markMessagesRead checkbox is the focused element.

Comment on attachment 315315 [details] [diff] [review] Migration of the prefpane v3 Nice attachment number ;-) 315315 = 3 * 3 * 5 * 7 * 7 * 11 * 13 >+ <label value="&seconds.label;" >+ id="secondsLabel"/> <label id="secondsLabel" value="&seconds.label;"> on one line is fine.

Comment on attachment 315315 [details] [diff] [review] Migration of the prefpane v3 >+ let textbox = document.getElementById(aTextboxId); >+ let pref = document.getElementById("mailnews.mark_message_read.delay.interval"); I've just realised that this line is wrong. The old line was >- textbox.disabled = (!checkbox.checked || >- parent.hPrefWindow.getPrefIsLocked(textbox.getAttribute("prefstring"))); In other words you need to use textbox.getAttribute("preference") instead of the literal string.

Comment on attachment 315315 [details] [diff] [review] Migration of the prefpane v3 >Index: mailnews/base/prefs/resources/content/pref-viewing_messages.js >=================================================================== >+function Startup() >+{ >+ let value = document.getElementById("mailnews.mark_message_read.delay").value; >+ EnableTextbox("markMessagesReadAfter", value, false); >+} >+ >+function EnableMarkMessagesReadAfter(aValue) >+{ >+ let focus = (document.getElementById("markMessagesRead") == document.commandDispatcher.focusedElement); >+ EnableTextbox("markMessagesReadAfter", aValue, focus); >+} >+ >+function EnableTextbox(aTextboxId, aValue, aFocus) As you are passing the same value for aTextboxId from both calls, just drop that argument and use "markMessagesReadAfter" directly. >+{ >+ let textbox = document.getElementById(aTextboxId); >+ let pref = document.getElementById("mailnews.mark_message_read.delay.interval"); >+ let enable = aValue && !pref.locked; >+ >+ textbox.disabled = !enable; >+ >+ if (!textbox.disabled && aFocus) As you know what !textbox.disabled is already (enable) just use that instead. >+ textbox.focus(); >+}

Comment on attachment 315315 [details] [diff] [review] Migration of the prefpane v3 Basically okay, given that Neil's and Ian's issues are fixed, plus >Index: mailnews/base/prefs/resources/content/pref-viewing_messages.xul >=================================================================== > <?xml-stylesheet href="chrome://communicator/skin/" type="text/css"?> > <?xml-stylesheet href="chrome://messenger/skin/prefPanels.css" type="text/css"?> You don't need these stylesheets. >+ <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd"> %brandDTD; You don't need this dtd. Minusing for the number of changes.

(In reply to comment #8) > (From update of attachment 315315 [details] [diff] [review]) > Nice attachment number ;-) > 315315 = 3 * 3 * 5 * 7 * 7 * 11 * 13 Could we somehow reserve 431612 for the landing of all mailpref patches? ;-)

Patch has all review comments addressed. Note: relies on patch for bug 429667

Comment on attachment 316425 [details] [diff] [review] Migration of the prefpane v4 >Index: mailnews/base/prefs/resources/content/pref-viewing_messages.xul >=================================================================== >+<!DOCTYPE overlay [ >+ <!ENTITY % prefViewingMessagesDTD SYSTEM "chrome://messenger/locale/pref-viewing_messages.dtd"> %prefViewingMessagesDTD; >+]> This should be folded back into a single DOCTYPE line: <!DOCTYPE overlay SYSTEM "chrome://messenger/locale/pref-viewing_messages.dtd"> r=me with that.

Patch ready for checkin after bug 429667 landed.

Landed v5 on trunk.

Thx for landing -> RESOLVED FIXED