Closed Bug 312777 Opened 19 years ago Closed 19 years ago

Negative margins cause floated elements to be placed to the right of incorrect earlier boxes

Categories

(Core :: Layout: Floats, defect)

defect
Not set
major

Tracking

()

VERIFIED FIXED

People

(Reporter: mozilla.org, Assigned: roc)

References

()

Details

(4 keywords)

Attachments

(9 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1

Quoting from CSS2.1, 9.5.1. Visual formatting model - Positioning the float: the
'float' property

"2. If the current box is left-floating, and there are any left-floating boxes
generated by elements earlier in the source document, then for each such earlier
box, either the left outer edge of the current box must be to the right of the
right outer edge of the earlier box, or its top must be lower than the bottom of
the earlier box. Analogous rules hold for right-floating boxes."


Gecko 1.8b2+ does not always "remember" the earlier box. Previous versions did.

Reproducible: Always

Steps to Reproduce:
Go the url above from where example pages are linked to

Actual Results:  
Incorrect layout rendering

Expected Results:  
Rendered the page as previous versions of Gecko did (and all other browsers)

This is in my opinion a very serious regression bug since it breaks adherence to
the specs and makes horizontal negative margins unpredictable - hence my ranking
of the severity as major.
Attached file simplified testcase
two floate blokcs (block1 with a left-margin, block2 with a negative margin),
followed by a static paragraph (red coloured) in the flow. The paragraph covers
the first floated block (block1).
This regressed between 20050306 and 20050307.

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050306
Firefox/1.0+ works correctly.
Summary: Negative margins cause floated elements to be placed to the right of incorrect earlier boxes → Negative margins cause floated elements to be placed to the right of incorrect earlier boxes
Well, Robert probably changed this in bug 276602. I don't know of any dupes of this.
Blocks: 276602
Keywords: testcase
Status: UNCONFIRMED → NEW
Ever confirmed: true
Yes, backing out the nsSpaceManager.cpp changes of the patch in bug 276602 fixes
this bug for me.
I actually don't think the spec is very clear on what happens to the flow around floats when floats have negative margin-box dimensions.  It would be nice to interoperate on this point, but it does seem really obscure, so it doesn't seem like something that's critical for 1.8.

I suspect we also have bugs on things that are clear, though.

The first step here seems to be writing and agreeing on what *should* happen in these cases.
So I think the reason this used to work is that we completely ignored floats with negative width, which seems wrong.  There seem to be two possible ways to fix this, and I'm leaning towards thinking that the first is more correct:

 * prevent float rects in the space manager from having negative dimensions:  increase negative dimensions to zero.

 * change nsBlockBandData::ComputeAvailSpaceRect (and the things it uses) to look at all floats rather than just the "rightmost" left float and "leftmost" right float (in quotes because with negative sizes they aren't actually always rightmost and leftmost)
Attached file Simplified testcase
This doesn't do anything really different from Philippe's, but it has more flowed text which reveals the various ways that browsers currently deal with things
David says that this is "obscure" and that the specs are unclear.

With regard to the specs being unclear, he's right. However it's absolutely clear that the new Gecko behaviour is utterly wrong and that the other modern browsers are marching in step (rightly or wrongly).

   http://www.fu2k.org/alex/css/testcases/negmarginfloat

As for it being obvious that the way that Safari, Opera and IE Mac (!) do it is right, we will have to wait for the CSS Working Group to pronounce on that.

(By the way, the reason for submitting the new simplified testcase was that the extra flowed text teases out the inconsistencies in all browsers that Philippe's does not)

As for this being an obscure issue, I don't think so and can only ask you to read the article I have just published at Position is Everything:

   http://www.positioniseverything.net/articles/onetruelayout

This is a real problem.
Where the floats should go is defined clearly by the rules in 9.5.1, we do that right. Since these are left floats, the text should wrap around the right margin edge of the floats, whichever encroaches most onto the line.
Rather than the previous link which puts the onus on the reader to trawl through my magnum opus, the part of the article dealing with negative margins applied to floats as an extermely powerful and standards based layout technique is:

   http://www.positioniseverything.net/articles/onetruelayout/anyorder

NB. the fact that there is a lack of clarity about how text should flow around floats when any of them have been negatively margined should not cloud the fact that there is no uncertainty about where a contained box should be positioned: with its left outer edge to the right of the rightmost float.
To clarify, what we do -- layout wise -- is right for attachment 200768 [details]. However it isn't right for the page given in the URL, where we put the third float in the wrong place, and it isn't right for the text in any of the attachments so far.

I agree with David that this isn't critical. Sorry, but if you're using floats to get columnar layouts, all bets are off. :-)
I've added a new test case which shows that the problem is not just with the flow - it affects boxes too. My mistake for not twigging that Philippe's original simplified test case might unwittingly given that impression.

> To clarify, what we do -- layout wise -- is right for attachment 200768 [details]

No it isn't. Firefox 1.5 has the text starting to the right of #float-two, not #float-one - which is what every other browser does and over which there is no lack of clarity in the specs. Unless I am spectacularly misunderstanding what you mean here.

As for not relying on floats for columnar layouts - huh? The vast majority of css-based layouts already do. The technique I've described is incredibly robust. Except for the fact that the new Gecko bug breaks it.
Layout. Not including text flow.
(In reply to comment #7)
> So I think the reason this used to work is that we completely ignored floats
> with negative width, which seems wrong.  There seem to be two possible ways to
> fix this, and I'm leaning towards thinking that the first is more correct:
> 
>  * prevent float rects in the space manager from having negative dimensions: 
> increase negative dimensions to zero.

I'm not sure why this would help. The floats here don't have negative dimensions.

>  * change nsBlockBandData::ComputeAvailSpaceRect (and the things it uses) to
> look at all floats rather than just the "rightmost" left float and "leftmost"
> right float (in quotes because with negative sizes they aren't actually always
> rightmost and leftmost)

I think this is actually what is needed.
Attached patch fix (obsolete) — Splinter Review
Sorry, I see what David meant in comment #7 now.

The space manager isn't set up to handle negative-width band rectangles. We should not be adding them; in any case, it's not clear what they mean. There shouldn't be any need to; if we know what area should be excluded, we can always express it as the union of non-negative-dimension band rectangles.

There's a choice here between increasing negative width to zero, and adjusting the rectangle to represent the same area using positive dimensions. This patch adjusts the rectangle to use positive dimensions, because I think that's more correct for right floats; we should be flowing around a negative-margin right-float's left border-edge (right?). Unfortunately the negative margin right-float case still doesn't work, I think due to some other bug... But the patch does fix the testcases here.
Assignee: nobody → roc
Status: NEW → ASSIGNED
Border edges shouldn't be affecting anything here, everything is in terms of margin edges. Also, only one margin edge should matter (the one that is opposite the side of the float, so left for right floats and right for left floats). No?
I think one fundamental problem here is that the space manager is set up to track occlusion regions and does not specifically track edges the way the CSS spec wants us to, so we have various hacks and bugs to get closer to the CSS spec ... although for things like SVG shape-flowed text the space manager is closer to what we want.

Thinking about this some more, the important thing when we transform a negative-width margin-box into a non-negative space manager rectangle is to preserve the right margin-edge for left floats and the left margin-edge for right floats. So for left floats we can set the rectangle's x to the old XMost and set its width to zero, and for right floats we can just set the width to zero.
Attached patch better fixSplinter Review
Okay, I believe this is the right fix; it's exactly what I said in the previous comment.
Attachment #200834 - Attachment is obsolete: true
Attachment #200923 - Flags: superreview?(dbaron)
Attachment #200923 - Flags: review?(dbaron)
Comment on attachment 200923 [details] [diff] [review]
better fix

>+    // Preserve the right margin-edge for left floats and the left
>+    // margin-edge for right floats
>+    if (isLeftFloat) {
>+      region.x = region.XMost();
>+    }
>+    region.width = 0;

I haven't thought about this in much detail, but could you explain why you chose this way rather than the other way around?
Er, actually, I'm a little puzzled about how that change fixes the bug given that it seems the wrong way around to me, unless it's the height check that's fixing the bug somehow.

Have you written testcases for a float with negative width but larger height?  e.g., in

left float, margin box 200x200
left float, margin box -100x300
text that wraps

and check interoperability of both how does the text wraps between:
 * 0 and 200 pixels from the tops of the floats
 * 200 and 300 pixels from the tops of the floats
(In reply to comment #21)
> (From update of attachment 200923 [details] [diff] [review] [edit])
> >+    // Preserve the right margin-edge for left floats and the left
> >+    // margin-edge for right floats
> >+    if (isLeftFloat) {
> >+      region.x = region.XMost();
> >+    }
> >+    region.width = 0;
> 
> I haven't thought about this in much detail, but could you explain why you
> chose this way rather than the other way around?

I'm not sure precisely what you mean by "this way".

I chose to preserve "right margin-edge for left floats and left margin-edge for right floats" because that's what matters and Hixie agrees (see comment #18).

This code achieves that:
a) for a left float, region.x is set to the right edge (which is to the left of the left edge, but no matter), and width is set to zero.
b) for a right float, region.x remains at the left edge, and width is set to zero.

It fixes the bug because (e.g. in attachment 200807 [details]) the addition of the second float inserts an empty rect into the space manager before the band-rect for the first float. The available space is then what you'd expect: all the space to the right of the first float's right edge.

I will try the test in comment #22 when I get to the office.
Attached file testcase
This is rougly what David asked for.
This is how it renders. I believe this rendering is correct. KHTML 4.2 gives the same rendering (modulo font choices).
Not exactly the same as my patch or KHTML. It's hard for me to judge whether this is a serious interoperability issue.
Attachment #201011 - Attachment mime type: text/html → image/png
Latest Opera also renders attachment 201009 [details] as KHTML
Re: Comment #25

That is exactly how it should render. Ian has confirmed this to me off list that that is what is intended by the specs.

Re: Comment #26

That is the IE Double-Margin float

   http://www.positioniseverything.net/explorer/doubled-margin.html

IE is wrong plain and simple
3 floated blocks, with the leftmost block (negative margined, block 2) being shorter than block 1.

Current Firefox trunk build [Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20051027 Firefox/1.6a1 ID:2005102705] renders the right most column too low.
How does it fare with this patch ?
(In reply to comment #30)
> Created an attachment (id=201071) [edit]
> it looks like this
> 

This looks correct to me, and is the same as what Opera 8.5, Safari 1.3 and iCab 3.0 do.
*** Bug 314274 has been marked as a duplicate of this bug. ***
Comment on attachment 200923 [details] [diff] [review]
better fix

I haven't followed the logic through for the left vs. right edge of negative-width rectangle question, but if it works that's good enough for me (for now, at least).

r+sr=dbaron.  (Sorry for the delay, just catching up after traveling.)
Attachment #200923 - Flags: superreview?(dbaron)
Attachment #200923 - Flags: superreview+
Attachment #200923 - Flags: review?(dbaron)
Attachment #200923 - Flags: review+
Comment on attachment 200923 [details] [diff] [review]
better fix

This fixes a significant layout regression that at least some authors are complaining about. The fix is conservative; it only changes behaviour in situations that were guaranteed to go wrong before. It would be great to get this into 1.8.
Attachment #200923 - Flags: approval1.8rc2?
checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Fantastic. Thanks so much Mozilla people.
It's not fixed for FF1.5 yet, that depends on the FF1.5 triage team.
Just finished building my own 'fox (trunk based). And yay! This works nicely, no regression so far. Thank you Robert.
Can we get a trunk verification - and DBaron can you comment on risk level for taking this late in 1.5 release?   Also - what is the scope of the impact of this bug on web developers.  
Keywords: qawanted
I think comment #38 counts as verification.

> what is the scope of the impact of this bug on web developers.

Maybe the web developers should comment on that...
Status: RESOLVED → VERIFIED
In an important page of web development encounter a tutorial that mentions this bug.

http://www.positioniseverything.net/articles/onetruelayout/

(Sorry my my English is bad)
This is very low risk, since it only affects floats with negative-size margin boxes, which are very rare.
Since you are asking the web developer point of view here is my take ;)

The article mentionned by Fernando has been circulating in all CSS mailing lists and is viewed by many as a solution to several old CSS positionning problems that currently force many webdeveloppers to use a table or javascript.

I hope that if the patch is safe it will be integrated into FF1.5 because it would incitate web designers to use full CSS-P designs more frequently. Good CSS support is one of Gecko's strong points, everytime you convince a web-developper to switch to CSS layouts, they realize how good gecko is in this field and they start promoting Firefox to their clients and colleagues, very good PR to fix this in 1.5 IMHO :)
(In reply to comment #39)
> Can we get a trunk verification 

I've gone been using a trunk version with this patch since it landed on two different machines (mentioned in comment #38); we haven't found any real world site that had problems. All my test-cases on the float property also checkout correctly. If anything, one my sites that uses negative margined floats (but unaffected by this bug) has a better rendering than before (a small rounding error that only occurred at certain window widths).

> what is the scope of the impact of
> this bug on web developers.  

Alex' (rather long) article in comment #9 already makes a convincing case. That article has been doing the rounds on many mailing lists and  webdev oriented sites. Other web-developers are releasing more templates and articles illustrating the benefits of techniques based on negative-margined floats.

The core of Alex article is the creation of css based layouts that are completely independent of the (html) source order of the document: that is the ability to reposition blocks of content in the visual layout while keeping the source-code well organised, with the most important part(s) of the content coming up first in the source. This is a benefit both for the user/visitor and for search engines.

Using those negative-margined float techniques works pretty well across various browsers (from old IE Mac to IE windows, Opera, iCab, Safari, ..). The only rendering engine where those techniques (currently) fail is Gecko 1.8.
Eric Meyer calls the CSS development technique to be enable by fixing this regression "a truly major development (and that's an understatement)", and mentions that we should fix this bug. 

http://meyerweb.com/eric/thoughts/2005/11/01/layout-revolution/

dbaron calls this a "very low risk".

Considering the importance of the technique, it would be a terrible PR move to alienate our fellow Web developers by not fixing this regression.
Flags: blocking1.8rc2?
The article that explains this CSS technique is making its way around the world. That includes the bit that this currently doesn't work in FF1.5, which is pretty bad PR.

roc calls this a "significant layout regression" and the fix is "conservative".
dbaron says it's "very low risk" to include it in 1.8rc2/FF1.5rc2.

That by itself should be enough of a reason to include it, but I'll go on for a bit.

As a web developer, I'd really like to have a go at the new technique because we need a flexible, yet cross-browser CSS layout. But it's important for us to support the current version of browsers so if it won't work on FF1.5, it'll be a no-go.

IMHO, Mozilla has a webdev-friendly reputation. So please keep it that way and include this fix. The web developers will be happy, and regular users won't be able to complain about broken layouts :-)
Attachment #200923 - Flags: approval1.8rc2? → approval1.8rc2+
Checked into branch.
Keywords: fixed1.8
Flags: blocking1.8rc2?
Wonderful, thanks to all :-)
(In reply to comment #48)
> Wonderful, thanks to all :-)
> 

does that mean it will be fixed in 1.5? As a webdev I think it is very important :)
(In reply to comment #49)
> (In reply to comment #48)
> > Wonderful, thanks to all :-)
> > 
> 
> does that mean it will be fixed in 1.5? As a webdev I think it is very
> important :)
> 

Yes, roc's comment that it was checked in on the branch means it will be in 1.5.
You need to log in before you can comment on or make changes to this bug.