Filing this since I've started to work on it...

Just attaching a wip patch. Needs some testing - I think it should work, though. This patch also removes the pref-publish panel and its .dtd file - not sure when the panel was used (has it ever been used?)- it's commented out in the overlay file. This version of the patch uses a broadcaster in order to disable/enable labels/buttons when customColors radio values are changed. It also improves the handling of locked prefs a bit. That said, I've spent some time thinking of what to do with locked "editor.*_color" prefs, but I haven't found any solution that seems reasonable.

OK, so I tested this (attachment #323735 [details] [diff] [review]) on mac with instantApply set to "false" in about:config, and everything seems to work. I still like to think a bit more on this, though.

Another question that probably should be discussed: Do we want to move everything in editor/ui to some other location? I can imagine that stuff that both seamonkey and thunderbird uses could go to one place (should be settled with thunderbird folks) and the seamonkey-specific pref files could go to another place (suite/somewhere).

Comment on attachment 323735 [details] [diff] [review] WIP patch >+ toggleElement("editorAuthor"); >+ toggleElement("useCustomColors"); >+ toggleElement("backgroundImageInput"); As IanN points out, I don't need those (and the function)

I started to look into how one should be able to honor the locked editor color prefs. I have a version that doesn't work 100% yet, I'll investigate more when I'm back (from vacation).

Hmm, I can't use the broadcaster if I want to honor locked editor.*_color" prefs...

Here's another WIP. This should work 100%. This one also uses new styling on the default attribute. MailNews not tested yet. Comments are welcome - not sure if my strategy is the best...

(In reply to comment #7) > MailNews not tested yet. Forget that, not sure why I thought that mattered...

+ for (var i = 0; i < buttons.length; i++) + { + let isLocked = isLocked(buttons[i].getAttribute("id")); Oops, this doesn't work (of course).

Some points of interest: 1) Removes pref-publish.xul and pref-publish.dtd 2) No more dependencies on editor js 3) One global var left 4) Honors locked prefs (even the editor.*_color ones). 5) In order to make the win/nix ui respond to browser.display* (etc) colour changes in about:config, "instantapply="true" is set on the browser colours preference elements. I'm not sure if this is considered hacky, but it works - for some reason I couldn't make it work without this. 6) In order to remove the color well colour (spacer bg), the editor version of setColorwell uses the "default" attribute and removes the style attribute. It seems to me that should be enough to use the "opacity: 0;" on 'default="true"' in toggleElements, executed before any colour is set. If a editor color pref is locked and you switch to use custom colors, colors will be set but they won't show up because of the opacity rule set in toggleElements. That said, I wonder if there's a better way of doing this than what is done in toggleElements (previousSibling etc)... 7) selectImageFile is without the hack in http://mxr.mozilla.org/seamonkey/source/editor/ui/dialogs/content/EdDialogCommon.js#210. It seems to work fine for me without it on mac (I'm not sure how to test it, but I haven't noticed anything), but I don't know about other OS. 8) We could get rid of the getColorAndUpdatePref function if we added a new color picker file for the composer prefs. I'm not 100% satisfied with my solution of passing id and types (looks ugly)...

Comment on attachment 327330 [details] [diff] [review] Another version Please see comment #10 for more info.

Comment on attachment 327330 [details] [diff] [review] Another version Eww, our editor default colours aren't the same as our browser default colours. That got me rather confused! >- // Setting to blank color will remove color from buttons, >- setColorWell("textCW", ""); >- setColorWell("linkCW", ""); >- setColorWell("activeCW", ""); >- setColorWell("visitedCW", ""); >- setColorWell("backgroundCW", ""); If you don't want to do that, then put !important on the CSS, not opacity. >+ if ((previewID == "normalText" || previewID == "ColorPreview")) ((Double vision)) >+ instantApply="true" This is no good, I managed to change my browser colours without hitting OK :-( [open preferences, open new page preferences, open appearance/colours, change]

Hmm, it also seems that using the onchange event on the same preference elements in different panels is not a god idea... Open preferences, open new page prefs, open appearance/colours and then change the "Use system colors" prefs and notice the little js warning from pref-colors.js ("Warning: Empty string passed to getElementById().") I can make that worse if I first load both panels (new page prefs first) and then change the pref in about:config: Error: document.getElementById(element.getAttribute("preference")) is null Source File: chrome://communicator/content/pref/pref-colors.js Line: 53

As Karsten points out, you can't really have the same id's in the 2 panels... I have to use nsIPrefBranch2 instead.

Some changes: - Now using browser pref observers in order to make the ui respond to about:config changes in browser colors - function names now starts with uppercase. Not sure if I like it, but it looks like it was the original style and it will make Mnyromyr happy - changed a few id's in the xul file. All id's except "ColorPreview" (which depends on css style rules) now begins with lowercase I'll have a -w version up in a few minutes

Comment on attachment 328536 [details] [diff] [review] Take 2, use browser pref observers etc +{ // add browser prefs observers Whoops, ignore that (should start with uppercase there)

Comment on attachment 328536 [details] [diff] [review] Take 2, use browser pref observers etc Did one of the versions update the editor preview if you updated the browser colours even in non-instant-apply mode, or have I been dreaming? >+const browserPrefsObserver = Nit: trailing space >+ switch(aData) Nit: space before ( >+ if(customColors) Nit: space before ( >+ let isLocked = CheckLocked(buttons[i].getAttribute("id")); Nit: .id >+ gPreviewBGColor = color; >+ UpdateBgPreview(document.getElementById("editor.default_background_image").value); UpdateBgPreview is the only user of gPreviewBGColor, so you should pass it in as a parameter rather than abusing a global. r+sr=me with that fixed. >+ document.getElementById(previewID).setAttribute("style","color: " + color + ";"); Nit: space after ,

If you're OK with it, I'd like to do what I did in bug 411226: 1) I land the re-named file 2) I then land #328536 (with updated paths and addressed comments), and at the same time I remove the old file 3) I post a "this is what I landed" patch

Comment on attachment 328567 [details] [diff] [review] re-name pref-composer.js to pref-editing.js I think you should a) land the patch using the wrong filenames b) wait for us to switch to hg c) create a new patch for the rename.

Ah, right - that makes sense.

(In reply to comment #18) > UpdateBgPreview is the only user of gPreviewBGColor You're right, it isn't, I didn't think to look in the XUL. Sorry.

Hmm, maybe I could split UpdateBgPreview somehow and get rid of the global...

Comment on attachment 328536 [details] [diff] [review] Take 2, use browser pref observers etc I'll attach a new patch once I've solved comment #23

- Due to usage of style.backgroundColor etc gPreviewBGColor is now gone (thanks to Neil who suggested it) - Parameter names are now prefixed with "a" - A few function names/comments have been changed, also added a few blank lines

Filed bug 444761 for the rename (comment #20)

Comment on attachment 329081 [details] [diff] [review] Take 3, without gPreviewBGColor >+ default: >+ SetBgAndFgColors(gPrefService.getBoolPref("browser.display.use_system_colors")) Nit: you don't actually need to call SetBgAndFgColors in the system colour case because they're already the correct colours. >+ colorPreview.style.backgroundImage = !aImage ? "" : "url(" + aImage + ")"; Nit: aImage && "url(" + aImage + ")";

(In reply to comment #27) >(From update of attachment 329081 [details] [diff] [review]) >>+ default: >>+ SetBgAndFgColors(gPrefService.getBoolPref("browser.display.use_system_colors")) >Nit: you don't actually need to call SetBgAndFgColors in the system colour case >because they're already the correct colours. You're right, you need this in case it was that preference that changed. >>+ colorPreview.style.backgroundImage = !aImage ? "" : "url(" + aImage + ")"; >Nit: aImage && "url(" + aImage + ")"; Instead of !aImage ? "" : "url(" + aImage + ")";

Landed (cvs trunk) with 'aImage && "url(" + aImage + ")";': Checking in editor/ui/jar.mn; /cvsroot/mozilla/editor/ui/jar.mn,v <-- jar.mn new revision: 1.24; previous revision: 1.23 done Checking in editor/ui/composer/content/editorPrefsOverlay.xul; /cvsroot/mozilla/editor/ui/composer/content/editorPrefsOverlay.xul,v <-- editorPrefsOverlay.xul new revision: 1.16; previous revision: 1.15 done Checking in editor/ui/composer/content/pref-composer.js; /cvsroot/mozilla/editor/ui/composer/content/pref-composer.js,v <-- pref-composer.js new revision: 1.18; previous revision: 1.17 done Checking in editor/ui/composer/content/pref-editing.xul; /cvsroot/mozilla/editor/ui/composer/content/pref-editing.xul,v <-- pref-editing.xul new revision: 1.41; previous revision: 1.40 done Removing editor/ui/composer/content/pref-publish.xul; /cvsroot/mozilla/editor/ui/composer/content/pref-publish.xul,v <-- pref-publish.xul new revision: delete; previous revision: 1.7 done Checking in editor/ui/locales/jar.mn; /cvsroot/mozilla/editor/ui/locales/jar.mn,v <-- jar.mn new revision: 1.8; previous revision: 1.7 done Removing editor/ui/locales/en-US/chrome/composer/pref-publish.dtd; /cvsroot/mozilla/editor/ui/locales/en-US/chrome/composer/pref-publish.dtd,v <-- pref-publish.dtd new revision: delete; previous revision: 1.2 done Checking in suite/themes/classic/editor/EditorDialog.css; /cvsroot/mozilla/suite/themes/classic/editor/EditorDialog.css,v <-- EditorDialog.css new revision: 1.28; previous revision: 1.27 done Checking in suite/themes/modern/editor/EditorDialog.css; /cvsroot/mozilla/suite/themes/modern/editor/EditorDialog.css,v <-- EditorDialog.css new revision: 1.42; previous revision: 1.41 done Note: I'll try and get this in to mozilla-central in the coming week.

reopening to ensure editor/ui lands in mozilla-central; adding checkin-needed per stefanh on IRC.

Comment on attachment 329081 [details] [diff] [review] Take 3, without gPreviewBGColor Adding to http://wiki.mozilla.org/Sheriff_Duty/PendingCheckins for sdwilsh. Checkin Comment: Bug 435079 - Migrate Composer's New Page Settings prefs to the new prefpane NPOTB (FF/XulRunner) r+sr=NeilAway, p=stefanh feel free to edit comment as you see best :-)

Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/index.cgi/rev/c32a70debffa

The two obsoleted files, pref-publish.xul and pref-publish.dtd, was not removed in the mozilla-central landing.

(In reply to comment #33) > The two obsoleted files, pref-publish.xul and pref-publish.dtd, was not removed > in the mozilla-central landing. > Thanks Hasse - this is now fixed (also fixed Neils last nit in comment #27): http://hg.mozilla.org/index.cgi/mozilla-central/rev/5f8f1247789f