Closed Bug 133598 Opened 23 years ago Closed 22 years ago

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

Categories

(SeaMonkey :: Composer, defect)

Product:
SeaMonkey
Component:
Composer
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla1.2beta

People

(Reporter: sfraser_bugs, Assigned: cmanske)

References

Details

Attachments

(1 file, 5 obsolete files)

New editor bindings with support for editortype attribute v4
(deleted), patch
akkzilla
: review+
sfraser_bugs
: superreview+
Details | Diff | Splinter Review
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.
I think <editor> is an important enough XUL feature that this should work for 1.0.
Target Milestone: --- → mozilla1.0
Attached patch <eidtor> XBL goodness (obsolete) (deleted) — — Splinter Review
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 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.
Attachment #76353 - Flags: needs-work+
Please update this bug with an [adt1] - [adt3] impact rating (or take it off the list if it doesn't even rate adt3.) Thanks!
--> over to simon
Assignee: syd → sfraser
Pre-sabbatical bug triage. ->saari, for triage.
Assignee: sfraser → saari
->mjudge
Assignee: saari → mjudge
Target Milestone: mozilla1.0 → mozilla1.2alpha
Blocks: edembed
leaving assigned to me. XBL work is needed here. I will try to find sfrasiers hack he did before to get this to work.
Status: NEW → ASSIGNED
My hack is attached to this very bug.
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?
Seems like I should take this bug now.
Assignee: mjudge → cmanske
Status: ASSIGNED → NEW
Keywords: nsbeta1
> 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.
Status: NEW → ASSIGNED
Keywords: nsbeta1 → nsbeta1+
*** Bug 79895 has been marked as a duplicate of this bug. ***
Summary: <editor> tag in XUL should just work → <editor> tag in XUL should support creation of different editors with "editortype" attribute
Attachment #76353 - Attachment is obsolete: true
Whiteboard: [FIX IN HAND] need r=,sr=
Target Milestone: mozilla1.2alpha → mozilla1.2beta
Blocks: editorshell
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.
Changed previous XBL to autostart editor only when "editortype" is supplied.
Attachment #102860 - Attachment is obsolete: true
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.
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.
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.
Attachment #102938 - Attachment is obsolete: true
Removing nsIEditorBox will be done in another bug. I think I finally removed all the tabs!
Attachment #103060 - Attachment is obsolete: true
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?
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.
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++.
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=?
> 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?
Ok, I'll drop the try/catch in getEditor and getHTMLEditor
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.
Attachment #103221 - Flags: review+
Comment on attachment 103221 [details] [diff] [review] New editor bindings with support for editortype attribute v3 Address akkana's issues, and sr=sfraser
Attachment #103221 - Flags: superreview+
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.
Attachment #103221 - Attachment is obsolete: true
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.
Attachment #103752 - Flags: review+
doh! Sure is simpler! change to: return nsBoxObject::Init(aContent, aPresShell); has been made.
Whiteboard: [FIX IN HAND] need r=,sr= → [FIX IN HAND] need sr=
Comment on attachment 103752 [details] [diff] [review] New editor bindings with support for editortype attribute v4 sr=sfraser
Attachment #103752 - Flags: superreview+
Whiteboard: [FIX IN HAND] need sr= → [FIX IN HAND] need a=
checked into 1.3a trunk
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Whiteboard: [FIX IN HAND] need a=
rs vrfy.
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: