72415 - [PDT+]text is out from table's border

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Eugene Savitsky]

1. Load the URL and look at the 2nd news. The URL is way wider than the table
allows it.

moz 2001031204

Comment 1

[karnaze (gone)]

Attachment: test exhibiting <div align=justify> overflowing table

Comment 2

[karnaze (gone)]

Reassigning to attinasi and moving to m0.9.1.

In the attachment, the cell's block has a max element size of 7272, but when 
reflowed a 2nd time (resize reflow) can't accomodate an availableWidth of 9930.

Tbl 00DEA380 r=0 a=10665,UC c=0,0 cnt=6054
 Tbl 00E90DD8 r=0 a=10665,UC c=10050,UC cnt=6055
  RowG 00E90E40 r=0 a=UC,UC c=UC,UC cnt=6056
   Row 00E90E7C r=0 a=UC,UC c=UC,UC cnt=6057
    Cell 00E90EC4 r=0 a=UC,UC c=75000,UC cnt=6058
     Block 00E90F20 r=0 a=UC,UC c=UC,UC cnt=6059
     Block 00E90F20 d=16290,765 me=7275
    Cell 00E90EC4 d=16350,825 me=7335
   Row 00E90E7C d=UC,825
  RowG 00E90E40 d=UC,825
Initialized min=10080 des=10080 pref=10080
Balanced min=10080 des=10080 pref=10080 cols=9990
  RowG 00E90E40 r=2 a=10050,UC c=10050,UC cnt=6060
   Row 00E90E7C r=2 a=10050,UC c=10050,UC cnt=6061
    Cell 00E90EC4 r=2 a=9990,UC c=9930,UC cnt=6062
     Block 00E90F20 r=2 a=9930,UC c=9930,UC cnt=6063
     Block 00E90F20 d=16155,1050
    Cell 00E90EC4 d=16215,1110
   Row 00E90E7C d=10050,1110
  RowG 00E90E40 d=10050,1110
 Tbl 00E90DD8 d=10080,1200
Tbl 00DEA380 d=10080,1200

Comment 3

[Marc Attinasi]

Two issues, possibly related:
1) if I remove the align='justify' it is fine.
2) even with align, if I add some whitespace (newline) between the text and the
<a> it is fine

I think it is just a problem with whitespace processing (BWDIK).

Attaching a testcase that only differs from the previous one by whitespace
before the <a>

Comment 4

[Marc Attinasi]

Attachment: Testcase with whitespace between text and anchor: renders correctly

Comment 5

[Marc Attinasi]

--> P3

Comment 6

[Alexandru Savulov]

Attachment: modified sample html

Comment 7

[Alexandru Savulov]

This bug seems to have something to do with the nonstandard characters
(japaneese) contained in html before the anchor.
I created a sample file that demonstrates what I mean. 
(see attachment "modified sample html")
Chris Karnaze asked me to reassign this bug to [e-mail] 
- and CC to [e-mail] - however I'm not empowered sufficiently
to do this so I just added the comment.

Comment 8

[Marc Attinasi]

Reassigning and cc'ing for Alex. Thanks for the testcase and reduction!

Comment 9

[karnaze (gone)]

Moving to m0.9.2

Comment 10

[Marc Attinasi]

The URL and the testcases look fine on the 0530 build. Marking WORKSFORME

Comment 11

[Eugene Savitsky]

moz 2001053004 on win98. The bug is present.

Comment 12

[Marc Attinasi]

OK, sorry, I tested it on the Mac and it worked fine, but only because the 
characters are not handled correctly on the Mac. Accepting again, moving to 
Mozilla 0.9.3

Frank, do you have a replacement for Erik yet??? This would be going to Erik if 
he were here I think.

Comment 13

[Marc Attinasi]

Reassign to ftang. This is a text measurement issue I think, with intl
characters. Does Erik have a replacement yet?

Comment 14

[Frank Tang]

mark it as moz0.9.2. I can reproduce this on my window Nt. What does OS=other 
mean here?
Can we reproduce this on Linux?
Give to shanjian. I think we should fix this one for moz.0.92.

Comment 15

[Shanjian Li]

Attachment: a modified testcase with only one non-ascii

Comment 16

[Shanjian Li]

Attachment: remove the non-ascii, now it works fine.

Comment 17

[Shanjian Li]

Attachment: frame,content dump of the above two test case

Comment 18

[Shanjian Li]

I traced the problem for quite a while, and it looks more complicated than I was expected. 
I still could not get a clue why this happens. I modified the testcase and left only one 
non-ascii, and it has the problem. Without this non-ascii, everything works fine. And this 
problem seems only exist with western languages. If I choose japanese/chinese encoding for 
to load the test case, everything looks just fine. I dump the 2 testcase through viewer. 
One has problem and one does not, and their content model looks exact the same. The frame 
model does have some different, that's the position of inline frame (link). I guess this is 
a problem inside reflow layout. 

Marc, can you take a look?

Comment 19

[Shanjian Li]

Attachment: proposed fix

Comment 20

[Shanjian Li]

I finally pinned down the problem. Inside nsTextFrame::MeasureText, when we are doing 
lookahead text-run measurement, the existing buffer is not big enough. In that case,
only part of the characters are copied to the buffer, and we are still try to measuring 
it as a whole piece. Out-of-bound character caused the linebreaker to treat text as 
CJK text-run, this cause early line-break. 

Since this buffer is not used outside ComputeTotalWordWidth, and we can grow buffer 
inside ComputeTotalWordWidth, I allocated a new buffer. For performance reason, this 
buffer is only allocated if the old one is not enough. That's should keep the performance 
as good as old one. 

Comment 21

[Shanjian Li]

Chris, can you give r/sr for this one?

Comment 22

[Frank Tang]

pdt+ base on 6/11 pdt meeting.

Comment 23

[Marc Attinasi]

The patch looks good to me. I would put an assertion after the second call to
ComputeWordFragmentWidth to make sure that the return value is not negative - it
cannot be since you have computed (and allocated) the new buffer size, right?
[s]r=attinasi

Comment 24

[Roy Yokoyama]

I agree with marc for return check on the second call.

shanjian:
Your comment (2001-06-02 16:17) said that if you choose the
FarEast encoding, then the test attachements (id=36935 and id=36936) 
worked well.  How does the encoding selection affect the buffer size in 
your patch?  If the buffer size was the issue, then we should 
have seen the problem regardless of encoding.

Comment 25

[Chris Waterson]

Man, this code is spaghetti. r=waterson. Toss it up.

Comment 26

[Frank Tang]

roy, can you add the assertion according to marc's comment and attach a new 
patch here. ?

Comment 27

[Frank Tang]

shanjian is on vacation. reassign to yokoyama for check in. 
yokoyama, make sure you said "check in for [e-mail]" in the cvs 
check in log.

Comment 28

[Christopher Blizzard (:blizzard)]

a=blizzard on behalf of drivers for the trunk

Comment 29

[Roy Yokoyama]

Attachment: Adding Assertion if case the second ComputeWordFragmentWidth() fails

Comment 30

[Roy Yokoyama]

checked-in

Comment 31

[Chris Petersen]

Marking verified in the June 27th build (2001062708).