too much space between lines: whitespace must collapse inwards

RESOLVED FIXED in mozilla1.3beta

Status

()

Core
Layout
P2
major
RESOLVED FIXED
16 years ago
7 years ago

People

(Reporter: Benjamin Streeck, Assigned: dbaron)

Tracking

({regression, testcase})

Trunk
mozilla1.3beta
regression, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch])

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

16 years ago
Gecko in Version 1.1alpha behaves way differently (WRONG in my opinion) from
1.0. Things seem to be stretched longer so that five lines of text take up more
vertical space than before.

Really broken: code like
   Normal text<br>
   <small>Line1<br>
   Line2<small><br>
   Normal text again.
renders in such a way that 'Line2' has the same height as 'Normal text', which
looks way messed up!

(I'm presuming this will be a duplicate, but I couldn't find one, and in my
opinion this is a MAJOR MAJOR bug, and a huge step back from version 1.0 -- I
took 1.1alpha off my system because of this one bug!)

Thx and keep up the great work!

Comment 1

16 years ago
Could you give the buildIDs of both versions?
(Reporter)

Comment 2

16 years ago
The 1.0 Version is the official 1.0 release: 2002053012
I've been tracking this with most of the nightly build up till now, the latest
one that showed this behavior was the one from yesterday: 20020701
Created attachment 89902 [details]
testcase described by reporter

This is the testcase you gave, but it works fine for me.  Could you attach a
testcase that shows the problem?  (Or do you see the problem in this one?)
(Reporter)

Comment 4

16 years ago
Created attachment 89904 [details]
Testcase

This is the correct testcase. I admit it varies somewhat from the above one,
this is part of the code I noticed it on, majorly reduced.
(Reporter)

Comment 5

16 years ago
What is even stranger: on my homepage I have a pop-up window informing of my
love to Mozilla. The content of that window fits perfectly in IE5.5 and Mozilla
1.0. Now using 1.1alpha it is stretched to look unpleasent.

http://www.streeck.com/

Comment 6

16 years ago
WFM 1.1a+ ID: 2002062321 & 2002070204 on Linux
Created attachment 89969 [details]
simplified testcase
Attachment #89902 - Attachment is obsolete: true

Updated

16 years ago
Keywords: testcase

Comment 8

16 years ago
If you put <BR> after your last line, you will get same line height. But this is
only workaround. 

Comment 9

16 years ago
occurs on linux
might be annoying, but no hang/crash, so not critical

applying attachment 77116 [details] [diff] [review] from bug 134580 causes this behavior to occur on the
branch.

marking NEW
Severity: critical → major
Status: UNCONFIRMED → NEW
Depends on: 134580
Ever confirmed: true
Keywords: regression
OS: Windows 2000 → All
Summary: 1.1alpha-layout is strange, different from correct 1.0-layout → too much space between lines
Comment on attachment 89969 [details]
simplified testcase

This testcase is too simplified, since IE for Windows behaves the same way we
do on it.
Attachment #89969 - Attachment is obsolete: true
Created attachment 90395 [details]
simplified testcase

This is a correct simplified testcase.

Updated

16 years ago
Priority: -- → P2

Updated

16 years ago
Target Milestone: --- → Future

Comment 12

16 years ago
*** Bug 160242 has been marked as a duplicate of this bug. ***

Comment 13

16 years ago
Also happening on my site...  Last line on "news" section is spaced too far when
lines wrap or there is a hard "br"

http://www.engin.umd.umich.edu/~npietran

build 202072204 Mandrake Linux 8.2

Comment 14

16 years ago
in testcase:  attachment (id=90395)

<html><head><title>Testcase bug 155333</title></head>
<body><span style="font-size: 50%">foo<br>bar<br>baz </span> </body></html>

If replace the blankspace between </span> and  </body> with string,such as
"aaa",you will find the display in ns7.0rc1/IE/mozilla is the same.

is that blankspace displayed by mozilla but canceled by IE or ns7.0rc1?
(Reporter)

Comment 15

16 years ago
Netscape 7 has this bug fixed. I'm worried about this since I think it is sad,
that, if I understand this correctly, 1.2 (a major stable release -- right?)
will be released with a major every-day rendering problem. Menus, download
manager, type ahead find, etc etc etc is all nice, but if the browser doesn't
render correctly all this is worth nothing.

Sorry, for getting on everybodys nerves, but I love open software and tell
everybody to get a Gecko browser, but I can't do that with a pure conscience
with this bug in it.
As far as I can tell, this is a relatively obscure issue relating to whether we
collapse the whitespace by ignoring the part that's inside the span or by
ignoring the part that's outside the SPAN (or, in the original case, SMALL). 
The backwards compatible thing seems to be to collapse inwards.

I'm quite surprised this behaves differently in Netscape 7.  Any ideas when the
behavior changed on the Mozilla trunk?  Knowing that would help figure out what
caused the problem.
Summary: too much space between lines → too much space between lines: whitespace must collapse inwards

Comment 17

16 years ago
comment 9:
applying attachment 77116 [details] [diff] [review] from bug 134580 causes this behavior to occur on the
branch.
Oh, right, that bug.  If I hadn't started caring about emulating stupid old
quirks I guess we wouldn't have gotten into this mess.  Maybe I should just back
that out.
dbaron, you wanna back out for 1.2?

/be

Comment 20

16 years ago
Created attachment 108647 [details] [diff] [review]
patch

Even if it deletes the space of the end of the line,
the height information on a space character remains.
The effect of a patch was checked by mozilla-1.2.1.

Comment 21

16 years ago
Comment on attachment 108647 [details] [diff] [review]
patch

test:
<pre>
a

b
</pre>

result:
a
b

A "test" will be displayed as a "result".
Attachment #108647 - Attachment is obsolete: true

Comment 22

16 years ago
Created attachment 108705 [details] [diff] [review]
patch v2

It changed so that preMode might be checked.

Updated

16 years ago
Attachment #108705 - Flags: review?(dbaron)
Comment on attachment 108705 [details] [diff] [review]
patch v2

This isn't quite right since it will cause us to break out of the loop without
setting |zeroEffectiveSpanBox| to true, when we would really want to continue.

However, there's a more general problem, which is that this really should be
collapsed with the code right below it, and we should only have one way of
detecting text frames that have collapsed to nothing.  That might be a width
check, but I"ll think a little more about whether that's the best way.
Attachment #108705 - Flags: review?(dbaron) → review-
Created attachment 109253 [details] [diff] [review]
patch

I think this is the right fix.	It fixes the testcases here and still passes
all the testcases on bug 134580.
Attachment #108705 - Attachment is obsolete: true
Taking.
Assignee: attinasi → dbaron
Hardware: PC → All
Whiteboard: [patch]
Target Milestone: Future → mozilla1.3beta
Comment on attachment 109253 [details] [diff] [review]
patch

Requesting r+sr for my patch.

Thanks for the attempt to fix this, though.  It's not easy code to understand,
even for me (and I wrote parts of it).	I blame that on the complexity of the
requirements...
Attachment #109253 - Flags: superreview?(roc+moz)
Comment on attachment 109253 [details] [diff] [review]
patch

>+      if (pfd->GetFlag(PFD_ISTEXTFRAME) &&
>+          (preMode || pfd->mBounds.width != 0)) {

I'm wondering I actually want to add 

|| pfd->GetFlag(PFD_ISNONWHITESPACETEXTFRAME)

to the end of the disjunction there to handle things like zero-width-non-joiner
or some other weird unicode characters that have no width.  I don't think it
matters much, though, so I'm inclined to leave it as-is.
I don't like the width check. Couldn't this get confused by negative
letter-spacing, for example? (Although we may have other bugs when the width of
a text frame becomes 0 due to negative letter (or word) spacing, we probably
shouldn't make the situation worse.)

Perhaps we should steal the last PFD_ bit for a new flag, say PFD_ISCOLLAPSED.
What about doing the width check only if it's all-whitespace?  I'm not crazy
about it either, but this is quirks-mode only.

(We used to have a TRIMMED bit on something, which I think had the meaning you
suggest, but it wasn't used, and I needed the space, so I removed it.)
OK, so add "|| pfd->GetFlag(PFD_ISNONWHITESPACETEXTFRAME)" (actually put it
first since it's the common case) and you've got my r+sr.
Fix checked in to trunk, 2002-12-18 16:21 PDT.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Attachment #109253 - Flags: superreview?(roc+moz)
Attachment #109253 - Flags: superreview+
Attachment #109253 - Flags: review+

Comment 32

15 years ago
*** Bug 191008 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.