Closed Bug 427370 Opened 16 years ago Closed 16 years ago

list bullet alignment (ol, ul) with "float:left" inside the list item is broken

Categories

(Core :: Layout: Block and Inline, defect)

Product:
Core
Component:
Layout: Block and Inline
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9

People

(Reporter: jens-bugzilla.mozilla.org, Assigned: MatsPalmgren_bugz)

References

(
URL
)

Details

(Keywords: regression, testcase)

Attachments

(11 files, 5 obsolete files)

Reporter's testcase (with added border + outline)
1.44 KB, text/html
Details
Screenshot of 2nd Testcase
40.43 KB, image/png
Details
Testcase #2
4.19 KB, text/html
Details
Testcase #3
3.11 KB, text/html
Details
reftest.diff
9.71 KB, patch
Details | Diff | Splinter Review
Testcase #4
5.79 KB, text/html
Details
Testcase #5
5.89 KB, text/html
Details
Patch rev. 2
7.04 KB, patch
dbaron
: review+
dbaron
: superreview+
beltzner
: approval1.9+
Details | Diff | Splinter Review
Testcase #6
783 bytes, text/html
Details
Testcase #9
2.34 KB, text/html
Details
Patch rev. 4
8.61 KB, patch
dbaron
: review-
dbaron
: superreview-
Details | Diff | Splinter Review 
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>
This sounds like Bug 427129.  Which the problem arose because the CSS spec changed.  
(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.



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
Attached file Testcase (obsolete) — 
A more elaborate testcase.  Specifically, it tests all three places where
we call ReflowBullet().
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
Status: UNCONFIRMED → NEW
Component: General → Layout: Block and Inline
Ever confirmed: true
Keywords: regression, testcase
Product: Firefox → Core
QA Contact: general → layout.block-and-inline
Version: unspecified → Trunk
I think the 'outside' cases should look like this.  FWIW, I think we render
the 'inside' cases wrong too.
Attached file Testcase #2 — 

        Attachment #314061 -
        Attachment is obsolete: true

    
    

    
  
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.
Flags: blocking1.9?

    
    

    
  
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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch rev. 1 (obsolete)
        — 
          — Splinter Review
      

    


  
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.

    
    

    
  
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?

    
    

    
  
(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).

    
    

    
  

      
      
      
      

        Attached file
          
        Testcase #3
        — 
      

    


  

    
    

    
  
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.

    
    

    
  
It's ready for review.  I'll do a reftest from the 'outside' parts of
Testcase #2 and #3.

    
  

        Attachment #314116 -
        Flags: superreview?(dbaron)

        Attachment #314116 -
        Flags: review?(dbaron)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        reftest.diff
        — 
          — Splinter Review
      

    


  

    
    

    
  
Wouldn't block on this, but if we have solid patch with reftests, will take.

wanted1.9.0.x+

  
Flags: wanted1.9.0.x+
Flags: blocking1.9?
Flags: blocking1.9-

    
    

    
  
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).
Blocks: 413840
Summary: list alignment (ol, ul) with "float:left" is broken → list bullet alignment (ol, ul) with "float:left" inside the list item is broken

    
    

    
  
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).

    
    

    
  
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>

        Attachment #314116 -
        Flags: superreview?(dbaron)

        Attachment #314116 -
        Flags: superreview-

        Attachment #314116 -
        Flags: review?(dbaron)

        Attachment #314116 -
        Flags: review-

    
    

    
  

      
      
      
      

        Attached file
          
        Testcase #4
        — 
      

    


  
A few variations of <li><img align="..."><div>text</div></li>

    
    

    
  

      
      
      
      

        Attached file
          
        Testcase #5
        — 
      

    


  
Testcase #4 with each LI relative positioned outward 2em.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch rev. 2
        — 
          — Splinter Review
      

    


  
(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.

        Attachment #314116 -
        Attachment is obsolete: true

        Attachment #314515 -
        Flags: superreview?(dbaron)

        Attachment #314515 -
        Flags: review?(dbaron)

    
    

    
  
(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>

    
    

    
  

      
      
      
      

        Attached file
          
        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)

    
    

    
  
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>


    
    

    
  

      
      
      
      

        Attached file
          
        Testcase #7 (obsolete)
        — 
      

    


  
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.

    
    

    
  

      
      
      
      

        Attached file
          
        Testcase #8 (obsolete)
        — 
      

    


  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch rev. 3 (obsolete)
        — 
          — Splinter Review
      

    


  
Same as rev. 2, with the addition in FlowAndPlaceFloat() that adjusts
mOutsideBulletX if a float is outside it. This fixes Testcase #8 too.

        Attachment #314515 -
        Attachment is obsolete: true

        Attachment #314552 -
        Flags: superreview?(dbaron)

        Attachment #314552 -
        Flags: review?(dbaron)

        Attachment #314515 -
        Flags: superreview?(dbaron)

        Attachment #314515 -
        Flags: review?(dbaron)

    
  

        Attachment #314534 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        Testcase #9
        — 
      

    


  

        Attachment #314550 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch rev. 4
        — 
          — Splinter Review
      

    


  
Minor correction, need to add in the BorderPadding there too...

        Attachment #314552 -
        Attachment is obsolete: true

        Attachment #314560 -
        Flags: superreview?(dbaron)

        Attachment #314560 -
        Flags: review?(dbaron)

        Attachment #314552 -
        Flags: superreview?(dbaron)

        Attachment #314552 -
        Flags: review?(dbaron)

    
    

    
  
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.

    
    

    
  
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 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

        Attachment #314515 -
        Flags: superreview+

        Attachment #314515 -
        Flags: review+

    
    

    
  
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).

        Attachment #314560 -
        Flags: superreview?(dbaron)

        Attachment #314560 -
        Flags: superreview-

        Attachment #314560 -
        Flags: review?(dbaron)

        Attachment #314560 -
        Flags: review-

    
    

    
  
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).

        Attachment #314515 -
        Attachment is obsolete: false

        Attachment #314515 -
        Flags: approval1.9?

    
    

    
  
Comment on attachment 314515 [details] [diff] [review]
Patch rev. 2

a1.9=beltzner

        Attachment #314515 -
        Flags: approval1.9? → approval1.9+

    
    

    
  
Mats, did you want to land this, or should I?

(You're ok with that patch landing, right?)
Assignee: nobody → mats.palmgren
Target Milestone: --- → mozilla1.9

    
  
Blocks: 428810

    
    

    
  
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)
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Flags: wanted1.9.0.x+

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: