trailing whitespaece removed twice

RESOLVED FIXED

Status

()

Core
Layout
RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: Bernd, Assigned: Shanjian Li)

Tracking

({testcase})

Trunk
x86
Windows 98
testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

17 years ago
see attached testcase, we base our calculations on min width but we get a
different desired width.
(Reporter)

Comment 1

17 years ago
Created attachment 57906 [details]
testcase
(Reporter)

Updated

17 years ago
Keywords: testcase
(Reporter)

Comment 2

17 years ago
Created attachment 58277 [details]
testcase for table cells
(Reporter)

Comment 3

17 years ago
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. 
Assignee: bernd.mielke → bstell
Component: HTMLTables → Layout
Summary: caption min width problem → texttransformer spoils min width calculation

Comment 4

17 years ago
Roy: pls reassign
Assignee: bstell → yokoyama

Comment 5

17 years ago
layout->shanjian
Assignee: yokoyama → shanjian
(Reporter)

Comment 6

17 years ago
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.
Summary: texttransformer spoils min width calculation → trailing whitespaece removed twice
(Assignee)

Comment 7

17 years ago
Created attachment 59624 [details] [diff] [review]
patch
(Assignee)

Comment 8

17 years ago
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. 
Status: NEW → ASSIGNED
(Assignee)

Comment 9

17 years ago
chris, could you review my patch? thanks!

Comment 10

17 years ago
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--;
      }
    }
  }
[...]
(Assignee)

Comment 11

17 years ago
rbs, where is test0.html?

Comment 12

17 years ago
Debug -> Viewer Demos -> #0 Basic Styles
(Assignee)

Comment 13

17 years ago
Created attachment 59769 [details] [diff] [review]
new patch
Attachment #59624 - Attachment is obsolete: true
(Assignee)

Comment 14

17 years ago
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.
(Assignee)

Updated

17 years ago
Whiteboard: ready for r/sr

Comment 15

17 years ago
Are you happy with the block & table regression tests?
(Assignee)

Comment 16

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

Comment 17

17 years ago
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.
(Assignee)

Comment 18

17 years ago
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.)
(Assignee)

Updated

17 years ago
Attachment #59769 - Flags: review+
(Assignee)

Comment 19

17 years ago
waterson, could you sr?

Comment 20

17 years ago
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).
(Reporter)

Comment 21

17 years ago
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.
(Reporter)

Comment 22

17 years ago
*** Bug 113012 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 23

17 years ago
Created attachment 60358 [details]
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.
(Assignee)

Comment 24

17 years ago
Created attachment 60518 [details] [diff] [review]
another patch
Attachment #59769 - Attachment is obsolete: true
(Assignee)

Comment 25

17 years ago
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.)
(Assignee)

Comment 26

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

Comment 27

17 years ago
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

17 years ago
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).
(Assignee)

Comment 29

17 years ago
thanks rbs, you suggestion in #27 will be adopted in my final patch for checkin. 
(Assignee)

Comment 30

17 years ago
Created attachment 60572 [details] [diff] [review]
update address rbs's #28 comment
Attachment #60518 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #60572 - Flags: review+
(Assignee)

Comment 31

17 years ago
waterson, could you sr?
Whiteboard: ready for r/sr → ready for sr

Comment 32

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

sr=attinasi
Attachment #60572 - Flags: superreview+
(Reporter)

Comment 33

17 years ago
*** Bug 115725 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 34

17 years ago
Shanjian this bug already collects dupes, it has  r and sr, so why this does not
go in?
Whiteboard: ready for sr
(Assignee)

Comment 35

17 years ago
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.) 
(Assignee)

Comment 36

17 years ago
fix checked in. 
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 37

17 years ago
*** Bug 92671 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 38

17 years ago
*** Bug 102196 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.