Closed Bug 75963 Opened 23 years ago Closed 23 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: 23 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: