Last Comment Bug 312777 - 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 incorrec...
Status: VERIFIED FIXED
: fixed1.8, qawanted, regression, testcase
Product: Core
Classification: Components
Component: Layout: Floats (show other bugs)
: Trunk
: All All
: -- major with 17 votes (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
http://www.fu2k.org/alex/css/bugs/gec...
: 314274 (view as bug list)
Depends on:
Blocks: 276602
  Show dependency treegraph
 
Reported: 2005-10-17 17:14 PDT by Alex Robinson
Modified: 2006-03-12 18:59 PST (History)
29 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
simplified testcase (2.04 KB, text/html)
2005-10-18 17:44 PDT, philippe (part-time)
no flags Details
Simplified testcase (2.04 KB, text/html)
2005-10-25 11:46 PDT, Alex Robinson
no flags Details
Simplified testcase - supercedes previous ones (956 bytes, text/html)
2005-10-25 16:12 PDT, Alex Robinson
no flags Details
fix (3.64 KB, patch)
2005-10-25 21:27 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Splinter Review
better fix (8.55 KB, patch)
2005-10-26 15:01 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
dbaron: review+
dbaron: superreview+
mtschrep: approval1.8rc2+
Details | Diff | Splinter Review
testcase (1011 bytes, text/html)
2005-10-27 10:18 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details
screenshot of attachment 201009 (25.96 KB, image/png)
2005-10-27 10:21 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details
screenshot of attachment 201009 in IE6 (20.60 KB, image/png)
2005-10-27 10:27 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details
Additional testcase, short lefmost column (1.87 KB, text/html)
2005-10-27 16:49 PDT, philippe (part-time)
no flags Details
it looks like this (76.03 KB, image/png)
2005-10-27 18:30 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details

Description Alex Robinson 2005-10-17 17:14:56 PDT
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.
Comment 1 philippe (part-time) 2005-10-18 17:44:20 PDT
Created attachment 200018 [details]
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).
Comment 2 philippe (part-time) 2005-10-18 17:47:08 PDT
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.
Comment 4 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-10-19 03:06:35 PDT
Well, Robert probably changed this in bug 276602. I don't know of any dupes of this.
Comment 5 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-10-21 05:30:19 PDT
Yes, backing out the nsSpaceManager.cpp changes of the patch in bug 276602 fixes
this bug for me.
Comment 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-10-23 13:55:31 PDT
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.
Comment 7 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-10-23 14:19:20 PDT
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)
Comment 8 Alex Robinson 2005-10-25 11:46:22 PDT
Created attachment 200768 [details]
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
Comment 9 Alex Robinson 2005-10-25 12:17:24 PDT
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.
Comment 10 Hixie (not reading bugmail) 2005-10-25 14:06:59 PDT
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.
Comment 11 Alex Robinson 2005-10-25 14:33:50 PDT
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.
Comment 12 Hixie (not reading bugmail) 2005-10-25 15:50:48 PDT
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. :-)
Comment 13 Alex Robinson 2005-10-25 16:12:28 PDT
Created attachment 200807 [details]
Simplified testcase - supercedes previous ones
Comment 14 Alex Robinson 2005-10-25 16:20:23 PDT
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.
Comment 15 Hixie (not reading bugmail) 2005-10-25 16:22:41 PDT
Layout. Not including text flow.
Comment 16 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-10-25 19:53:58 PDT
(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.
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-10-25 21:27:06 PDT
Created attachment 200834 [details] [diff] [review]
fix

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.
Comment 18 Hixie (not reading bugmail) 2005-10-25 21:40:32 PDT
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?
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-10-26 13:49:14 PDT
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.
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-10-26 15:01:57 PDT
Created attachment 200923 [details] [diff] [review]
better fix

Okay, I believe this is the right fix; it's exactly what I said in the previous comment.
Comment 21 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-10-27 05:49:15 PDT
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?
Comment 22 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-10-27 05:59:19 PDT
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
Comment 23 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-10-27 08:17:05 PDT
(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.
Comment 24 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-10-27 10:18:03 PDT
Created attachment 201009 [details]
testcase

This is rougly what David asked for.
Comment 25 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-10-27 10:21:03 PDT
Created attachment 201010 [details]
screenshot of attachment 201009 [details]

This is how it renders. I believe this rendering is correct. KHTML 4.2 gives the same rendering (modulo font choices).
Comment 26 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-10-27 10:27:32 PDT
Created attachment 201011 [details]
screenshot of attachment 201009 [details] in IE6

Not exactly the same as my patch or KHTML. It's hard for me to judge whether this is a serious interoperability issue.
Comment 27 José Jeria 2005-10-27 11:18:36 PDT
Latest Opera also renders attachment 201009 [details] as KHTML
Comment 28 Alex Robinson 2005-10-27 11:26:31 PDT
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
Comment 29 philippe (part-time) 2005-10-27 16:49:51 PDT
Created attachment 201067 [details]
Additional testcase, short lefmost column

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 ?
Comment 30 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-10-27 18:30:24 PDT
Created attachment 201071 [details]
it looks like this
Comment 31 philippe (part-time) 2005-10-27 18:55:58 PDT
(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.
Comment 32 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-10-31 16:37:40 PST
*** Bug 314274 has been marked as a duplicate of this bug. ***
Comment 33 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-10-31 16:57:06 PST
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.)
Comment 34 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-10-31 17:07:31 PST
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.
Comment 35 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-10-31 17:18:14 PST
checked into trunk.
Comment 36 Alex Robinson 2005-10-31 17:48:19 PST
Fantastic. Thanks so much Mozilla people.
Comment 37 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-10-31 19:13:29 PST
It's not fixed for FF1.5 yet, that depends on the FF1.5 triage team.
Comment 38 philippe (part-time) 2005-10-31 20:04:02 PST
Just finished building my own 'fox (trunk based). And yay! This works nicely, no regression so far. Thank you Robert.
Comment 39 Mike Schroepfer 2005-11-01 14:33:08 PST
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.  
Comment 40 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-11-01 14:45:41 PST
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...
Comment 41 Fernando García Gómez, stripTM 2005-11-01 16:41:37 PST
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)
Comment 42 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-11-01 16:53:12 PST
This is very low risk, since it only affects floats with negative-size margin boxes, which are very rare.
Comment 43 Pascal Chevrel:pascalc(PTO until Sept 2) 2005-11-01 17:02:36 PST
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 :)
Comment 44 philippe (part-time) 2005-11-01 17:27:03 PST
(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.
Comment 45 Tristan Nitot 2005-11-02 04:18:25 PST
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.
Comment 46 Hans Melis 2005-11-02 08:03:55 PST
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 :-)
Comment 47 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-11-02 11:53:16 PST
Checked into branch.
Comment 48 Fernando García Gómez, stripTM 2005-11-02 12:01:33 PST
Wonderful, thanks to all :-)
Comment 49 dagomar 2005-11-03 05:53:03 PST
(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 :)
Comment 50 Jon Henry 2005-11-03 06:16:13 PST
(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.

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