Last Comment Bug 427370 - list bullet alignment (ol, ul) with "float:left" inside the list item is broken
: list bullet alignment (ol, ul) with "float:left" inside the list item is broken
Status: RESOLVED FIXED
: regression, testcase
Product: Core
Classification: Components
Component: Layout: Block and Inline (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9
Assigned To: Mats Palmgren (:mats)
:
Mentors:
http://register.dive-deeper.org/en/us...
Depends on:
Blocks: 413840 428810
  Show dependency treegraph
 
Reported: 2008-04-06 05:49 PDT by Jens Benecke
Modified: 2008-10-05 17:45 PDT (History)
10 users (show)
dsicore: blocking1.9-
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Reporter's testcase (with added border + outline) (1.44 KB, text/html)
2008-04-07 03:36 PDT, Mats Palmgren (:mats)
no flags Details
Testcase (4.18 KB, text/html)
2008-04-07 03:40 PDT, Mats Palmgren (:mats)
no flags Details
Screenshot of 2nd Testcase (40.43 KB, image/png)
2008-04-07 04:01 PDT, Mats Palmgren (:mats)
no flags Details
Testcase #2 (4.19 KB, text/html)
2008-04-07 04:09 PDT, Mats Palmgren (:mats)
no flags Details
Patch rev. 1 (9.16 KB, patch)
2008-04-07 10:18 PDT, Mats Palmgren (:mats)
dbaron: review-
dbaron: superreview-
Details | Diff | Review
Testcase #3 (3.11 KB, text/html)
2008-04-08 05:27 PDT, Mats Palmgren (:mats)
no flags Details
reftest.diff (9.71 KB, patch)
2008-04-08 14:52 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Review
Testcase #4 (5.79 KB, text/html)
2008-04-08 21:42 PDT, Mats Palmgren (:mats)
no flags Details
Testcase #5 (5.89 KB, text/html)
2008-04-08 21:45 PDT, Mats Palmgren (:mats)
no flags Details
Patch rev. 2 (7.04 KB, patch)
2008-04-08 22:44 PDT, Mats Palmgren (:mats)
dbaron: review+
dbaron: superreview+
mbeltzner: approval1.9+
Details | Diff | Review
Testcase #6 (783 bytes, text/html)
2008-04-08 23:45 PDT, Mats Palmgren (:mats)
no flags Details
Testcase #7 (626 bytes, text/html)
2008-04-09 00:33 PDT, Mats Palmgren (:mats)
no flags Details
Testcase #8 (2.14 KB, text/html)
2008-04-09 02:29 PDT, Mats Palmgren (:mats)
no flags Details
Patch rev. 3 (8.58 KB, patch)
2008-04-09 02:46 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Review
Testcase #9 (2.34 KB, text/html)
2008-04-09 03:47 PDT, Mats Palmgren (:mats)
no flags Details
Patch rev. 4 (8.61 KB, patch)
2008-04-09 03:50 PDT, Mats Palmgren (:mats)
dbaron: review-
dbaron: superreview-
Details | Diff | Review

Description Jens Benecke 2008-04-06 05:49:37 PDT
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; de; rv:1.9b5) Gecko/2008032619 Firefox/3.0b5
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; de; rv:1.9b5) Gecko/2008032619 Firefox/3.0b5

http://register.dive-deeper.org/en/users/new (Simple testcase below in "additional information")

Look for three checkboxes starting with "I am a vegetarian."
I used "label" elements with a fixed "width:12em;" to position the checkboxes, to avoid having to use tables.

Firefox 2.0.0.12 displayed the checkboxes neatly beneath each other. That is what I wanted.

Firefox 3.0 b5 adds an *additional* 12em to the left margin of each check box.



Reproducible: Always

Steps to Reproduce:
1.
2.
3.



<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
       "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">

<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
<head>
	<title>Title</title>
  <meta http-equiv="content-type" content="text/html;charset=UTF-8" />
<style type="text/css"><!--
	ol label {
		float:left;
	}
// --></style>
</head>

<body>
<p>Test document (W3C validated). The three list elements and checkboxes should be neatly above each other, _not_ cascaded.</p>

		<ol>
			<li>
				<label>
					<input type="checkbox" value="1" />
				</label>
				Choice A.
			</li>
			<li>
				<label>
					<input type="checkbox" value="1" />
				</label>
				Choice B.
			</li>
			<li>
				<label>
					<input type="checkbox" value="1" />
				</label>
				Choice C.
			</li>
</ol>
</body>
</html>
Comment 1 Brian Polidoro 2008-04-06 15:54:30 PDT
This sounds like Bug 427129.  Which the problem arose because the CSS spec changed.  
Comment 2 philippe (part-time) 2008-04-06 16:50:13 PDT
(In reply to comment #1)
> This sounds like Bug 427129.  Which the problem arose because the CSS spec
> changed.  
> 
Not really, bug 427129 is a margin(-left) issue. In this case, the floated label is displaced because it is floating, but the second (next) line is not clearing the previous line.

(text)-zoom just a little to see the checkboxes align vertically.

The floated label is made taller than it is in Gecko 1.8 due to the height of the checkbox.

li {clear:left} would solve that in the style sheet.


Comment 3 Jens Benecke 2008-04-07 02:21:35 PDT
So ... does that mean my HTML was incorrect (i.e. does not do what I wanted) and I need to clear:left; or use zooming, or is this a bug in FF3 that will be fixed?

If the size of the checkbox affects the next line, is this perhaps platform specific? (so far, I only tested FF3 b5 with Mac OS X, which sometimes has its own weird ideas of sizing widgets, but I don't know if/how that applies to FF.)

Do you need any more help / info / ... ?

Thanks,

Jens
Comment 4 Mats Palmgren (:mats) 2008-04-07 03:36:50 PDT
Created attachment 314060 [details]
Reporter's testcase (with added border + outline)
Comment 5 Mats Palmgren (:mats) 2008-04-07 03:40:21 PDT
Created attachment 314061 [details]
Testcase

A more elaborate testcase.  Specifically, it tests all three places where
we call ReflowBullet().
Comment 6 Mats Palmgren (:mats) 2008-04-07 03:46:56 PDT
I think this is a bug.  CSS 2.1 is quite clear that the marker should
be outside the principal box (for 'list-style-position:outside').
http://www.w3.org/TR/CSS21/generate.html#propdef-list-style-position
Comment 7 Mats Palmgren (:mats) 2008-04-07 04:01:27 PDT
Created attachment 314063 [details]
Screenshot of 2nd Testcase

I think the 'outside' cases should look like this.  FWIW, I think we render
the 'inside' cases wrong too.
Comment 8 Mats Palmgren (:mats) 2008-04-07 04:09:14 PDT
Created attachment 314065 [details]
Testcase #2
Comment 9 Boris Zbarsky [:bz] (Out June 25-July 6) 2008-04-07 09:58:20 PDT
So I'm not seeing the "checkbox offset" issue.  As philippe points out, it's just due to the page assuming that the checkbox doesn't overflow the list item vertically.  This will in fact vary per-platform.  That said, the fact that it does overflow seems bad; can we avoid that?

I do see a regression with the float overlapping the list marker.  This regressed between 2008-02-10-01 and 2008-02-11-01.  Bonsai checkins:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-02-10+01&maxdate=2008-02-11+01&cvsroot=%2Fcvsroot

Looks like the moz-float-edge change, perhaps.  This regression seems like something we need to definitely fix to me; it might need a separate bug.
Comment 10 Mats Palmgren (:mats) 2008-04-07 10:11:03 PDT
Yeah, I agree that the float causing the content in the items below
to be indented is not a bug.  The marker placement is though, IMO.
Comment 11 Mats Palmgren (:mats) 2008-04-07 10:18:43 PDT
Created attachment 314116 [details] [diff] [review]
Patch rev. 1

This fixes the placement of 'outside' markers in Testcase #2 and it passes
reftests.  The idea is to save the principal box's border-box edge with
potential displacement by sourrounding floats *before* floats in the
box itself is processed.  I couldn't figure out a way to calculate that
position when the bullet frame is placed, so I had to use a member to
save it.

The 'inside' marker problem seems harder to fix, but that doesn't work
correctly in Fx2 either.
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-04-07 12:06:52 PDT
So what exactly are you fixing?  There seem to be a bunch of different issues discussed in this bug.  Could you describe the change that this patch makes?
Comment 13 Mats Palmgren (:mats) 2008-04-08 05:25:04 PDT
(In reply to comment #12)
> So what exactly are you fixing?

I'm fixing the horizontal position of an 'list-style-position:outside'
marker box.  The position is currently affected by floats inside
the <LI> itself.  An 'outside' marker should be placed outside the <LI>
border-box shrinked to avoid floats earlier in the document.

> Could you describe the change that this patch makes?

It calculates the x-position (left edge in LTR, right in RTL) and
remembers it until the bullet frame is placed.  The current code
calculates the position when 'aState.mAvailSpaceRect' has already been
influenced by floats on the line (ie too late).
Comment 14 Mats Palmgren (:mats) 2008-04-08 05:27:00 PDT
Created attachment 314324 [details]
Testcase #3
Comment 15 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-04-08 13:09:59 PDT
Ah, ok, that makes sense.
Comment 16 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-04-08 13:13:33 PDT
Mats, do you think this patch is ready for review?  It would be good to get it in today if possible.

That said, we should have reftests for this.
Comment 17 Mats Palmgren (:mats) 2008-04-08 13:25:29 PDT
It's ready for review.  I'll do a reftest from the 'outside' parts of
Testcase #2 and #3.
Comment 18 Mats Palmgren (:mats) 2008-04-08 14:52:34 PDT
Created attachment 314418 [details] [diff] [review]
reftest.diff
Comment 19 Damon Sicore (:damons) 2008-04-08 15:44:35 PDT
Wouldn't block on this, but if we have solid patch with reftests, will take.

wanted1.9.0.x+

  
Comment 20 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-04-08 15:46:17 PDT
Yeah, I don't think this is a "we would hold the release for it" bug, but I think we definitely should take the patch -- it's a pretty bad regression (from bug 413840).
Comment 21 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-04-08 15:57:24 PDT
The patch looks fine, except why not make CalcOutsideBulletX a method on nsBlockReflowState (and just put a GetAvailableSpace call inside it)?

Also, given that you're removing this PR_MIN:
-    x = PR_MIN(rs.ComputedWidth(), aState.mAvailSpaceRect.XMost())
could you add an assertion in CalcOutsideBulletX that "mAvailSpaceRect.XMost() <= mReflowState.ComputedWidth()", since that's what you're assuming (I think it's true, but we'd want to know if it's not)?

The reftest seems fine as well, but I'd want to add some additional tests (probably in separate files) checking the simple cases against placement of the bullets without any float displacement issues -- i.e., test float displacement of the bullet (or not) against changing the horizontal margin on a div containing the list (or on the list itself).
Comment 22 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-04-08 16:03:25 PDT
That said, I also wonder why you call CalcOutsideBulletX in all three places rather than just in the first?  Couldn't floats be added between the two, and, if so, wouldn't the first calculation be correct and the second be incorrect?

I'm thinking in particular of a testcase that looks like:

<li><img align="left"><div>text</div></li>
Comment 23 Mats Palmgren (:mats) 2008-04-08 21:42:45 PDT
Created attachment 314499 [details]
Testcase #4

A few variations of <li><img align="..."><div>text</div></li>
Comment 24 Mats Palmgren (:mats) 2008-04-08 21:45:39 PDT
Created attachment 314501 [details]
Testcase #5

Testcase #4 with each LI relative positioned outward 2em.
Comment 25 Mats Palmgren (:mats) 2008-04-08 22:44:33 PDT
Created attachment 314515 [details] [diff] [review]
Patch rev. 2

(In reply to comment #21)
> could you add an assertion in CalcOutsideBulletX ...
> (I think it's true, but we'd want to know if it's not)?

Did so and it turned out the assumption was wrong (sorry, should have
checked), it triggered for: layout/reftests/bugs/420069-1.html
The reduced testcase is:
<p style="float:left;">F</p>
<div style="width:1px"></div>
it seems XMost() is bound by the viewport when reflowing the DIV,
removing the P makes it bound by 1px -- this seems strange to me, but let's
handle it in a separate bug, it isn't needed to fix bullet positioning.

> i.e., test float displacement
> of the bullet (or not) against changing the horizontal margin on a div
> containing the list (or on the list itself).

FYI, maybe these tests cover that (or can be used as a start):
layout/reftests/bugs/413840-ltr-offsets.html
layout/reftests/bugs/413840-rtl-offsets.html


(In reply to comment #22)
> That said, I also wonder why you call CalcOutsideBulletX in all three places
> rather than just in the first? 

To be honest, I fixed last two cases first and then didn't realize we
only need to set it once, good catch.
However, the later calls actually handled a case I wasn't aware of:
layout/reftests/bugs/413840-pushed-line-bullet.html
If I use patch1 with only the first CalcOutsideBulletX() then the bullet
is rendered in the middle of the text - this is because the float edge
is to the right of where the line actually starts.

So, we need to handle that, one solution is to calculate mOutsideBulletX
once as you suggested and then when bullet is placed use:
  PR_MIN(aState.mOutsideBulletX, aState.mAvailSpaceRect.x)
I think that should work since if there are line floats these always *add*
to mAvailSpaceRect.x and must be placed to the right of that edge so if
aState.mAvailSpaceRect.x < aState.mOutsideBulletX it must be because we
the line didn't care about the float edge...
Does that seem right?
(similar resoning for the right edge and PR_MAX)

Only calculating mOutsideBulletX once simplified things, so I moved it
into the block reflow state ctor instead.
Comment 26 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-04-08 23:15:25 PDT
(In reply to comment #25)
> So, we need to handle that, one solution is to calculate mOutsideBulletX
> once as you suggested and then when bullet is placed use:
>   PR_MIN(aState.mOutsideBulletX, aState.mAvailSpaceRect.x)
> I think that should work since if there are line floats these always *add*
> to mAvailSpaceRect.x and must be placed to the right of that edge so if
> aState.mAvailSpaceRect.x < aState.mOutsideBulletX it must be because we
> the line didn't care about the float edge...
> Does that seem right?
> (similar resoning for the right edge and PR_MAX)

I'm not sure this is quite right -- it wouldn't handle a case where there's both a float outside that you're clearing past, and a float inside whose position you want to ignore.  For example:

<div style="float:left; width:200px; height:50px"></div>
<div style="width: 250px">
  <div style="float:left; clear: left; width: 100px; height: 50px"></div>

  <span style="display:inline-block;width: 100px; height: 10px;">
    <!-- this clears past the first float but not the second; the bullet
         should be next to it -->
  </span>

</div>
Comment 27 Mats Palmgren (:mats) 2008-04-08 23:45:06 PDT
Created attachment 314525 [details]
Testcase #6

(In reply to comment #26)
Sorry, I don't understand where you meant the list-item to be in that test.
Does this test cover the case you're thinking of?
(there's no change in the layout with the patch)
Comment 28 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-04-09 00:15:08 PDT
Oops, sorry, I meant:

<div style="float:left; width:200px; height:50px"></div>
<li style="width: 250px">
  <div style="float:left; clear: left; width: 100px; height: 50px"></div>

  <span style="display:inline-block;width: 100px; height: 10px;">
    <!-- this clears past the first float but not the second; the bullet
         should be next to it -->
  </span>

</li>
Comment 29 Mats Palmgren (:mats) 2008-04-09 00:33:48 PDT
Created attachment 314534 [details]
Testcase #7

Ok, the layout is the same with the patch, which is a bug --
the bullet should be to the left of the green block.

I'll see if I can figure out a way to fix this case too, but I think
the current patch would be an improvement as is - it fixes most
cases and this remaining case renders the same as before.
Comment 30 Mats Palmgren (:mats) 2008-04-09 02:29:24 PDT
Created attachment 314550 [details]
Testcase #8
Comment 31 Mats Palmgren (:mats) 2008-04-09 02:46:44 PDT
Created attachment 314552 [details] [diff] [review]
Patch rev. 3

Same as rev. 2, with the addition in FlowAndPlaceFloat() that adjusts
mOutsideBulletX if a float is outside it. This fixes Testcase #8 too.
Comment 32 Mats Palmgren (:mats) 2008-04-09 03:47:07 PDT
Created attachment 314558 [details]
Testcase #9
Comment 33 Mats Palmgren (:mats) 2008-04-09 03:50:24 PDT
Created attachment 314560 [details] [diff] [review]
Patch rev. 4

Minor correction, need to add in the BorderPadding there too...
Comment 34 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-04-09 17:29:44 PDT
Comment on attachment 314560 [details] [diff] [review]
Patch rev. 4

>+  nscoord x = rs.mStyleVisibility->mDirection == NS_STYLE_DIRECTION_LTR ?
>+    PR_MIN(aState.mOutsideBulletX, aState.mAvailSpaceRect.x)
>+      - reflowState.mComputedMargin.right - aMetrics.width :
>+    PR_MAX(aState.mOutsideBulletX, aState.mAvailSpaceRect.XMost())
>+      + reflowState.mComputedMargin.left;

I think the idea of this last change was that you wouldn't need these PR_MIN and PR_MAX anymore (you could just use aState.mOutsideBulletX directly).

I need to think a little more about whether FlowAndPlaceFloat is the right place for the adjustment, and I don't think it is, but I need to think about it a little more.
Comment 35 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-04-10 01:25:51 PDT
Well, one reason I can guarantee that the FlowAndPlaceFloat change isn't sufficient is that the float could be inside a nested block, in which case you'd be touching the mOutsideBulletX of the nested block's reflow state, but not of the parent block.

I don't think it's possible to fix this correctly without actually poking through the actual floats in the band, and considering only those that were present before we started reflowing the block.  And once you're at that point, there's no point caching any of the work in advance.  This could be done using a cached pointer to within the space manager's mFrameInfoMap list, plus going through the mFrames of a BandRect... or something like that.

I haven't thought that through clearly enough to know how hard it would be.  It might be hard enough that we want to take some compromise here for this release... which probably means something like patch rev. 2.  (Or would you suggest a different one?)
Comment 36 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-04-11 10:31:30 PDT
Comment on attachment 314515 [details] [diff] [review]
Patch rev. 2

Let's take this patch (rev. 2) for now, with the addition of a comment here:

>+  // Place the bullet now, separate it from mOutsideBulletX by its margin.
>+  // If the mAvailSpaceRect position is outside the mOutsideBulletX
>+  // position it means the line didn't care about the float edge and we
>+  // use that position instead (there cannot be any floats at the start
>+  // of the line this case since that would violate CSS 2.1 float rules).
>+  nscoord x = rs.mStyleVisibility->mDirection == NS_STYLE_DIRECTION_LTR ?
>+    PR_MIN(aState.mOutsideBulletX, aState.mAvailSpaceRect.x)
>+      - reflowState.mComputedMargin.right - aMetrics.width :
>+    PR_MAX(aState.mOutsideBulletX, aState.mAvailSpaceRect.XMost())
>+      + reflowState.mComputedMargin.left;

That explains why the adjustment by mAvailSpaceRect.x or .XMost() isn't quite sufficient, and a pointer to a followup bug on fixing that issue.

r+sr=dbaron with that, so we can at least fix the 99% case here
Comment 37 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-04-11 10:32:28 PDT
Comment on attachment 314560 [details] [diff] [review]
Patch rev. 4

I don't think the changes in this revision really help (but note my r+sr on previous patch).
Comment 38 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-04-11 10:36:51 PDT
Comment on attachment 314515 [details] [diff] [review]
Patch rev. 2

Taking the liberty of requesting approval here as well.  This is a pretty bad regression from a recent change (enough that I was pretty torn about taking it off the blocker list), and I think we should take the fix (although it doesn't quite fix all cases).
Comment 39 Mike Beltzner [:beltzner, not reading bugmail] 2008-04-11 12:37:46 PDT
Comment on attachment 314515 [details] [diff] [review]
Patch rev. 2

a1.9=beltzner
Comment 40 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2008-04-13 08:49:46 PDT
Mats, did you want to land this, or should I?

(You're ok with that patch landing, right?)
Comment 41 Mats Palmgren (:mats) 2008-04-13 11:53:37 PDT
Landed rev. 2 with an additional comment referring to bug 428810.

mozilla/layout/generic/nsBlockFrame.cpp 	3.948
mozilla/layout/generic/nsBlockReflowState.cpp 	3.542
mozilla/layout/generic/nsBlockReflowState.h 	3.480
mozilla/layout/reftests/bugs/427370-1.html 	1.1
mozilla/layout/reftests/bugs/427370-1-ref.html 	1.1
mozilla/layout/reftests/bugs/reftest.list 	1.440 

I'll make some more reftests from the testcases on this bug later.

-> FIXED (filed bug 428810 for remaining cases)

Note You need to log in before you can comment on or make changes to this bug.