174389 - Finish removing "editorShell" dependencies in Messenger Composer

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Akkana Peck]

In mail compose, MsgComposeCommands.js in several places needs to set focus to
the content area, and currently uses the obsolete editor shell to do that.  AIM
will also need this.

Mail compose does this:

    window.editorShell.contentWindow.focus();

Comment 1

[Charles Manske]

I'm simply going to expand the scope of this bug to cover all the "remove 
editorshell" changes I'm working on in Mail (Messenger) Composer.
Replacing "editorShell.contentWindow" is just the simplest part!
Use "window._content"

Comment 2

[Charles Manske]

Attachment: Final removal of editorShell from Message Composer v1

Note that a new command observer is used in JS instead of the 
nsDocumentStateListener that was used in C++ code. Overall, the startup code of

Message Composer has been simplified.

Comment 3

[Charles Manske]

btw, I missed one instance of "editorShell" in mail code in this fix.
That issue is being addressed in bug 137255.

Comment 4

[Jean-Francois Ducarroz]

I applied the patch on top of a trunk build yesterday afternoo on Mac OS X and
now the compose window refuse to open, I have the following JS Errors:

JavaScript error: 
chrome://help/content/contextHelp.js line 1: redeclaration of const MOZ_HELP_URI

JavaScript error: 
chrome://editor/content/editorUtilities.js line 1: redeclaration of const XUL_NS

Do I need to apply a patch for editor as well?

Comment 5

[Charles Manske]

Yes! Sorry not to mention that here. Unfortunately there's lots of additional
patches that are required. Editor fixes are tracked in bug 121648.
I'll supply a build to test with.

Comment 6

[Jean-Francois Ducarroz]

I see only one problem: the bg/fg color control on the formatting toolbar does 
not show the current selection. On a brand new window, they always stay
transparent. On a recycled window, there are alway black over white.

Comment 7

[Charles Manske]

hmmm. The color feedback is busted in Web Composer as well, so that's probably
a separate problem from the mailnews patch. Investigating...

Comment 8

[Jean-Francois Ducarroz]

Comment on attachment 103475 [details] [diff] [review]
Final removal of editorShell from Message Composer v1

R=ducarroz

Comment 9

[(not reading, please use [e-mail] instead)]

Comment on attachment 103475 [details] [diff] [review]
Final removal of editorShell from Message Composer v1

sorry for the delay. sr=sspitzer

before you seek approval, can you elaborate on why this has to be in 1.2 final.
 (embedding reason?)

two comments / question / nits:

1)

why the new case statement?

+      case "cmd_smiley":

2)

+  if (!docShell)
+  {
+    NS_ASSERTION(0, "Failed to get docShell");
+    return NS_ERROR_UNEXPECTED;
+  }

Alternatively, i think you can do:

NS_ENSURE_TRUE(docShell, NS_ERROR_UNEXPECTED);

Comment 10

[Charles Manske]

Yes! This work is intended to be checked into 1.2 final. That is the important
deadline to finalize the editor embedding code (freeze API, work out basic 
issues.) Removing editorshell from all current editor users is essential to 
know that the embedding code by itself can support editing in various context.
The core bugs (tracked in bug 121648) give the details of this effort, which
includes elimintating access to editor and editorShell from nsIEditorBoxObject,
allowing and reliance on the new nsIEditingSessiong interface. Most embeddors 
should exclusively use command-based interaction with the editor, and but this
was deemed to be too disruptive to existing Mail and Web Composer. So while the 
changes to that code is still significant, it is actually a good compromise to
ensure continued stablility while providing platorms that excercise embedding-
critical editor interfaces.

Comment 11

[(not reading, please use [e-mail] instead)]

If there are various projects that will be based off the 1.2 branch, and those
projects require this fix, then drivers should support you.

thanks again for running through all the various compose tests.

Comment 12

[Charles Manske]

Concerning the problem Ducarroz noted in comment #6:
The primary problem has been solved in editor code by calling onFontColorChange()
and onBackgroundColorChange() in the "DocumentCreated" observer used by all
editors, but in the case of reusing a recycled window for mail, I'll need to add 
these lines inside the "if (recycled)..." startup block:
        if (gMsgCompose.composeHTML)
        {
          // Force color picker on toolbar to show document colors
          onFontColorChange();
          onBackgroundColorChange();
        }

Comment 13

[Charles Manske]

Re: Seth's comments in #11:
My thorough review of Mail Composer startup code revealed that "cmd_smiley" 
was missing from the defaultController's supportsCommand: function(command)
All the other supported commands were there, so I thought that was an obvious
thing to add.
The "NS_ENSURE_TRUE(docShell, NS_ERROR_UNEXPECTED);" sounds fine with me, I'll
use that instead.

Comment 14

[Jean-Francois Ducarroz]

fine with me with extra change in comment #12 for forcing toolbar colors. R=ducarroz

Comment 15

[Charles Manske]

We're not going to rush this into 1.2final

Comment 16

[Charles Manske]

checked into 1.3a trunk

Comment 17

[Charles Manske]

checked into 1.3a trunk

Comment 18

[Kathleen :Brade]

reopening
I still see "editorshell" in mailnews with lxr:

/mailnews/compose/build/win32.order, line 213 --
?CreateAndSendMessage@nsMsgComposeAndSend@@UAGIPAVnsIEditorShell@@PAVnsIMsgIdentity@@PAVnsIMsgCompFields@@HHHPAVnsIMsgDBHdr@@PBD4IPBUnsMsgAttachmentData@@PBUnsMsgAttachedFile@@PAXPAPAVnsIMsgSendListener@@I@Z
; 2

/mailnews/compose/build/win32.order, line 254 --
?SetEditor@nsMsgCompose@@UAGIPAVnsIEditorShell@@@Z ; 1

/mailnews/compose/build/win32.order, line 289 --
?ConvertAndLoadComposeWindow@nsMsgCompose@@QAEIPAVnsIEditorShell@@AAVnsString@@11HH@Z
; 1

/mailnews/compose/src/nsMsgCompose.cpp, line 491 -- // next stage of editorshell
removal.

/mailnews/compose/src/nsMsgCompose.cpp, line 1258 -- // Since editorShell
removal, it is called from JS after editor creation

Comment 19

[Charles Manske]

Attachment: Remove every last mention of editorshell

Comment 20

[Kathleen :Brade]

Comment on attachment 106139 [details] [diff] [review]
Remove every last mention of editorshell

   // Note!  enableEditableFields in gComposeRecyclingListener::onReopen
   // will redundantly set this flag to writable, but it gets there

Is that comment still valid?

r=brade

Comment 21

[Charles Manske]

Ok, this time I mean it. Remaining "editorShell" references removed.
Note that the Win32.order file isn't used any more.

Comment 22

[Charles Manske]

Re: brade's comment #20:
yes, the gComposeRecyclingListener::onReopen method will also reset the 
editor flag, but the enableEditableFields() does that as well as other things
and is called from other places besides "onReopen", so there's no point in 
changing any code to avoid the double setting of the editor flag.
The code in C++ is still needed to make the window editable before inserting
content into the editor window.