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

VERIFIED FIXED in mozilla0.9.2

Status

()

Core
Editor
P2
major
VERIFIED FIXED
18 years ago
17 years ago

People

(Reporter: Ralph Hartley, Assigned: karnaze (gone))

Tracking

({regression})

Trunk
mozilla0.9.2
x86
All
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

18 years ago
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

18 years ago
assigning to cmanske
Assignee: beppe → cmanske
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Priority: -- → P3
Target Milestone: --- → mozilla0.9

Comment 2

18 years ago
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.
Assignee: cmanske → jfrancis
Keywords: regression

Comment 3

18 years ago
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
Assignee: jfrancis → cmanske
(Reporter)

Comment 4

18 years ago
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

18 years ago
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!
Status: NEW → ASSIGNED

Comment 6

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

Comment 7

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

Comment 8

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

Comment 9

17 years ago
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?

(Reporter)

Comment 10

17 years ago
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

17 years ago
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

17 years ago
Created attachment 30791 [details] [diff] [review]
Patch to EdTableProps.js to simplify DOM insert/delete testing

Comment 13

17 years ago
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.)
Assignee: cmanske → karnaze
Status: ASSIGNED → NEW
Summary: Changing cell style adds column to table → Changing cell style adds column to table (nsTableFrame::GetTableSize is incorrect after insert TH, then delete TD)

Updated

17 years ago
Severity: normal → major
Priority: P3 → P2

Comment 14

17 years ago
Modifying CC, priority, and severity.
(Assignee)

Comment 15

17 years ago
Moving to m0.9.1.
Status: NEW → ASSIGNED
Target Milestone: mozilla0.9 → mozilla0.9.1
(Assignee)

Comment 16

17 years ago
Created attachment 31790 [details] [diff] [review]
patch to fix the bug
(Assignee)

Updated

17 years ago
Keywords: patch
(Assignee)

Comment 17

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

Comment 18

17 years ago
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.
Summary: Changing cell style adds column to table (nsTableFrame::GetTableSize is incorrect after insert TH, then delete TD) → Cellmap's number of columns not updated after deleting a table cell

Comment 19

17 years ago
Created attachment 33886 [details] [diff] [review]
Patch to editor code to not hang when problem like this occurs.

Comment 20

17 years ago
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

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

Comment 22

17 years ago
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.

Updated

17 years ago
Blocks: 79467

Updated

17 years ago
Blocks: 79462

Comment 23

17 years ago
Created attachment 33901 [details] [diff] [review]
Updated patch to nsTableFrame.cpp to return error in GetCellDataAt when actual rowspan or colspan is 0

Comment 24

17 years ago
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

17 years ago
This all looks fine to me. sr=attinasi

Comment 26

17 years ago
Chris: Will you checkin my extra addition to nsTableFrame.cpp?
(Assignee)

Comment 27

17 years ago
Moving to m0.9.2
Target Milestone: mozilla0.9.1 → mozilla0.9.2
(Assignee)

Comment 28

17 years ago
The patches (attachments #2, #4) are in.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 29

17 years ago
verified .
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.