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

Status: NEW

(Core :: Layout: Positioned, defect)

Description

[Mats Palmgren (inactive)]

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.

Comment 1

[Mats Palmgren (inactive)]

Attachment: Testcase #1

Comment 2

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Comment 3

[Mats Palmgren (inactive)]

Attachment: Testcase #3

Comment 4

[Mats Palmgren (inactive)]

Attachment: Testcase #4

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

Comment 6

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

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?

Comment 7

[Mats Palmgren (inactive)]

(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?

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

Comment 9

[Mats Palmgren (inactive)]

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.

Comment 10

[Mats Palmgren (inactive)]

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...

Comment 11

[Mats Palmgren (inactive)]

Attachment: Testcase #5, over-constrained cases

Comment 12

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2 (diff -w)

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...

Comment 13

[Hixie (not reading bugmail)]

Please make sure to have dbaron look this over.

Comment 14

[Mats Palmgren (inactive)]

Attachment: Testcase #6, width:auto with min-width and one margin:auto

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.

Comment 15

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

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.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 20

[Gérard Talbot]

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

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Those are fixed by bug 419100.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.