Closed Bug 75963 Opened 24 years ago Closed 24 years ago

newline (whitespace) is messing with the layout of list items

Categories

(Core :: Layout, defect, P2)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9

People

(Reporter: attinasi, Assigned: attinasi)

References

Details

(Keywords: regression, Whiteboard: [ETA:04/19/01])

Attachments

(3 files)

Patch to avoid applying minHeight in QuirksMode if the line is empty (also some error checking, unrelated but general goodness)
1.67 KB, patch
Details | Diff | Splinter Review
original testcase "This works OK"
225 bytes, text/html; charset=UTF-8
Details
original testcase "This is BAD"
227 bytes, text/html; charset=UTF-8
Details
Whitespace between an <LI> and a <DIV> is whacking the bullet on the LI. This works OK: <HTML> <BODY> <OL> <LI><DIV align=left>This is number 1, with div align set to left</DIV> <LI>This is number 2 <LI><DIV align=left>This is number 3, with div align set to left</DIV> <LI>This is number 4 </OL> </BODY> </HTML> This is BAD: <HTML> <BODY> <OL> <LI> <DIV align=left>This is number 1, with div align set to left</DIV> <LI>This is number 2 <LI> <DIV align=left>This is number 3, with div align set to left</DIV> <LI>This is number 4 </OL> </BODY> </HTML> Only difference is the newline between teh <LI> and the <DIV>. newline between the <LI> and some text makes no difference, however. (Severity set to major due to internal request by a higher authority...)
Accepting, will look into asap
Status: NEW → ASSIGNED
marc, you did some changes related to the bullet and such recently? Ah, in that case, you may been bitten by the whitespace frame problem. See how I learned it the hard way too in bug 10207. With <LI>\n<DIV>, there is an intermediate whitespace frame that leads to the creation of an empty linebox. Knowing this, some trickery is needed to avoid the mess.
Blocks: 75664
Roger, I guess you are referring to this tomfoolery? + nsLineBox* line = mLines; + // see if this first line is empty. if so, move on to the second line + if (line && (0 == line->GetHeight())) { + line = line->mNext; + } I'll check if that applies to this problem too - thanks for the tip!
Yep, that's the one. Until something better from "bug 68489: savings from ignorable whitespace"
Milestone --> Mozilla 0.9
Priority: -- → P2
Target Milestone: --- → mozilla0.9
Likely a regression from the patch attached on bug 59086 - nsLineBox* line = aState.NewLineBox(mBullet, 1, PR_FALSE); - if (!line) { - return NS_ERROR_OUT_OF_MEMORY; + if (mLines) {
Keywords: regression
Two reasons I do not believe that this is a regression from the fix for bug 59086: 1) the code that was changed for bug 59086 is never hit in the testcase - it is ONLY if the bullet is positioned inside, and by default bullets are inside (put a breakpoint there, it is never hit with the testcase). *** notice that the position has to be INSIDE for the code I changed to matter if (NS_STYLE_LIST_STYLE_POSITION_INSIDE == styleList->mListStylePosition) { if (mBullet && HaveOutsideBullet()) { // We now have an inside bullet, but used to have an outside // bullet. Adjust the frame line list nsLineBox* line = aState.NewLineBox(mBullet, 1, PR_FALSE); if (!line) { return NS_ERROR_OUT_OF_MEMORY; } line->mNext = mLines; mLines = line; } mState &= ~NS_BLOCK_FRAME_HAS_OUTSIDE_BULLET; } 2) the problem was reported before my fix went in, in fact it exists in NS6.01. Is there some concrete reason that this is believed to be a regression? It matters, since it may impact how I go about trying to fix it - maybe.
No, there is no concrete reason. It was just a hint to help cut the search space. But it appears not to be relevant as you pointed out.
Thanks Roger - good guess anyway, that bullet code is a bit, ah, picky (or is it just tricky?).
Added conservative ETA
Whiteboard: [ETA:04/19/01]
One interesting thing I have discovered: This bug only appears in Quirks Mode. Setting Viewer to Standard mode the DIV layout on the same line as the bullet. Also, removing quirks.css from ua.css, does nto change this behavior, so it is not some rule in quirks.css that is causing this. My guess is that there is some quirks Mode Only handling in layout that is involved here, probably at fault. Another interesting finding that I cannot narrow down yet: the line that is created for the newline character (right after the LI block) has a height of 285, but the text frame itself has a height of 0. Beig paranoid, I reverted a change I made a few weeks ago in nsLineLayout (it made empty frames participate in the combined area fo a line) but it made no difference to either the visual layout or the values in the frame dump (~line 2911 nsLineLayout.cpp, look for #if 0).
I think I have figured this out now. Attahcing a patch to nsLineLayout (but I'm still testing a related bug mentioned in surrounding code).
I also tested the problem reported in bug 50480, which was reportedly fixed by a hack in this general area, and it is still OK. Chris, can you review this for me?
Could you walk me through how this fixes the bug?
Ok, I see what's going on. It seems like were sort of tricking line layout into making the second line over-write the first. This is a bit scary to me, but certainly is producing correct results AFAICT. Might it be worth considering a solution in the frame constructor that doesn't create the whitespace-only frame at all? Your call. r=waterson
Chris, I see the fix differently. Rather than tricking layout into putting the second line over the first, I think we are making layout handle the common case where the 'first' line is in fact not part of layout at all because it is only made up of whitespace (and we are not respecting whitespace stylistically) so it is really empty. This is very scary to me too, but for a different reason. Since we create frames for the whitespace in the markup, even when the whitespace is not significant to the layout, we have to continually make sure we are handling it in our line layout algorithms. In this case, applying a minHeight was a Quirk anyway, and that is what messed up an otherwise correct empty-line-height calculation, but in general it still seems to be a concern that crops up too often. To your second point, about attacking this at frame construction, I have to plead ignorance. I do not really understand why we create text frames for insignificant whitespace at all, and maybe we should not. Is it for DOM access? Do we need it for Composer? If a block does not have 'white-space:pre' or something eqiuivalent, then why bother creating and managing all of those empty text frames? My (ignorant) inclination is to avoid creating frames for insignificant whitespace, to save memory, make frame management faster, and make the layout code simpler. I'd liek to try but I have a feeling I'd break something important... In summary, the patch just checks to see if the loginal first line is empty before applying a minHeight to it. By allowing the empty line to stay at 0-height the next logical line gets positioned correctly next to the bullet, on the 'geometrical' first line. Handling of insignificant whitespace appears to be sprinkled all over the block and line layout code, and maybe we need to revisit our handling of ignorable whitespace (or at least teach me about it more). D. Baron, do you have any experience or recollection of whitespace handling in layout, why we create text frames for insignificant whitespace, what happens if we do not?
We need the white space frames for 'white-space: pre', but that should easily be solved by changing the style hint for 'white-space' to FRAMECHANGE. rbs filed a bug against me to do it, bug 68489. If you want to take that bug, feel free (but keep me on the cc: list).
r=karnaze fix this -#undef NOISY_VERTICAL_ALIGN +#define NOISY_VERTICAL_ALIGN I would rather see if ((mLineBox->mBounds.height > 0) || (mLineBox->mBounds.width > 0)) { than if (mLineBox->mBounds.height || mLineBox->mBounds.width) { since there could be a bug having a negative value.
Gotcha - Thanks Chris. Also, waterson indicated to me that his r= can be considered an sr= - really, he did.
sr=waterson
Checking in nsLineLayout.cpp; /cvsroot/mozilla/layout/html/base/src/nsLineLayout.cpp,v <-- nsLineLayout.cpp new revision: 3.97; previous revision: 3.96 done
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Is this the same as bug 55357?
Marking verified in the May 30th build.
Status: RESOLVED → VERIFIED
I was tracking bug 101180 and it seems it is a regression from this bug. The test "if (mLineBox->mBounds.height || mLineBox->mBounds.width)" in the patch assumes that mLineBox->mBounds is already set when the test is being done -- but this isn't the case at first reflow. Follow-up in bug 101180.
I think this caused regression bug 194831, which is actually rather serious (IMO a more important case). I think the real fix here is better logic for when the bullet should be considered to be attached to a nested block.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: