898321 - Joining table cells in the editor is horked

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

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

Steps to reproduce:

1. use for instance last nightly of Seamonkey
2. insert a 2x2 table into the document
3. select two adjacent table cells
4. use context menu to join the cells

EXPECTED RESULT: cells are joined, resulting cell is selected
ACTUAL RESULT: nothing happens, nsITableEditor.joinTableCells() throws an NS_ERROR_FAILURE

Comment 1

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

nsHTMLEditor::JoinTableCells() fails in
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsTableEditor.cpp#2063

Comment 2

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

Seamonkey rev 8a30e07815ff 14-dec-2012 works fine.
Seamonkey rev 18ff01530d1d 15-dec-2012 is horked.

glazou$ hg log --rev 8a30e07815ff:18ff01530d1d editor/libeditor/html/nsTableEditor.cpp 
changeset:   116042:3d898c39d05e
user:        Trevor Saunders <[e-mail]>
date:        Wed Aug 08 09:05:17 2012 -0400
summary:     bug 781409 - remove nsITableLayout r=roc,davidb

This is very likely the regression source. The fix for that bug got rid
of NS_EDITOR_ELEMENT_NOT_FOUND in nsHTMLEditor::GetCellDataAt() and that's
almost certainly the issue...

Comment 3

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

Attachment: 898321.patch

Absolutely trivial fix. Successfully tested in BlueGriffon:-)

Comment 4

[[e-mail]]

Comment on attachment 781654 [details] [diff] [review]
898321.patch

Actually I had failed to notice that NS_EDITOR_ELEMENT_NOT_FOUND is a success code, so technically you need to clear all your outparams too. (The change appears to work because all of the callers use an nsCOMPtr for the cell, which is still its default of nullptr when they come to check.)

Comment 5

[(no longer active)]

Comment on attachment 781654 [details] [diff] [review]
898321.patch

Review of attachment 781654 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with Neil's comment addressed.  Also, a test case would be really nice here, to make sure that we don't break it in the future.

Comment 6

[wefalck]

I am still on SeaMonkey version 2.16.1, as this was the last version in which the command 'join selected cells' still worked. Any idea when this issue might be fixed ?

BTW, as I don't understand the above techno-speak I am not sure whether it is supposed to contain a remedy ...

wefalck

Comment 7

[(no longer active)]

Daniel, is this patch ready to be checked in?  Have you posted it to the try server?

Comment 8

[Matt]

came to this bug via 962557.  Can someone confirm that it is only a merge involving the last row of the table that is affected.  Bug 962557 has no entry in the error console and merge only fails if the last row of the table is included in the merge.

Comment 9

[rsx11m]

Can someone drive this patch in? It doesn't look like Daniel is active on this any more...
This will hopefully fix bug 962557 in the process.

Comment 10

[Mikhail T.]

The trivial patch is and the "needinfo" are over two years-old! Let's see, if clearing it will get the attention of some _active_ developer...

Comment 11

[Mikhail T.]

Attachment: Updated version of the patch for comm-esr38 tree

Apparently, the libeditor-tree has been reorganized somewhat -- this version of the patch changes the path to the modified file.

I just rebuilt Thunderbird 38.2.0 with it and the cell-joining now works fine for me.

Comment 12

[wefalck]

I have just downloaded Seamonkey 2.35 for Mac and the joining of cells still does not work. I am on MacOS 10.7.5.

Comment 13

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Try run for patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e05ec34dc3af

I will check in the patch if it passes try.

Does anyone have a way to reproduce this in Firefox so I can write a test for Gecko?  Is this interface exposed to websites in any way?

(In reply to [e-mail] from comment #4)
> Actually I had failed to notice that NS_EDITOR_ELEMENT_NOT_FOUND is a
> success code, so technically you need to clear all your outparams too. (The
> change appears to work because all of the callers use an nsCOMPtr for the
> cell, which is still its default of nullptr when they come to check.)

This doesn't seem to be relevant, because the only outparam is aCell, which is already cleared, unless I'm missing something.  Do we really need to re-clear this?  I will anyway in the patch to avoid getting new review, just to be safe.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd48d3450594

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fd48d3450594