89116 - table reflow issues with entering text into columns/rows

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[sujay]

using 7/2 build of netscape

1) launch netscape
2) launch composer
3) insert 2 x 5 table
4) enter text in each of the 5 cells in the first row

two problems here:

a) as you enter text in the last two cells, notice how the
tables second cells are no longer directly below the first row
cells. This gets corrected when you enter text in the second
row cells. This is a table reflow problem. And this is an end
user frustration

b) right when you enter text in the last cell(cell 5), notice
how the text gets rendered outside the table. this is not
good.

Comment 1

[rubydoo123]

i think this needs to go over to karnaze, and will cc marc

Comment 2

[msanz]

I have seen this bug using NT and Mozilla/5.0 (Windows; U; WinNT4.0; en-US;
rv:0.9.2) Gecko/20010710 Netscape6/6.1. This is the only bug that has made me
use 4.77!!! Please, fix this for final.

Comment 3

[kmurray1115]

Agreed. This is a huge issue (and very disconcerting to the user).

Comment 4

[karnaze (gone)]

I'm not seeing the problem on Win2K 7/10 trunk or 7/11 branch. Marking wfm.

Comment 5

[sujay]

I definitely see it on 7/11 trunk and my bet is that I will see it on 7/11
branch.  very easy to reproduce:

1) launch netscape
2) launch composer
3) insert 4 x 4 table
4) enter text in each cell( make sure you enter enough text to go to the end of
the cell and beyond) <-----this triggers the problem.

I am 100% confident I can reproduce these two problems.

Comment 6

[rubydoo123]

to reproduce what the reported issue is -- try this:
1. open a new composer window
2. enter a table with 2 rows and 4 columns (where each column defaults to 25% of
the table width)
3. enter 5-7 characters in each cell in the first row, enter cahracters in the
first cell in the second row
4. go back to the first cell in the first row and enter enough characters to
push the cell width paste the initial cell width (enter 30 or so characters)

what happens: the entire table format shifts, vertical cell boundary markers get
incorrectly aligned and text from the fourth cell in the top row gets shifted
out of the table.

expect: that the content of the first cell wraps and the boundary markers stay
aligned and the text in the fourth column stays within the table.

Attaching screen grabs so you can see the problem

Comment 7

[rubydoo123]

Attachment: image 1

Comment 8

[rubydoo123]

Attachment: image 2

Comment 9

[rubydoo123]

Attachment: image 3

Comment 10

[Marc Attinasi]

FWIW, I can repro this too. Chris, visit me if you want some help causing the
bug to happen.

Comment 11

[karnaze (gone)]

cmanske or beppe,

Does editor use cols= to get equal width columns or does it put a width on a 
colgroup or all of the cols?

Comment 12

[Charles Manske]

No, we don't use "cols" or "colgroup" at all.

Comment 13

[chris hofmann]

karnaze is testing a possible fix. pdt+

Comment 14

[karnaze (gone)]

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

Comment 15

[karnaze (gone)]

Attachment: revised patch from bug 51727

Comment 16

[karnaze (gone)]

jrgm, if you have time could you test the performance impact of the patch.

The bug erronously caused table columns to be equal width until things broke. 
With the patch columns are recalculated when necessary and will likely not be 
equal width. If composer needs to support equal width columns like Nav4.x, then 
use proportional cols (or in real desparation, use the non standard cols 
attribute).

Comment 17

[Charles Manske]

I've tried the patch. I remember seeing this "dynamic" column resizing many
weeks ago, so I was surprised to see that before this fix, columns were equal 
width! The fixed behavior may be "W3C correct", but it is somewhat wacky! 
After typing some enough text in all of the cells in the first row such that 
each cell has text wrapped to a second line, typing in the second line of any
cell causes changes in all the column widths. It seems the column width 
calculations are based on total length of the cell text, ignoring the wrapping?
Obviously fixing the bug is important, so we can live with the new behavior.
We'll deal with better control of column widths in Composer in the next 
version.

Comment 18

[karnaze (gone)]

If the table has no constraints on columns or cells and there are no colspans
(which I think describes the default table in composer), then a column's width
will be proportional to the largest preferred width of the cells in the column.
So, the whacky behavior is expected.

Comment 19

[Marc Attinasi]

Have you gotten confinrmation on your suspicion of a possible performance
impact? Assuming that ther is no performance problem, sr=attinasi

Comment 20

[Charles Manske]

Subjective testing (insert a 20x20 table and type in it) definitely reveals a 
performance difference before and after this patch.

Comment 21

[John Morrison]

I'll try this out later today.

Comment 22

[karnaze (gone)]

I was more concerned about the performance impact on normal top 100 static pages 
than an N x M table in composer. The 20 x 20 case will suffer as long as column 
widths need to change. Actually, it wasn't that slow for me - but it took a long 
time just to create the table. If the N x M case ever becomes an issue, then we 
could consider optimizations like fixed layout tables (where the layout is 
determined by the column widths and/or 1st row cells) or equal width columns. 
 

Comment 23

[kmurray1115]

What do we think the perf impact will be on the target machine? I'll take the
perf hit over correctness in this particular case.

Comment 24

[Charles Manske]

Informal testing: Creating a table is slow but this patch doesn't make it 
worse (450mhz PII, debug build): 13sec for 30x30, 22sec for 40x40 tables.
The typing in those tables is much slower; type as fast as you can and count
the number of seconds it takes to catch up (to finish inserting all characters 
typed). It seems 2-4 times slower to catch up, and we're talking many ( > 8)
seconds.

Comment 25

[Alexandru Savulov]

r=Alex Savulov. patch is OK.

Comment 26

[Alexandru Savulov]

i didn't tested performance

Comment 27

[kmurray1115]

Sujay, can you file separate bugs on table building perf and typing within table
perf for a future milestone? thanks.

Comment 28

[Charles Manske]

I have bug 64403 on general table loading slowness. I've discussed issues with
many in editor group. There's probably other bugs already.

Comment 29

[John Morrison]

I pulled a trunk build for this afternoon, and ran the page-loader test, first
with the trunk build, then with the trunk build plus this short patch. There is
no difference in the times for any of the 40 test pages. 

Comment 30

[karnaze (gone)]

Kevin, I can't tell from your comments if you want the patch checked in or not. 

Charles, I'll spend some time trying to figure out why we are so much slower 
than 4.x in rebalancing a 40 x 40 table after each character is entered.

Comment 31

[kmurray1115]

It sounds like the patch doesn't actaully *degrade* perf - that we've got
pre-existing perf problems to deal with in separate bugs. That being the case,
I'm all for this patch going in!
Thanks for your quick work on this.
KM

Comment 32

[rubydoo123]

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

Comment 33

[karnaze (gone)]

The patch was checked in last week.

Comment 34

[sujay]

Is this bug fixed on both branch and trunk ?
can't tell from the comments.

Comment 35

[karnaze (gone)]

The patch was checked into the trunk and m0.9.2 branch.

Comment 36

[sujay]

verified on 7/16 branch on windows and mac....looks good...

need to verify on trunk now. then I can mark this verified-fixed.

Comment 37

[sujay]

verified on 7/17 trunk. looks good..

marking VERIFIED

however, there's a new issue when typing in the first cell of a 2 x 4
table. The cell starts expanding even though you haven't reached the
end of that cell. see how 4.x behaves. I am expecting the cell to only expand
once the text reaches the border and you continue typing. see new bug 91115

Comment 38

[Taras Tielkes]

Attachment: Testcase that demonstrates failing reflow

Comment 39

[Taras Tielkes]

I have attached a modified testcase from bug 51727.

The testcase shows that when a table cell is created with a content that exceeds
the cell width, the cell will not reflow when content of shorter length is inserted.

The other testcases from 51727 work, though.

Reopen?

Comment 40

[sujay]

Taras, can you file a new bug report?

Comment 41

[sujay]

REOPEN based on conversation with Marc...

see Taras' testcase below. Taras do not file another bug report.
we will reopen this one instead.

Comment 42

[Taras Tielkes]

Sujay, done:
Bug 91934

Comment 43

[Taras Tielkes]

Oops, didn'r read your second comment, Sujay.
I'll mark the new bug invalid. Apologies.

Comment 44

[sujay]

marking this one fixed...new bug is 91934..

Comment 45

[sujay]

verified.

Comment 46

[sujay]

REOPEN....there was some confusion..Taras is closing out the other

Comment 47

[Taras Tielkes]

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

Comment 48

[selmer (gone)]

Removing PDT+.  Testing with winNT and 7/20 branch build doesn't exhibit the
problem.

Comment 49

[sujay]

It still is a problem...please see the new test case that Taras
has attached below..

the original problem in this bug is fixed...there is another
case that is still broken and that is exhibited in Taras'
reduced test case. Marc Attinasi and I both saw it together on
his sytem this morning.

Comment 50

[karnaze (gone)]

Closing again. bug 91934 handles the new problem.

Comment 51

[sujay]

verified.

Comment 52

[Eric Pollmann]

See bug 92647, a regression caused by this patch.