I'm looking at the code that implements CSS2.1 10.3.7 and 10.6.4 and it doesn't look correct to me, for the case when the offsets and width/height are all non-auto (or, the width/height is auto but is constrained by a min/max value, which I interpret as the same situation). Here are the paragraphs from the spec I'm referring to: http://www.w3.org/TR/CSS21/visudet.html#abs-non-replaced-width "If none of the three [left,width,right] is 'auto': If both 'margin-left' and 'margin-right' are 'auto', solve the equation under the extra constraint that the two margins get equal values, unless this would make them negative, in which case when direction of the containing block is 'ltr' ('rtl'), set 'margin-left' ('margin-right') to zero and solve for 'margin-right' ('margin-left'). If one of 'margin-left' or 'margin-right' is 'auto', solve the equation for that value. If the values are over-constrained, ignore the value for 'left' (in case the 'direction' property of the containing block is 'rtl') or 'right' (in case 'direction' is 'ltr') and solve for that value." Problems in our implementation: A1. we treat some cases as over-constrained even though there is a margin that is 'auto'. A2. we don't have any logic to take care of the "unless this would make them negative" case above. http://www.w3.org/TR/CSS21/visudet.html#abs-non-replaced-height "If none of the three [top,height,bottom] are 'auto': If both 'margin-top' and 'margin-bottom' are 'auto', solve the equation under the extra constraint that the two margins get equal values. If one of 'margin-top' or 'margin-bottom' is 'auto', solve the equation for that value. If the values are over-constrained, ignore the value for 'bottom' and solve for that value." B1. as A1 B2. as A2, but note that the spec does not have a similar clause for the vertical case - I think this is a mistake in the spec. (The current Gecko code actually avoids making the margins negative (due to B1), which isn't compliant with the current spec.) Setting 'margin-top' to zero and solving for 'margin-bottom' seems like the correct algorithm to me.

You're looking at this code on the reflow branch, right?

1. When there is at least one margin that is 'auto', adjust that to satisfy the equation. If both are 'auto' and they would be negative then set one to zero and let the other be negative only. (this implies that we are never over-constrained unless both offsets and margins and width/height are non-auto (or width/height was 'auto' but was constrained by a min/max value)). 2. Avoid making both margin-top and margin-bottom negative when they are both 'auto' (as for the horisontal case), even though the spec says otherwise. Comments?

(In reply to comment #5) > You're looking at this code on the reflow branch, right? > This is for trunk, what's the branch tag for the reflow branch?

http://wiki.mozilla.org/Gecko:Reflow_Refactoring#Build_Instructions

I tested REFLOW_20060302_BRANCH and it renders the testcases the same as trunk. I looked briefly at the code and it seems to be identical to trunk for the parts I touched. Applying the patch gave the same rendering as trunk+patch.

BTW, shouldn't we subtract mComputedMargin.top/bottom here: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsHTMLReflowState.cpp&rev=1.237&root=/cvsroot&mark=1324-1326#1316 Like we do here: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsHTMLReflowState.cpp&rev=1.237&root=/cvsroot&mark=1406-1410#1383 and similarly for mComputedMargin.left/right here: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsHTMLReflowState.cpp&rev=1.237&root=/cvsroot&mark=1114-1119#1102 compared with: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsHTMLReflowState.cpp&rev=1.237&root=/cvsroot&mark=1224-1233#1197 It doesn't look like both blocks can be right to me...

Addressing comment 10, I'm pretty sure we should subtract the margin in both places. This makes the two blocks where we adjust margins/offset identical, so it could be worth breaking out into a separate method to make the code more readable and easier to maintain...

Please make sure to have dbaron look this over.

I added assertions to make sure InitAbsoluteConstraints() satisfies CSS2.1 and found one more error in the existing code (mea culpa, bug 182748 tried to fix this but it fails for width:auto and exactly one of the margins is auto in the case where the desired width is limited by min- or max-width). In Testcase #6, the pink block have width:auto and it should be 140px to satisfy 10.3.7, but min-width is 150px so the computed width is 150px and we need to adjust margin-left which was auto (it should become -10px). The bug in the code is in 'availMarginSpace = autoWidth - mComputedWidth' at line 1207. http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsHTMLReflowState.cpp&rev=1.237&root=/cvsroot&mark=1207#1197 What 'availMarginSpace' really is is how much *additional* margin space we got when limiting the width to min-/max-width. (in this case -10px, so we end up setting "margin.left = -10px - 50px" at line 1215. (Since we have left:10px the block is 50px left of the blue line.) In the case we had two auto-margins it doesn't matter, since they would be zero at this point and if we had two non-auto margins it doesn't matter either since it falls into the over-constrained case. I guess we could optimize the code for this particular case (just assign the additional margin to the one that was auto), but I prefer to have common code for the two places where this calculation is needed. I'm guessing this case is very rare in practice.

This patch breaks out the common code for the auto-margin/ over-constrained calculations into separate methods. This fixes the bug mentioned in the last comment. I also cleaned up the InitAbsoluteConstraints() code slightly.

So what's the state of this post-reflow-branch? I think a good bit of the complexity here is no longer needed, and I simplified InitAbsoluteConstraints a good bit on the branch.

Mats, I think I fixed a lot of bugs here on the reflow branch, but I have no idea what the expected results for those testcases are. Is there anything that still needs fixing here?

Hrm. It looks like applying the above patch to the 1.8 branch yields changes that I didn't make on the reflow branch. Not sure what's correct, though.

Comment on attachment 224264 [details] [diff] [review] Patch rev. 3 Marking review- since the patch doesn't apply anymore. It would be nice to know whether there are still valid bugs here, though.

Firefox 3.6.x and Firefox 4.0 fail these testcases: http://test.csswg.org/suites/css2.1/20110111/html4/absolute-non-replaced-width-008.htm http://test.csswg.org/suites/css2.1/20110111/html4/absolute-non-replaced-width-009.htm regards, Gérard

Those are fixed by bug 419100.

(I think the parts of this bug that weren't fixed on the reflow branch are fixed by bug 419100.)