Closed
Bug 427370
Opened 17 years ago
Closed 17 years ago
list bullet alignment (ol, ul) with "float:left" inside the list item is broken
Categories
(Core :: Layout: Block and Inline, defect)
Core
Layout: Block and Inline
Tracking
()
RESOLVED
FIXED
mozilla1.9
People
(Reporter: jens-bugzilla.mozilla.org, Assigned: MatsPalmgren_bugz)
References
()
Details
(Keywords: regression, testcase)
Attachments
(11 files, 5 obsolete files)
|
(deleted),
text/html
|
Details | |
|
(deleted),
image/png
|
Details | |
|
(deleted),
text/html
|
Details | |
|
(deleted),
text/html
|
Details | |
|
(deleted),
patch
|
Details | Diff | Splinter Review | |
|
(deleted),
text/html
|
Details | |
|
(deleted),
text/html
|
Details | |
|
(deleted),
patch
|
dbaron
:
review+
dbaron
:
superreview+
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
|
(deleted),
text/html
|
Details | |
|
(deleted),
text/html
|
Details | |
|
(deleted),
patch
|
dbaron
:
review-
dbaron
:
superreview-
|
Details | Diff | Splinter Review |
Screenshot of 2nd Testcase
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•17 years ago
|
||
This sounds like Bug 427129. Which the problem arose because the CSS spec changed.
|
||
Comment 2•17 years ago
|
||
(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.
|
Reporter | |
Comment 3•17 years ago
|
||
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
|
Assignee | |
Comment 4•17 years ago
|
||
|
|
Assignee | |
Comment 5•17 years ago
|
||
A more elaborate testcase. Specifically, it tests all three places where
we call ReflowBullet().
|
|
Assignee | |
Comment 6•17 years ago
|
||
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
|
|
Assignee | |
Comment 7•17 years ago
|
||
I think the 'outside' cases should look like this. FWIW, I think we render
the 'inside' cases wrong too.
|
|
Assignee | |
Comment 8•17 years ago
|
||
Attachment #314061 -
Attachment is obsolete: true
|
|
||
Comment 9•17 years ago
|
||
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?
|
|
Assignee | |
Comment 10•17 years ago
|
||
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.
|
|
Assignee | |
Comment 11•17 years ago
|
||
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•17 years ago
|
||
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?
|
|
Assignee | |
Comment 13•17 years ago
|
||
(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).
|
|
Assignee | |
Comment 14•17 years ago
|
||
|
|
||
Comment 15•17 years ago
|
||
Ah, ok, that makes sense.
|
|
||
Comment 16•17 years ago
|
||
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.
|
|
Assignee | |
Comment 17•17 years ago
|
||
It's ready for review. I'll do a reftest from the 'outside' parts of
Testcase #2 and #3.
|
|
Assignee | |
Updated•17 years ago
|
Attachment #314116 -
Flags: superreview?(dbaron)
Attachment #314116 -
Flags: review?(dbaron)
|
|
Assignee | |
Comment 18•17 years ago
|
||
|
||
Comment 19•17 years ago
|
||
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-
|
|
||
Comment 20•17 years ago
|
||
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
|
|
||
Comment 21•17 years ago
|
||
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•17 years ago
|
||
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>
|
|
||
Updated•17 years ago
|
Attachment #314116 -
Flags: superreview?(dbaron)
Attachment #314116 -
Flags: superreview-
Attachment #314116 -
Flags: review?(dbaron)
Attachment #314116 -
Flags: review-
|
|
Assignee | |
Comment 23•17 years ago
|
||
A few variations of <li><img align="..."><div>text</div></li>
|
|
Assignee | |
Comment 24•17 years ago
|
||
Testcase #4 with each LI relative positioned outward 2em.
|
|
Assignee | |
Comment 25•17 years ago
|
||
(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)
|
|
||
Comment 26•17 years ago
|
||
(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>
|
|
Assignee | |
Comment 27•17 years ago
|
||
(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•17 years ago
|
||
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>
|
|
Assignee | |
Comment 29•17 years ago
|
||
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.
|
|
Assignee | |
Comment 30•17 years ago
|
||
|
|
Assignee | |
Comment 31•17 years ago
|
||
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)
|
|
Assignee | |
Updated•17 years ago
|
Attachment #314534 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 32•17 years ago
|
||
Attachment #314550 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 33•17 years ago
|
||
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 34•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
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 37•17 years ago
|
||
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 38•17 years ago
|
||
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 39•17 years ago
|
||
Comment on attachment 314515 [details] [diff] [review]
Patch rev. 2
a1.9=beltzner
Attachment #314515 -
Flags: approval1.9? → approval1.9+
|
|
||
Comment 40•17 years ago
|
||
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
|
|
Assignee | |
Comment 41•17 years ago
|
||
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: 17 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
|
||
Updated•16 years ago
|
Flags: wanted1.9.0.x+
You need to log in
before you can comment on or make changes to this bug.
Description
•