Open Bug 339978 Opened 16 years ago Updated 11 years ago

Problems with auto-margins, over-constrained etc (CSS2.1 10.3.7 and 10.6.4)

Categories

(Core :: Layout: Positioned, defect)

defect
Not set
normal

Tracking

()

People

(Reporter: mats, Assigned: mats)

Details

(Keywords: css2, testcase, Whiteboard: [reflow-refactor])

Attachments

(7 files, 2 obsolete files)

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.
Attached file Testcase #1
Attached file Testcase #2
Attached file Testcase #3
Attached file Testcase #4
You're looking at this code on the reflow branch, right?
Attached patch Patch rev. 1 (obsolete) — Splinter Review
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?
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch Patch rev. 2 (diff -w) (obsolete) — Splinter Review
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...
Attachment #224086 - Attachment is obsolete: true
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.
Attached patch Patch rev. 3Splinter Review
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.
Attachment #224132 - Attachment is obsolete: true
Attachment #224264 - Flags: review?(dbaron)
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.
Whiteboard: [reflow-refactor]
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.
Attachment #224264 - Flags: review?(dbaron) → review-
Blocks: css2.1-tests
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.)
No longer blocks: css2.1-tests
You need to log in before you can comment on or make changes to this bug.