133598 - <editor> tag in XUL should support creation of different editors with "editortype" attribute

Status: VERIFIED FIXED

(SeaMonkey :: Composer, defect)

Description

[Simon Fraser [no longer active]]

Currently, it's not possible to just put an <editor> tag in your XUL, and have
it automatically work. You have to have a bunch of JS as well that sets
everything up.

With the editor embedding work, we now have the power to make <editor> do the
right thing. With some minor XBL hacking, we can get <editor> to automagically
set up an editor.

Comment 1

[Simon Fraser [no longer active]]

I think <editor> is an important enough XUL feature that this should work for 1.0.

Comment 2

[Simon Fraser [no longer active]]

Attachment: <eidtor> XBL goodness

First cut at making <editor> work automagically. Note that you should not check
this in without fixing existing <editor> users, and changing the editor
instantiation and ownership by nsEditorShell.

Comment 3

[Kathleen :Brade]

Comment on attachment 76353 [details] [diff] [review]
<eidtor> XBL goodness

In addition to the comments Simon makes above regarding checking in this patch,
please also remove the tabs.

note:  I am setting the needs-work flag because the patch is incomplete.

Comment 4

[selmer (gone)]

Please update this bug with an [adt1] - [adt3] impact rating (or take it off the
list if it doesn't even rate adt3.)  Thanks!

Comment 5

[Syd Logan]

--> over to simon

Comment 6

[Simon Fraser [no longer active]]

Pre-sabbatical bug triage. ->saari, for triage.

Comment 7

[saari (gone)]

->mjudge

Comment 8

[mjudge]

leaving assigned to me. XBL work is needed here.  I will try to find sfrasiers 
hack he did before to get this to work.

Comment 9

[Simon Fraser [no longer active]]

My hack is attached to this very bug.

Comment 10

[Charles Manske]

I experimented with Simon's XBL code by simply replacing existing "editor" 
binding code in general.xml just to prove the concept and it seems to work fine
for getting all the attributes, and getting the nsIEditor/nsIHTMLEditor from 
the <editor> element.
Of course this isn't a good thing to do until all the editorShell stuff is 
converted and the init code is worked out; e.g., with the new code,
nsIWebProgressListener and command controllers are installed twice.
So would I be correct in assuming that all the initialization that is in 
editorShell that isn't already being done in nsEditorSession needs to be moved
into the latter?

Comment 11

[Charles Manske]

Seems like I should take this bug now.

Comment 12

[Simon Fraser [no longer active]]

> So would I be correct in assuming that all the initialization that is in 
> editorShell that isn't already being done in nsEditorSession needs to be moved
> into the latter?

I'm not sure about 'all'. You'll see that nsEditingSession already does a lot of
stuff: makes controllers, registers load listeners etc.

Be careful about moving nsEditorShell code wholesale into nsEditingSession; that
might not be its rightful new home (talk to akkana about nsEditorShell issues). 
Also be sure to keep the stuff in nsEditingSession generic enough that it's not
tied too closely to any one editor type. The plan is that nsEditingSession
should be able to manage different types of editors (say, frameset/html/xml/svg
or whatever). So it's not the job of the nsEditingSession to say whether
specific content can be edited; you'll have to push that burden onto the editors
themselves.

Kin suggested that we register editors with progIDs that say what they can
handle by MIME type (e.g. "@mozilla.org/editor/text/html",
"@mozilla.org/editor/text/plain") or by combination of MIME type and certain
tags ("@mozilla.org/editor/text/html-frameset") or something. However, this
pushes the tag sniffing onto the nsEditingSession; somehow, it still needs to be
told what tags to sniff for.

Comment 13

[Charles Manske]

*** Bug 79895 has been marked as a duplicate of this bug. ***

Comment 14

[Charles Manske]

Attachment: New editor bindings with support for editortype attribute

Comment 15

[Kathleen :Brade]

Comment on attachment 102860 [details] [diff] [review]
New editor bindings with support for editortype attribute

it looks like there are tabs in here; also seems to be inconsistent whitespace
(blank lines) between some of the properties

It seems like this can't go in early since you are removing the editorshell
property.  This needs to be committed with other patches.

Comment 16

[Charles Manske]

Attachment: New editor bindings with support for editortype attribute v2

Changed previous XBL to autostart editor only when "editortype" is supplied.

Comment 17

[[e-mail]]

Comment on attachment 102938 [details] [diff] [review]
New editor bindings with support for editortype attribute v2

>+              if (this.editorType)
editorType or editortype? (attribute is and should be lower case)

>+                makeEditable(true);
Should be this.makeEditable

>+      <method name="makeEditable">
>+	      <parameter name="waitForUrlLoad"/>
>+        <body>
No tabs please! (applies to other examples).

>+        <![CDATA[
>+		      try
>+		      {
>+			      this.editingSession.makeWindowEditable(this.contentWindow, this.editortype, waitForUrlLoad);
>+		      }
>+		      catch(ex)
>+		      {
>+			      dump("makeEditable got exception: " + ex + "\n");
>+		      }
Do you really want try/catch here (and other examples)?
Caller might prefer to catch exceptions.

>+      <method name="getEditor">
>+        <parameter name="containingWindow"/>
Isn't this always going to be the content window (and other example)?

>+      <property name="editable"
>+                onget="return this.getAttribute('editable');"
>+                onset="this.setAttribute('editable',val); return val;"/>
What does this do? Is this watched by the editor?

>+      <property name="editortype"
>+                onget="return this.getAttribute('editortype');"
>+                onset="this.setAttribute('editortype',val); return val;"/>
Almost the same here, although I see you using the getter.

Comment 18

[Charles Manske]

Neil: oops :(, wrong patch.
Yes, the "editortype" is all lower case.
And "editable" is not supposed to be there (earlier experiment).
Question about try/catch is a good one.

Comment 19

[Charles Manske]

Attachment: New editor bindings with support for editortype attribute v3

This includes removing editorShell from nsEditorBoxObject, since it is no
longer
needed.
Errors spotted by Neil are correct, but I left in the try/catch pending
opinions
from other reviewers.

Comment 20

[Charles Manske]

Attachment: New editor bindings with support for editortype attribute v3

Removing nsIEditorBox will be done in another bug.
I think I finally removed all the tabs!

Comment 21

[Akkana Peck]

I agree with Neil's suggestion of throwing exceptions and letting the caller
handle error conditions -- assuming that this value can get back to C++ as a
return value and not require C++ exception handling.  I'm not clear whether it
works that way or not.  If we can't do that, we do need some way of
communicating an error to the caller -- dump(e) alone isn't enough.  Charley and
I discussed catching the error and returning true or false; if specific
exception info doesn't get returned, then maybe that's all we can do.  Need more
info.

You should probably remove the dump() calls.

+              if (this.editortype)
+                makeEditable(true);

What about Neil's suggestion of this.makeEditable?  Does it matter?

Comment 22

[Akkana Peck]

From further discussion on #editor I realized that we need to make the question
really clear:
When calling these interfaces from C++, if an exception is thrown, will it
(a) crash
(b) return the fact that an exception was thrown, but no further info
(c) return info about which exception was thrown
?  If (a) or (b), then Charley should catch exceptions, and return true or false
to indicate error or success.  If (c), then we should let the exception be
thrown and let the caller handle it.

Comment 23

[Simon Fraser [no longer active]]

When calling these interfaces from C++, if an exception is thrown, will it
(a) crash
(b) return the fact that an exception was thrown, but no further info
(c) return info about which exception was thrown

JS exceptions != C++ exceptions

JS exceptions get thrown when XPCOM interface calls return errors. Our C++
never, ever throws exceptions; all the C++ error handling is simply done via
nsresult return values.

As for returning errors from the XBL, JS exceptions are enough. XBL
methods/properties are not callable/accessible from C++.

Comment 24

[Charles Manske]

Simon: I agree. Can you SR this, please?
Now that Simon has confirmed that the try/catch in XBL code is a good thing,
can someone please confirm with an r=?

Comment 25

[Simon Fraser [no longer active]]

> Now that Simon has confirmed that the try/catch in XBL code is a good thing,
> can someone please confirm with an r=?

Why would you not want to let exceptions propagate out of the XBL?

Comment 26

[Charles Manske]

Ok, I'll drop the try/catch in getEditor and getHTMLEditor

Comment 27

[Akkana Peck]

Comment on attachment 103221 [details] [diff] [review]
New editor bindings with support for editortype attribute v3

r=akkana with these fixes:
1. this.makeEditable (probably doesn't make a difference but apparently it's
the standard pattern in xbl).
2. Remove the try/catch.
3. Remove the various dump()s.

Comment 28

[Simon Fraser [no longer active]]

Comment on attachment 103221 [details] [diff] [review]
New editor bindings with support for editortype attribute v3

Address akkana's issues, and sr=sfraser

Comment 29

[Charles Manske]

Attachment: New editor bindings with support for editortype attribute v4

This has the changes requested by Akkana.
It also includes changes to nsIEditorBoxObject to remove both editor and 
editorShell references. After further discussion, we decided we need to keep
this
object around for awhile, so we should these changes now.

Comment 30

[Akkana Peck]

Comment on attachment 103752 [details] [diff] [review]
New editor bindings with support for editortype attribute v4

>-      <property name="editor"

Wow, we don't need to get the nsIEditor from the box object any more either? 
We're sure that no clients will need it?  Cool, if so.

It looks like Init now looks like this:

>@@ -98,30 +82,6 @@
> {
>   nsresult rv = nsBoxObject::Init(aContent, aPresShell);
>   if (NS_FAILED(rv)) return rv;
>   return NS_OK;
> }
> 

If I'm reading that right, can you change that to
{
  return nsBoxObject::Init(aContent, aPresShell);
}
?

Fix that, and r=akkana.

Comment 31

[Charles Manske]

doh! Sure is simpler! change to:
  return nsBoxObject::Init(aContent, aPresShell);
has been made.

Comment 32

[Simon Fraser [no longer active]]

Comment on attachment 103752 [details] [diff] [review]
New editor bindings with support for editortype attribute v4

sr=sfraser

Comment 33

[Charles Manske]

checked into 1.3a trunk

Comment 34

[sairuh (rarely reading bugmail)]

rs vrfy.