Closed Bug 155333 Opened 23 years ago Closed 23 years ago

too much space between lines: whitespace must collapse inwards

Categories

(Core :: Layout, defect, P2)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.3beta

People

(Reporter: benjamin, Assigned: dbaron)

References

Details

(Keywords: regression, testcase, Whiteboard: [patch])

Attachments

(3 files, 4 obsolete files)

Testcase
924 bytes, text/html
Details
simplified testcase
132 bytes, text/html
Details
patch
2.52 KB, patch
roc
: review+
roc
: superreview+
Details | Diff | Splinter Review
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!
Could you give the buildIDs of both versions?
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
Attached file testcase described by reporter (obsolete) —
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?)
Attached file 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.
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/
WFM 1.1a+ ID: 2002062321 & 2002070204 on Linux
Attached file simplified testcase (obsolete) —
Attachment #89902 - Attachment is obsolete: true
Keywords: testcase
If you put <BR> after your last line, you will get same line height. But this is only workaround.
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
Attached file simplified testcase —
This is a correct simplified testcase.
Priority: -- → P2
Target Milestone: --- → Future
*** Bug 160242 has been marked as a duplicate of this bug. ***
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
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?
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 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
Attached patch patch (obsolete) — — Splinter Review
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 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
Attached patch patch v2 (obsolete) — — Splinter Review
It changed so that preMode might be checked.
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-
Attached patch patch — — Splinter Review
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
Closed: 23 years ago
Resolution: --- → FIXED
Attachment #109253 - Flags: superreview?(roc+moz)
Attachment #109253 - Flags: superreview+
Attachment #109253 - Flags: review+
*** 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.

Attachment

General

Creator:
Created:
Updated:
Size: