Can't join selected cells in same column in table

VERIFIED FIXED in mozilla0.9.3

Status

()

Core
Editor
P2
normal
VERIFIED FIXED
17 years ago
16 years ago

People

(Reporter: rohan.hart, Assigned: Charles Manske)

Tracking

({regression})

Trunk
mozilla0.9.3
regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed, nsBranch+, PDT+)

Attachments

(1 attachment)

(Reporter)

Description

17 years ago
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; IRIX 6.5 IP32; en-US; rv:0.9.1+) Gecko/20010612
BuildID:    2001061221

Joining selected cells instead joins one cell to the cell on its right. This
worked correctly in build 2001052521

Reproducible: Always
Steps to Reproduce:
1. Select several contiguous cells in a single column
2. bring up context menu or select table menu
3. select "join selected cells"

Actual Results:  One cell merges with that on its right

Expected Results:  Cells will merge with other cells

Comment 1

17 years ago
confirming; I see this on Macintosh so it isn't an SGI/Irix specific bug.
Reassign this to myself for now; cc cmanske
Assignee: beppe → brade
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
OS: IRIX → All
Hardware: SGI → All
Summary: Can't join selected cells in table → Can't join selected cells in same column in table

Updated

17 years ago
Target Milestone: --- → mozilla0.9.3

Updated

17 years ago
Assignee: brade → cmanske

Comment 2

17 years ago
this should go to cmanske, reassigning

Updated

17 years ago
Priority: -- → P2

Comment 3

17 years ago
joining should work in column and row selection

reviewed and approved
Keywords: nsBranch

Comment 4

17 years ago
No matter how you select the cell and how many cells you selected,
when "JOIN SELECTED CELL", only the first cell will join
with the cell on its right.
If you select several cell VERTICALLY, the first cell will
join the cell on its right, even though that cell is not selected.
(Assignee)

Comment 5

17 years ago
I'm working on this. Beth: Should we nominate for PDT+? Fix looks simple and I
think the cause influences many other table-editing operations.
Status: NEW → ASSIGNED
(Assignee)

Comment 6

17 years ago
Created attachment 41298 [details] [diff] [review]
Simplify nsHTMLEditor::GetFirstSelectedCellInTable to fix the bug
(Assignee)

Comment 7

17 years ago
Here's what was happening:
The method GetFirstSelectedCellInTable() was an old method written before we
automatically sorted the selected cell ranges as a user selected > 1 cells.
Thus it had logic to find the first selected cell in the table. It turns out
that the "Join Cells" command was the only method that used
"GetFirstSelectedCellInTable()", so we never noticed the
error for any other table editing methods, which used just
"GetFirstSelectedCell()" instead. GetFirstSelectedCellInTable() does the same
thing, but also returns the row and columns indexes of the cell found.
The problem was that GetFirstSelectedCellInTable() called
"GetNextSelectedCell()", which incremented the member variable
mSelectedCellIndex, and then JoinTableCells would also call
"GetNextSelectedCell()" but now mSelectedCellIndex was 1 more than it should
have been, and it wouldn't detect the second selected cell.
The solution to all this is to simplify "GetFirstSelectedCellInTable()" to
get the row / column indexes of the first cell found instead of having to
search through other selected cells; thus we don't need to call
"GetNextSelectedCell()"  at all.
Here's the complete new method, so it's easier to read than the diff:

NS_IMETHODIMP
nsHTMLEditor::GetFirstSelectedCellInTable(nsIDOMElement **aCell,
			PRInt32 *aRowIndex, PRInt32 *aColIndex)
{
  if (!aCell) return NS_ERROR_NULL_POINTER;
  *aCell = nsnull;
  if (aRowIndex)
    *aRowIndex = 0;
  if (aColIndex)
    *aColIndex = 0;

  nsCOMPtr<nsIDOMElement> cell;
  nsresult res = GetFirstSelectedCell(getter_AddRefs(cell), nsnull);
  if (NS_FAILED(res)) return res;
  if (!cell) return NS_EDITOR_ELEMENT_NOT_FOUND;

  *aCell = cell.get();
  NS_ADDREF(*aCell);

  // Also return the row and/or column if requested
  if (aRowIndex || aColIndex)
  {
    PRInt32 startRowIndex, startColIndex;
    res = GetCellIndexes(cell, startRowIndex, startColIndex);
    if(NS_FAILED(res)) return res;

    if (aRowIndex)
      *aRowIndex = startRowIndex;

    if (aColIndex)
      *aColIndex = startColIndex;
  }

  return res;
}

Keywords: patch, review
Whiteboard: FIX IN HAND, need r=, sr=, a=

Comment 8

17 years ago
r=mjudge

Comment 9

17 years ago
sr=sfraser
(Assignee)

Comment 10

17 years ago
Checked in.
Keywords: patch, review → vtrunk
Whiteboard: FIX IN HAND, need r=, sr=, a= → FIX IN HAND

Comment 11

17 years ago
tested on 7/6 trunk build -- works great
Whiteboard: FIX IN HAND → fixed, nsBranch+

Comment 12

17 years ago
0706 Work in Win2k also.
Status might change to FIXED?

Comment 13

17 years ago
per conversation with selmer, adding PDT+ and removed vtrunk
Keywords: vtrunk
Whiteboard: fixed, nsBranch+ → fixed, nsBranch+, PDT+
(Assignee)

Comment 14

17 years ago
Checked into trunk and branch.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 15

17 years ago
verified in 7/10 branch build.

will verify oin trunk also. Then I will mark verified-fixed.

Comment 16

17 years ago
verified in branch and trunk builds 7/10.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.