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)
Core
Layout: Floats
Tracking
()
VERIFIED
FIXED
People
(Reporter: mozilla.org, Assigned: roc)
References
()
Details
(4 keywords)
Attachments
(9 files, 1 obsolete file)
2.04 KB,
text/html
|
Details | |
2.04 KB,
text/html
|
Details | |
956 bytes,
text/html
|
Details | |
8.55 KB,
patch
|
dbaron
:
review+
dbaron
:
superreview+
mtschrep
:
approval1.8rc2+
|
Details | Diff | Splinter Review |
1011 bytes,
text/html
|
Details | |
25.96 KB,
image/png
|
Details | |
20.60 KB,
image/png
|
Details | |
1.87 KB,
text/html
|
Details | |
76.03 KB,
image/png
|
Details |
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•19 years ago
|
||
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•19 years ago
|
||
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
Comment 3•19 years ago
|
||
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&filetype=match&whotype=match&sortby=Date&hours=2&date=explicit&mindate=20050306+0500&maxdate=20050307+0800&cvsroot=%2Fcvsroot Martijn, you know if this is a dupe or not ?
Keywords: regression
Comment 4•19 years ago
|
||
Well, Robert probably changed this in bug 276602. I don't know of any dupes of this.
Updated•19 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 5•19 years ago
|
||
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)
Reporter | ||
Comment 8•19 years ago
|
||
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
Reporter | ||
Comment 9•19 years ago
|
||
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•19 years ago
|
||
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.
Reporter | ||
Comment 11•19 years ago
|
||
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•19 years ago
|
||
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. :-)
Reporter | ||
Comment 13•19 years ago
|
||
Reporter | ||
Comment 14•19 years ago
|
||
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•19 years ago
|
||
Layout. Not including text flow.
Assignee | ||
Comment 16•19 years ago
|
||
(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.
Assignee | ||
Comment 17•19 years ago
|
||
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
Comment 18•19 years ago
|
||
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?
Assignee | ||
Comment 19•19 years ago
|
||
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.
Assignee | ||
Comment 20•19 years ago
|
||
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
Assignee | ||
Comment 23•19 years ago
|
||
(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.
Assignee | ||
Comment 24•19 years ago
|
||
This is rougly what David asked for.
Assignee | ||
Comment 25•19 years ago
|
||
This is how it renders. I believe this rendering is correct. KHTML 4.2 gives the same rendering (modulo font choices).
Assignee | ||
Comment 26•19 years ago
|
||
Not exactly the same as my patch or KHTML. It's hard for me to judge whether this is a serious interoperability issue.
Assignee | ||
Updated•19 years ago
|
Attachment #201011 -
Attachment mime type: text/html → image/png
Comment 27•19 years ago
|
||
Latest Opera also renders attachment 201009 [details] as KHTML
Reporter | ||
Comment 28•19 years ago
|
||
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•19 years ago
|
||
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 ?
Assignee | ||
Comment 30•19 years ago
|
||
Comment 31•19 years ago
|
||
(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+
Assignee | ||
Comment 34•19 years ago
|
||
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?
Assignee | ||
Comment 35•19 years ago
|
||
checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 36•19 years ago
|
||
Fantastic. Thanks so much Mozilla people.
Assignee | ||
Comment 37•19 years ago
|
||
It's not fixed for FF1.5 yet, that depends on the FF1.5 triage team.
Comment 38•19 years ago
|
||
Just finished building my own 'fox (trunk based). And yay! This works nicely, no regression so far. Thank you Robert.
Comment 39•19 years ago
|
||
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
Assignee | ||
Comment 40•19 years ago
|
||
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
Comment 41•19 years ago
|
||
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.
Comment 43•19 years ago
|
||
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•19 years ago
|
||
(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•19 years ago
|
||
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.
Updated•19 years ago
|
Flags: blocking1.8rc2?
Comment 46•19 years ago
|
||
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 :-)
Updated•19 years ago
|
Attachment #200923 -
Flags: approval1.8rc2? → approval1.8rc2+
Assignee | ||
Updated•19 years ago
|
Flags: blocking1.8rc2?
Comment 48•19 years ago
|
||
Wonderful, thanks to all :-)
Comment 49•19 years ago
|
||
(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•19 years ago
|
||
(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.
Description
•