78400 - Deleting row(s), doesn't delete all selected rows

Status: VERIFIED WORKSFORME

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

Description

[scalkins]

See this on
Win 2001-04-30-06Mtrunk
Linux 2001-05-01-08Mtrunk
Mac 2001-05-01-04Mtrunk

Steps to repro:
1)Open a new compose page, Create a table of 2 columns and 5 rows.
2) Click and drag down from 2nd row to 4th row to select these rows.
3)Select the menu option Table-->Delete-->Row(s)

Actual results: Only the first row gets deleted. The rest of the selected rows
remain intact.

Nominating nsbeta1 for implied functionality not present.
We should either change the wording from
Table-->Delete-->Row(s)
to
Table-->Delete-->Row
or make this functionality actually available to users.

Comment 1

[rubydoo123]

that's odd, the delete will only remove 1 row at a time, confirmed on win98 
using the build from 2-May, this should be fixed for moz0.9.1

Comment 2

[Charles Manske]

I know about this. Need to count number of rows or columns selected and use that
number to delete them.
The complication is: What if user has non-contiguous selection? I'm inclined to
keep it simple and only delete the first contiguous block of selected rows/cols.
We can get fancier in the next version.

Comment 3

[David Park]

I don't think you should be able to select non-contiguous rows.  It is very
useful to be able to delete multiple rows selected.

Comment 4

[Charles Manske]

Allowing deletion of rows won't interfere with being able to select
non-contiguous rows. It will just igore rows after the first contiguous block.
The table selection code is working and very "delicate", allowing drag-selection
of contiguous blocks as well as non-contiguous blocks using Ctrl+Shift+click.
I don't want to touch that code any more.

Comment 5

[Charles Manske]

Attachment: Fix

Comment 6

[Charles Manske]

With the 5/10 patch, ALL rows that have one or more selected cell will be
deleted, whether they are contiguous or not. Also fixes similar problem for
deleting columns: all columns containing one or more deleted cells are deleted
when "Delete -> Column" is used.

Comment 7

[Kathleen :Brade]

Can we delete all of the rows/columns that have selection (even if they are 
discontiguous)?  What if we just deleted each row/column as we encountered a 
selected cell?  Would that be too much of a performance hit?

Comment 8

[Charles Manske]

I don't quite understance you question. The fix results in every column (or row)
containing at least one selected cell being deleted, whether discontiguous or not.

Comment 9

[Charles Manske]

I optimized for performance by utilizing the multiple-number capabilities that
nsTableEditor::DeleteTable[Row | Column] provides. Thus the new
"GetNumberOfContiguous..." methods in editor.js. So the new command loops through
the selected cells and deletes in larger (contiguous)chunks.

Comment 10

[Charles Manske]

r=mjudge

Comment 11

[Simon Fraser [no longer active]]

Loops would be more clearly written as:

var rows = GetNumberOfContiguousSelectedRows();
if (!rows)
  rows = 1;

while (rows)
{
  window.editorShell.DeleteTableRow(rows);
  rows = GetNumberOfContiguousSelectedRows();
}

Should try/catch around here. Maybe add batching in the command.

Comment 12

[Charles Manske]

Attachment: Updated patch with Simon's suggestions

Comment 13

[Simon Fraser [no longer active]]

Why not Begin/EndBatchChanges outside of the try/catch?

Comment 14

[Charles Manske]

Checked in.

Comment 15

[Kathleen :Brade]

Something strange happens when I undo/redo this.  The data is getting corrupted. 
Reopening bug/removing target milestone.

Comment 16

[Kathleen :Brade]

Maybe I'm just seeing layout/rendering bug?
Could someone else test this?

Comment 17

[scalkins]

Using the original test case, I still see the same behavior too.
Win32 2001-05-23-06trunk
(classic on Nt)

Comment 18

[Charles Manske]

It is the display redraw problem, not my code.

Comment 19

[rubydoo123]

no, this is still there, I followed the original steps and only 1 row gets 
deleted

Comment 20

[Charles Manske]

Wait a minute: The original description of "Click and drag down from 2nd row to
4th row to select these rows" doesn't select rows, it selects *cell contents
across cell boundaries*. This is not how you select rows and cells. You use
Accel+Click to selecte cells, Accel+Click + drag to select multiple cells, or
Shift+Accel+click then drag to select rows or columns.
Thus in the described way of selecting, table selection only looks at the
"anchor node" of the selection (the cell where drag-selecting began).
If we want Delete Rows or Delete Columns to interpret what to delete from
selected *text* (not complete rows or columns), then that is a future
enhancement. I think we should file a new bug on that issue, and leave this one
to cover the issue of deleting multiple rows/columns when *cells* are selected.
If it is deemed important enough, this request might be possible in 0.9.2
timeframe.
Sorry to not clarify that issue in previous comments to this bug.
Resolving this bug as "worksforme"

Comment 21

[Charles Manske]

Bug id=82651 has been filed for the issue of using text selection to tell
what cells to use with table commands.

Comment 22

[sujay]

verified.