110243 - trailing whitespaece removed twice

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Bernd]

see attached testcase, we base our calculations on min width but we get a
different desired width.

Comment 1

[Bernd]

Attachment: testcase

Comment 2

[Bernd]

Attachment: testcase for table cells

Comment 3

[Bernd]

This is a problem in the texttransformer which does not remove the final \n.
before the closing </td> tag. Furthermore it  creates some extra space used for
the alignment. Reassigning to Brian Stell as he seems to be the font man. 

Comment 4

[kill this account]

Roy: pls reassign

Comment 5

[Roy Yokoyama]

layout->shanjian

Comment 6

[Bernd]

the trailing whitespace is removed twice during the initial reflow and final
reflow (
http://lxr.mozilla.org/mozilla/source/layout/html/base/src/nsTextFrame.cpp#5307
). Changing summary. Adding Chris to CC because he touched last time this code
(from the log:   Bug 58384, 86279 . Be sure to trim letter spacing as well as
word spacing in TrimTrailingWhiteSpace(). Also, explicitly add letter spacing
where appropriate, instead of including it in word spacing. r=shanjian,
sr=attinasi.) This bug hurts tables pretty much as all the cells that define the
column width's in autolayout are also showing this behaviour.

Comment 7

[Shanjian Li]

Attachment: patch

Comment 8

[Shanjian Li]

explaination,

Each time TrimTrailingWhiteSpace is called, MeasureText is suppose to reset the 
width. That is perfectly fine in 1st call. In 2nd call to MeasureText, because 
we have maxWidth set, the trailing space width is not added. So we have to let 
TrimTrailingWhiteSpace know when this happen, and fortunately 
there is a flag (TEXT_TRIMMED_WS) we can use for this purpose. 

Comment 9

[Shanjian Li]

chris, could you review my patch? thanks!

Comment 10

[rbs]

crashes for me in the regression test0.html
[...]
  // Remove trailing whitespace if it was trimmed after reflow
  if (TEXT_TRIMMED_WS & mState) {
    NS_ASSERTION(aTextBuffer != nsnull,
      "Nonexistent text buffer should only occur during reflow, i.e. before 
whitespace is trimmed");
    if (--dstOffset >= 0) {
>>>>>>PRUnichar ch = aTextBuffer->mBuffer[dstOffset]; ++++++aTextBuffer is NULL
      if (XP_IS_SPACE(ch)) {
        textLength--;
        numSpaces--;
      }
    }
  }
[...]

Comment 11

[Shanjian Li]

rbs, where is test0.html?

Comment 12

[rbs]

Debug -> Viewer Demos -> #0 Basic Styles

Comment 13

[Shanjian Li]

Attachment: new patch

Comment 14

[Shanjian Li]

thanks for rbs for pointing out the problem. Because we can now set 
"TEXT_TRIMMED_WS" in MeasureText, we can't assume "aTextBuffer" is 
valid. 

I also checked all places where TEXT_TRIMMED_WS got referenced. One 
other places needs considersation is in MeasureText() as following. 
I added space-width back just before I clear the flag, and called 
MeasureText(). This should take care of this place. 
4797   // If we didn't actually measure any text, then make sure it looks
4798   // like we did
4799   if (!aTextData.mMeasureText) {
4800     aTextData.mAscent = mAscent;
4801     aTextData.mDescent = mRect.height - aTextData.mAscent;
4802     aTextData.mX = mRect.width;
4803     if (mState & TEXT_TRIMMED_WS) {
4804       // Add back in the width of a space since it was trimmed away last 
time
4805       // NOTE: Trailing whitespace includes word and letter spacing!
4806       aTextData.mX += aTs.mSpaceWidth + aTs.mWordSpacing + 
aTs.mLetterSpacing;
4807     }
4808   }

A note: we can in fact remove judgement in line 4803 since MeasureText is only 
called once. Such implication might not be clear to other people when they 
want to call MeasureText in other places in future. So I keep it there for the 
independency of MeasureText.

Comment 15

[rbs]

Are you happy with the block & table regression tests?

Comment 16

[Shanjian Li]

I tried all testcases in viewer and they all looked fine. Are there any other 
testcase I need to run against?

Comment 17

[karnaze (gone)]

There are a bunch of regression tests in layout/html/tests/block and 
layout/html/tests/table. Waterson has a link on how to run them, but I can't 
find it. From windows you run "rtest baseline" before the change and "rtest 
verify >myfile" after the change. If there are bbox differences then manually 
view the page in question. 

r=karnaze if you run the tests without problems.

Comment 18

[Shanjian Li]

I just finish running the test, it all looked fine except 
"html/tests/bugs/bug29157.html". I manually checked the result, and I 
could not see any difference. (I ran this test with the same binary against 
this testcase, and it still failed. That made me believe this is a false alarm.)

Comment 19

[Shanjian Li]

waterson, could you sr?

Comment 20

[rbs]

I am getting a lot of assertions in test0.html when resizing the window 
(line-layout is complaining that metrics.width < 0 on continuing text frames).

Comment 21

[Bernd]

I love the aussie humor: "Are you happy with the block & table regression
tests?", regression tests are a pain. 29157 is a well known fellow.

Comment 22

[Bernd]

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

Comment 23

[Bernd]

Attachment: wired testcase

I see the assertions too with the patch, and as visible form the testcase the
trimming sometimes work also with a two stage reflow.

Comment 24

[Shanjian Li]

Attachment: another patch

Comment 25

[Shanjian Li]

I proposed a new patch to address the assertion found by rbs. As I run block/table test
with this patch, I got tons of failed cases. I examined those cases one by one through 
comparing the appearance on 2 different computers, one with patch and one without. (It 
took me hours to finish this job, sigh...) Only one bug(17318) has real difference. Version
without my patch show a piece of text truncated by border. That is the problem we addressed
in the bug. 

(Why I did not get so many failed cases with my original patch was a mystery now. I think 
I must have made some mistake there. I tried to test my original patch again, and I got 
tons of failed cases too. I didn't examine those cases though.)

Comment 26

[Shanjian Li]

Please give r/sr (or challenging my new patch with problem). thanks!

Comment 27

[rbs]

Haven't tested the patch :-) Just a syntatic comment: what about making
aTrailingSpaceTrimmed a member of aTextData (which is meant to carry general
information about the text during text measurements).

Comment 28

[rbs]

The dreaded layout regressions are a paint... But rejoice, no further issues to 
report at this end. I have verified that it also fixes bug 92671 which I had 
suspected could be the same problem.
r=rbs (modulo the syntatic fix I suggested above).

Comment 29

[Shanjian Li]

thanks rbs, you suggestion in #27 will be adopted in my final patch for checkin. 

Comment 30

[Shanjian Li]

Attachment: update address rbs's #28 comment

Comment 31

[Shanjian Li]

waterson, could you sr?

Comment 32

[Marc Attinasi]

Comment on attachment 60572 [details] [diff] [review]
update address rbs's #28 comment

sr=attinasi

Comment 33

[Bernd]

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

Comment 34

[Bernd]

Shanjian this bug already collects dupes, it has  r and sr, so why this does not
go in?

Comment 35

[Shanjian Li]

It will get in soon after my DSL is up. Otherwise I could not reponse promptly 
to the possible regression. (Hopefully it will be today.) 

Comment 36

[Shanjian Li]

fix checked in. 

Comment 37

[rbs]

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

Comment 38

[Bernd]

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