830233 - Remove nsITableEditor

Status: RESOLVED WONTFIX

(Core :: DOM: Editor, defect)

Comment 1

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 2

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

(In reply to David Zbarsky (:dzbarsky) from comment #1)
> Created attachment 701685 [details] [diff] [review]
> Patch

Wait ! This patch seems to remove entirely all script access to nsITableEditor
methods since I see no change to nsIHTMLEditor. I *strongly* object to this; these
methods are key to the editor and you'll break *all* wysiwyg editors based on Gecko
(Thunderbird, BlueGriffon, BlueGriffon EPUB, SeamonKey) with respect to tables if
you do that.

If I am correct under that assumption, then that's a clear r- from me; all
methods defined by nsITableEditor *must* remain scriptable.

Comment 3

[David Zbarsky (:dzbarsky)]

Would you be happy if I move the methods to nsIHTMLEditor but change the signatures to use nsINode/Element instead of nsIDOMNode/nsIDOMElement?

Comment 4

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

As soon as all in nsITableEditor remains available from
script through nsIHTMLEditor or any other interface queryable from there
and using nodes or elements in JS, I'm fine with everything you want :-)

Comment 5

[(no longer active)]

(In reply to Daniel Glazman (:glazou) from comment #2)
> (In reply to David Zbarsky (:dzbarsky) from comment #1)
> > Created attachment 701685 [details] [diff] [review]
> > Patch
> 
> Wait ! This patch seems to remove entirely all script access to
> nsITableEditor
> methods since I see no change to nsIHTMLEditor. I *strongly* object to this;
> these
> methods are key to the editor and you'll break *all* wysiwyg editors based
> on Gecko
> (Thunderbird, BlueGriffon, BlueGriffon EPUB, SeamonKey) with respect to
> tables if
> you do that.

Does BlueGriffon use nsITableEditor?  If yes, which parts of it does BG use?

(In reply to David Zbarsky (:dzbarsky) from comment #3)
> Would you be happy if I move the methods to nsIHTMLEditor but change the
> signatures to use nsINode/Element instead of nsIDOMNode/nsIDOMElement?

nsINode/Element are not accessible from script.

Comment 6

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

(In reply to :Ehsan Akhgari from comment #5)

> Does BlueGriffon use nsITableEditor?  If yes, which parts of it does BG use?

Sure it does! Modification of the table (number of rows, columns), joining
and splitting cells, turning cells into header cells, and *much* more. The
"Insert a table" dialog, the "Table Properties" dialog and the
Tables Layout add-on entirely rely on it. The "Import table from textual
data" menu also relies on it.

> nsINode/Element are not accessible from script.

Then that's a strict no-go for me, sorry.

Comment 7

[(no longer active)]

Hmm, OK.  David, what was the original goal of your patch here?  Does it have something to do with the Web IDL bindings, or just cleanup?  If it's just cleanup, then perhaps you could modify the patch to move those methods to nsIHTMLEditor, but I'm not sure how much of an improvement that's going to be.  :/

Comment 8

[(no longer active)]

Comment on attachment 701685 [details] [diff] [review]
Patch

Clearing the request for now...

Comment 9

[David Zbarsky (:dzbarsky)]

(In reply to :Ehsan Akhgari from comment #7)
> Hmm, OK.  David, what was the original goal of your patch here?  Does it
> have something to do with the Web IDL bindings, or just cleanup?  If it's
> just cleanup, then perhaps you could modify the patch to move those methods
> to nsIHTMLEditor, but I'm not sure how much of an improvement that's going
> to be.  :/

Just trying to use less nsIDOM interfaces.  I guess we could rewrite editor to use WebIDL and then use this patch, but it's not worth it yet.

Comment 10

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

(In reply to David Zbarsky (:dzbarsky) from comment #9)

> Just trying to use less nsIDOM interfaces.  I guess we could rewrite editor
> to use WebIDL and then use this patch, but it's not worth it yet.

It could be interesting to ping the users of the editor (I am speaking of
embedders here, not front-end users) to know if the projected change is
meaningful _BEFORE_ investing time on code itself.

Seamonkey, Thunderbird, BlueGriffon, Postbox and a few more.

I think this bug should be closed as WONTFIX for the time being.

Comment 11

[(no longer active)]

(In reply to Daniel Glazman (:glazou) from comment #10)
> (In reply to David Zbarsky (:dzbarsky) from comment #9)
> 
> > Just trying to use less nsIDOM interfaces.  I guess we could rewrite editor
> > to use WebIDL and then use this patch, but it's not worth it yet.
> 
> It could be interesting to ping the users of the editor (I am speaking of
> embedders here, not front-end users) to know if the projected change is
> meaningful _BEFORE_ investing time on code itself.
> 
> Seamonkey, Thunderbird, BlueGriffon, Postbox and a few more.

SM and Thunderbird we can investigate by looking at comm-central.  For BG I'll always ask you.  For Postbox, I'm not sure if I have any contact that I can talk to for these kinds of changes.  (Please let me know if you do.)

> I think this bug should be closed as WONTFIX for the time being.

Yeah agreed that this is not worth fixing right now.