Last Comment Bug 339978 - Problems with auto-margins, over-constrained etc (CSS2.1 10.3.7 and 10.6.4)
: Problems with auto-margins, over-constrained etc (CSS2.1 10.3.7 and 10.6.4)
Status: NEW
[reflow-refactor]
: css2, testcase
Product: Core
Classification: Components
Component: Layout: R & A Pos (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: ---
Assigned To: Mats Palmgren (:mats)
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-06-01 09:49 PDT by Mats Palmgren (:mats)
Modified: 2011-04-27 17:38 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase #1 (3.87 KB, text/html)
2006-06-01 09:56 PDT, Mats Palmgren (:mats)
no flags Details
Testcase #2 (3.89 KB, text/html)
2006-06-01 09:56 PDT, Mats Palmgren (:mats)
no flags Details
Testcase #3 (3.98 KB, text/html)
2006-06-01 09:57 PDT, Mats Palmgren (:mats)
no flags Details
Testcase #4 (3.82 KB, text/html)
2006-06-01 09:58 PDT, Mats Palmgren (:mats)
no flags Details
Patch rev. 1 (10.28 KB, patch)
2006-06-01 10:11 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
Testcase #5, over-constrained cases (6.31 KB, text/html)
2006-06-01 16:13 PDT, Mats Palmgren (:mats)
no flags Details
Patch rev. 2 (diff -w) (10.43 KB, patch)
2006-06-01 16:34 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
Testcase #6, width:auto with min-width and one margin:auto (1.27 KB, text/html)
2006-06-02 16:29 PDT, Mats Palmgren (:mats)
no flags Details
Patch rev. 3 (20.02 KB, patch)
2006-06-02 16:34 PDT, Mats Palmgren (:mats)
dbaron: review-
Details | Diff | Splinter Review

Description Mats Palmgren (:mats) 2006-06-01 09:49:26 PDT
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 (:mats) 2006-06-01 09:56:17 PDT
Created attachment 224081 [details]
Testcase #1
Comment 2 Mats Palmgren (:mats) 2006-06-01 09:56:48 PDT
Created attachment 224082 [details]
Testcase #2
Comment 3 Mats Palmgren (:mats) 2006-06-01 09:57:18 PDT
Created attachment 224083 [details]
Testcase #3
Comment 4 Mats Palmgren (:mats) 2006-06-01 09:58:12 PDT
Created attachment 224084 [details]
Testcase #4
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2006-06-01 10:09:49 PDT
You're looking at this code on the reflow branch, right?
Comment 6 Mats Palmgren (:mats) 2006-06-01 10:11:27 PDT
Created attachment 224086 [details] [diff] [review]
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 (:mats) 2006-06-01 10:16:42 PDT
(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 [:bz] (still a bit busy) 2006-06-01 10:31:07 PDT
http://wiki.mozilla.org/Gecko:Reflow_Refactoring#Build_Instructions
Comment 9 Mats Palmgren (:mats) 2006-06-01 13:17:52 PDT
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 11 Mats Palmgren (:mats) 2006-06-01 16:13:41 PDT
Created attachment 224129 [details]
Testcase #5, over-constrained cases
Comment 12 Mats Palmgren (:mats) 2006-06-01 16:34:48 PDT
Created attachment 224132 [details] [diff] [review]
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) 2006-06-01 19:57:18 PDT
Please make sure to have dbaron look this over.
Comment 14 Mats Palmgren (:mats) 2006-06-02 16:29:34 PDT
Created attachment 224261 [details]
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 (:mats) 2006-06-02 16:34:59 PDT
Created attachment 224264 [details] [diff] [review]
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-10 2006-12-12 10:35:01 PST
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-10 2007-03-14 11:43:46 PDT
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-10 2007-03-14 12:08:25 PDT
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-10 2007-04-05 12:45:36 PDT
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 21 David Baron :dbaron: ⌚️UTC-10 2011-03-22 14:30:53 PDT
Those are fixed by bug 419100.
Comment 22 David Baron :dbaron: ⌚️UTC-10 2011-03-22 14:32:55 PDT
(I think the parts of this bug that weren't fixed on the reflow branch are fixed by bug 419100.)

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