74731 - Cellmap's number of columns not updated after deleting a table cell

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P2)

Description

[Ralph Hartley]

Insert a 2x2 table.
Select a cell (optional).
Click the Table button to open the table cell porperties dialog.
Change "Cell Style" to "Header".
Click apply or OK.

Expected: The selected cell changes to header style and nothing else changes.

Observed: The cell changes to header style, AND a new, empty, column is added to
the table.

If you put text in the cells first, you can see that the new column is to the
right of the existing ones, regardless of which cell you change.

Comment 1

[rubydoo123]

assigning to cmanske

Comment 2

[Charles Manske]

The problem is that nsEditor::ReplaceContainer() is failing to
remove the node in nnsHTMLEditor::SwitchTableCellHeaderType().
What is supposed to happen is that a TH is replaced by a TD or vice versa.
Note that a call to "NormalizeTable()" is what is adding the lower-right cell
(assuming you changed the "cell type" in the first row). It now sees that
the first row has 3 cells, so it adds a 3rd cell to the 2nd row.
Joe: I traced through and didn't see any obvious problem. If you could spend
some debugging time, I'll be happy to help out as much as I can.

Comment 3

[Joe Francis]

I checked this out and ReplaceContainer() is working fine.  If you simply skip 
the NormalizeTable step everything is fine.  Something in NormalizeTable is 
getting confused by the <th>.  Perhaps the layout cellmap stuff you use is 
confused by a table cell changing to a th?

bouncing back to charlie for further investigation

Comment 4

[Ralph Hartley]

If it's any help, I also noticed that increasing the row or column span of any
cell has the same effect, a new column to the right of the old ones.

I assume that's the same bug, if not I'll enter a new one. In this case it is
clear that it is the "displaced" cell, that causes the problem. The cell that
previously occupied the space the cell expands into, and the rest of its row is
shifted to the right.

Comment 5

[Charles Manske]

Ralph: don't file a new bug yet. Joe's probably right -- Normalize table is
messing up and fixing that will probably fix all related problems.
Joe: Sorry I didn't figure that out myself!

Comment 6

[rubydoo123]

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

Comment 7

[Charles Manske]

I can't reproduce this any more! Can anyone else?

Comment 8

[Charles Manske]

Oops! "nevermind" I had "normalize" table removed in the JS. I see the bug.

Comment 9

[Charles Manske]

The problem is that after the insertion of a new TH element, and deletion of
the TD, the layout cellmap is not updated correctly. We use the method
nsITableLayout::GetTableSize(), implemented by nsTableFrame::GetTableSize(),
to get the number of rows and columns in the table, but we get back 3 columns
in this case when we should get 2.
Ralph: You mentioned that changing rowspan or colspan has the same problem; I
couldn't reproduce that, so can you supply an example?
Chris Karnaze: We shouldn't have to relayout the table when making DOM changes
in order to get proper cellmap data, do we?

Comment 10

[Ralph Hartley]

Insert a 2x2 table.
Click the Table button to open the table cell properties dialog.
Type "2" into the "Row Span" box.
Click OK.

Expected:
------------
|     |     |
|     |-----
|     |     |
------------
Observed:
------------------
|     |     |     |
|     |-----------
|     |     |     |
------------------

Comment 11

[Charles Manske]

Ok, that example is acting correctly, because "pushing" the first cell into the
second row via rowspan=2 causes the other 2 cells to be pushed to the right in
the layout cellmap. Thus "NormalizeTable()" now finds 3 columns and creats the
new cell in upper-right to keep table rectangular.
You should really never change the colspan or rowspan using the properties
dialog unless you realize these consequences. Users should select cells and
use "Join Selected Cells" on Table or popup context menu. This method is
smarter Your observation is good, though. Mayber we should only allow access
to rowspan and colspan in the Advanced Edit dialog? Maybe we should replace
them with a "Join Cells" button in the dialog? We certainly should
document what you found in the dialog help. CC'ing Robin for this issue.
Kathy, Beth: any opinions?

Comment 12

[Charles Manske]

Attachment: Patch to EdTableProps.js to simplify DOM insert/delete testing

Comment 13

[Charles Manske]

Use the attached patch to modify
mozilla/editor/ui/dialogs/content/EdTableProps.js
Test for the bug as described. The patch inserts a new "TH" node and removes the
current "TD" node via JS, so there's no editor influence. You will still see
the extra column added because of nsTableEditor::NormalizeTable().
If you break at the end of nsTableFrame::GetTableSize(),
you will see that aColCount 1 more than it should be.
Note that I can't do a JS-only test since nsITableLayout is not exposed to
JS (and neither is any other way to access the cellmap data, that I know of.)

Comment 14

[Charles Manske]

Modifying CC, priority, and severity.

Comment 15

[karnaze (gone)]

Moving to m0.9.1.

Comment 16

[karnaze (gone)]

Attachment: patch to fix the bug

Comment 17

[karnaze (gone)]

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

Comment 18

[Charles Manske]

So this fix wasn't checked in! Sorry to file another bug. I changed the Summary
to something that is easier to find in queries.

Comment 19

[Charles Manske]

Attachment: Patch to editor code to not hang when problem like this occurs.

Comment 20

[Charles Manske]

I still think the 5/10 patch should be applied to editor code (infinite loops
aren't nice!), but the main problem is addressed by patch on 4/22.

Comment 21

[Charles Manske]

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

Comment 22

[Charles Manske]

I applied and tested the 4/22 patch and it fixes all the symptoms in this bug as
well as bug 79462 and bug 79467. The fix looks good to me.
r=cmanske, though I'm not the best judge for subtle layout code issues.
I suggest the "sr" reviewer evaluate those issues.

Comment 23

[Charles Manske]

Attachment: Updated patch to nsTableFrame.cpp to return error in GetCellDataAt when actual rowspan or colspan is 0

Comment 24

[Charles Manske]

Kin had a very good suggestion that we should detect the error condition of
having a cell, but reporting incorrect rowspan or colspan.
Note that this patch (5/10/ 12:10) contains karnaze's changes to nsTableFrame.cpp

Comment 25

[Marc Attinasi]

This all looks fine to me. sr=attinasi

Comment 26

[Charles Manske]

Chris: Will you checkin my extra addition to nsTableFrame.cpp?

Comment 27

[karnaze (gone)]

Moving to m0.9.2

Comment 28

[karnaze (gone)]

The patches (attachments #2, #4) are in.

Comment 29

[sujay]

verified .