Last Comment Bug 182748 - min/max-width/height not considered in computation of auto abs. pos. margins
: min/max-width/height not considered in computation of auto abs. pos. margins
Status: RESOLVED FIXED
[csswg]
: testcase
Product: Core
Classification: Components
Component: Layout: R & A Pos (show other bugs)
: Trunk
: All All
: P4 normal (vote)
: ---
Assigned To: Mats Palmgren (vacation)
: Hixie (not reading bugmail)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2002-11-29 18:28 PST by Stanimir Stamenkov
Modified: 2006-10-19 23:20 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Absolutely positioned box which should be centered in the browser window (1.13 KB, text/html)
2002-11-29 18:32 PST, Stanimir Stamenkov
no flags Details
Patch rev. 1 (3.30 KB, patch)
2004-02-28 17:03 PST, Mats Palmgren (vacation)
dbaron: review+
Details | Diff | Splinter Review
Another testcase (817 bytes, text/html)
2004-12-05 15:21 PST, Mats Palmgren (vacation)
no flags Details
Testcase #3 - the overconstrained case (1.32 KB, text/html)
2004-12-14 18:40 PST, Mats Palmgren (vacation)
no flags Details
Testcase #4, overconstrained, RTL (1.29 KB, text/html)
2004-12-15 17:56 PST, Mats Palmgren (vacation)
no flags Details
Testcase #5, overconstrained, LTR (1.29 KB, text/html)
2004-12-15 17:57 PST, Mats Palmgren (vacation)
no flags Details
Patch rev. 2 (7.62 KB, patch)
2004-12-15 18:56 PST, Mats Palmgren (vacation)
dbaron: review+
dbaron: superreview+
Details | Diff | Splinter Review

Description Stanimir Stamenkov 2002-11-29 18:28:20 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3a) Gecko/20021119
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3a) Gecko/20021119

It seems Mozilla doesn't compute the width, height and the margins of an
absolutely positioned element correctly. More precisely it fails to compute the
correct margin widths. The case is when the 'width' and/or 'height' and the
margins of such an element (absolutely positioned) have a value of 'auto'. I'll
attach a test case to clarify the problem.

I have an absolutely positioned block of content which I wanted to center both
horizontally and vertically in the containing block. The block itself doesn't
get 'width' and 'height' explicitly (they have value of 'auto') but has
'min-width', 'max-width', 'min-height' and 'max-height' to constrain it. The
'top', 'left', 'bottom' and 'right' properties are set to 0 (zero). And the
margins are set to 'auto'.

According to the CSS2 spec "Computing widths and margins" and "Computing heights
and margins" first the width and the height should take all the available space
while 'top', 'left', 'bottom' and 'right' have value of 0 and the margins are
'auto'. Therefore the margins get 0 (zero) width. But next the width and height
are constrained to their 'max-' properties. It seems at this point the
computation of the widths and margins is not done again with the already known
values of 'width' and 'height' (which are substituted with their 'max-' values)
and the top and left (the 'direction' doesn't seem to affect) margin remain 0.

If I specify 'width' and 'height' explicitly the box gets centered.

P.S.: I'm reporting this with older Mozilla but it happens with the 20021130
build too.

Reproducible: Always

Steps to Reproduce:
1. Open the attachment. The box is top-left aligned.
2. Comment 'width: auto' and/or 'height: auto' and uncomment corresponding exact
declaration in the style definition.
3. The box get centered.
Comment 1 Stanimir Stamenkov 2002-11-29 18:32:09 PST
Created attachment 107795 [details]
Absolutely positioned box which should be centered in the browser window
Comment 2 David Baron :dbaron: ⌚️UTC+1 (mostly busy through August 4; review requests must explain patch) 2002-11-29 19:31:59 PST
If I'm reading this correctly (and I did read quickly), the problem is that
min-width and max-width aren't being considered in the calculation of auto
margins.  This seems like a problem not only with our implementation but with
the spec as well.
Comment 3 Stanimir Stamenkov 2002-11-30 06:30:56 PST
After the "Computing widths and margins" and "Computing heights and margins"
(respectively) chapters there are sections about 'min-width', 'max-width' and
'min-height', 'max-height' (respectively). These sections describe (this is for
the width):

1. The width is computed (without 'min-width' and 'max-width') following the
rules under "Computing widths and margins" above.
2. If the computed value of 'min-width' is greater than the value of
'max-width', 'max-width' is set to the value of 'min-width'.
3. If the computed width is greater than 'max-width', the rules above are
applied again, but this time using the value of 'max-width' as the specified
value for 'width'.
4. If the computed width is smaller than 'min-width', the rules above are
applied again, but this time using the value of 'min-width' as the specified
value for 'width'.

The height rules are identical - the computation must be done again with the new
values for 'width' and 'height'.
Comment 4 Stanimir Stamenkov 2002-12-01 05:38:50 PST
The thing I couldn't get clear from the spec is about computing the width and
height of absolutely positioned non-replaced elements in the following situation:

There are no cases described where either the width or height gets intrinsic
value like the replaced elements. This way specifying the width either
explicitly with the 'width' property or constrained with 'min-max-width' but not
specifying a height (i.e. its initial value is 'auto') there's no way the height
could receive intrinsic value to take only as much place as the inline content
needs and therefore to take place in the computation with this value.

And with the example case I've attached there's no way to make the box centered,
to take minimum 20em and maximum 30em width (or just 'width: 60%') and the
height to accommodate to fit the inline text. If no height is specified in any
way ('height: auto; min-height: 0; max-height: none') the height will take the
whole available space according to the algorithm given in the spec (and this is
what we see now, this is not broken).


I felt like I must make this clarification because it is part of the whole
widths/heights and margins computation algorithm.
Comment 5 David Baron :dbaron: ⌚️UTC+1 (mostly busy through August 4; review requests must explain patch) 2002-12-01 07:37:05 PST
Are you looking at the algorithm in CSS 2.1?  (Also, please don't discuss issues
unrelated to bug reports on the bug.  It just makes the bug more confusing to
understand.  Use newsgroups:  http://mozilla.org/community.html#mozilla-layout .)
Comment 6 Stanimir Stamenkov 2002-12-01 08:00:17 PST
I'm looking at "Visual formatting model details"
<http://www.w3.org/TR/REC-CSS2/visudet.html>. I'm not sure it is CSS 2.1 but
haven't been able to find any references to it.
Comment 7 David Baron :dbaron: ⌚️UTC+1 (mostly busy through August 4; review requests must explain patch) 2002-12-01 08:07:57 PST
http://www.w3.org/TR/CSS21/visudet.html
Comment 8 Stanimir Stamenkov 2002-12-01 09:07:15 PST
E.absbox {
  position: absolute;
  left: 0;
  right: 0;
  min-width: 20em;
  max-width: 30em;
}

The rules are the same ("[[" and "]]" denote my comments):

"10.3.7 Absolutely positioned, non-replaced elements"
<http://www.w3.org/TR/CSS21/visudet.html#abs-non-replaced-width>

[[
    'margin-left' and 'margin-right' become 0 then applies:
]]

   5. 'width' is 'auto', 'left' and 'right' are not 'auto', then solve for 'width'

[[
    I agree here doesn't get clear enough but in the next section:
]]

"10.4 Minimum and maximum widths: 'min-width' and 'max-width'"
<http://www.w3.org/TR/CSS21/visudet.html#min-max-widths>

The following algorithm describes how the two properties influence the computed
value of the 'width' property:

   1. The width is computed (without 'min-width' and 'max-width') following the
rules under "Computing widths and margins" above.
   2. If the computed value of 'min-width' is greater than the value of
'max-width', 'max-width' is set to the value of 'min-width'.
   3. If the computed width is greater than 'max-width', the rules above are
applied again, but this time using the value of 'max-width' as the specified
value for 'width'.
   4. If the computed width is smaller than 'min-width', the rules above are
applied again, but this time using the value of 'min-width' as the specified
value for 'width'.

[[
    The spec is clear enough that the computation must be done again with either
'max-width' or 'min-width' as the specified value of 'width'.
]]
Comment 9 Mats Palmgren (vacation) 2004-02-28 17:01:50 PST
Taking...
Comment 10 Mats Palmgren (vacation) 2004-02-28 17:02:47 PST
Taking...
Comment 11 Mats Palmgren (vacation) 2004-02-28 17:03:15 PST
Created attachment 142565 [details] [diff] [review]
Patch rev. 1
Comment 12 David Baron :dbaron: ⌚️UTC+1 (mostly busy through August 4; review requests must explain patch) 2004-03-11 21:11:53 PST
Comment on attachment 142565 [details] [diff] [review]
Patch rev. 1

>Index: layout/html/base/src/nsHTMLReflowState.cpp
>+        if (autoWidth != mComputedWidth) {
>+          // Re-calculate any 'auto' margin values since the computed width
>+          // was adjusted by a 'min-width' or 'max-width'.
>+          PRInt32 availMarginSpace = autoWidth - mComputedWidth;
>+
>+          if (eStyleUnit_Auto == mStyleMargin->mMargin.GetLeftUnit()) {
>+            if (eStyleUnit_Auto == mStyleMargin->mMargin.GetRightUnit()) {
>+              // Both margins are 'auto' so their computed values are equal
>+              mComputedMargin.left = availMarginSpace / 2;
>+              mComputedMargin.right = availMarginSpace - mComputedMargin.left;
>+            } else {
>+              mComputedMargin.left = availMarginSpace - mComputedMargin.right;
>+            }
>+          } else {
>+            mComputedMargin.right = availMarginSpace - mComputedMargin.left;

You need to base the contents of this section on 'direction'.

>+          }
>+        }

With that, r=dbaron, although I suspect the spec should probably be changed to
add the paragraph that's in 10.3.3:

  If 'width' is not 'auto' and 'border-left-width' +
  'padding-left' + 'width' + 'padding-right' +
  'border-right-width' (plus any of 'margin-left' or
  'margin-right' that are not 'auto') is larger than
  the width of the containing block, then any 'auto'
  values for 'margin-left' or 'margin-right' are, for
  the following rules, treated as zero.

to other places.

Sorry for the delay in getting to this.
Comment 13 David Baron :dbaron: ⌚️UTC+1 (mostly busy through August 4; review requests must explain patch) 2004-03-11 21:12:28 PST
Or, rather, you need to base it on direction *if* margin-right is not 'auto'.
Comment 14 Mats Palmgren (vacation) 2004-12-05 15:21:26 PST
Created attachment 167961 [details]
Another testcase
Comment 15 Mats Palmgren (vacation) 2004-12-14 18:40:14 PST
Created attachment 168736 [details]
Testcase #3 - the overconstrained case
Comment 16 Mats Palmgren (vacation) 2004-12-14 18:59:12 PST
Comment on attachment 168736 [details]
Testcase #3 - the overconstrained case

No, the description is not correct of what should happen...
Comment 17 Mats Palmgren (vacation) 2004-12-15 17:56:56 PST
Created attachment 168822 [details]
Testcase #4, overconstrained, RTL
Comment 18 Mats Palmgren (vacation) 2004-12-15 17:57:37 PST
Created attachment 168823 [details]
Testcase #5, overconstrained, LTR
Comment 19 Mats Palmgren (vacation) 2004-12-15 18:56:58 PST
Created attachment 168828 [details] [diff] [review]
Patch rev. 2
Comment 20 Mats Palmgren (vacation) 2005-01-16 08:47:12 PST
David, this is fairly straight-forward patch, any chance you can have look
before 1.8b? should I ask someone else?
Comment 21 David Baron :dbaron: ⌚️UTC+1 (mostly busy through August 4; review requests must explain patch) 2005-01-24 11:59:30 PST
Comment on attachment 168828 [details] [diff] [review]
Patch rev. 2

This seems like more code than should be needed to do this, but rubber-stamp
r+sr=dbaron
Comment 22 David Baron :dbaron: ⌚️UTC+1 (mostly busy through August 4; review requests must explain patch) 2005-01-24 12:00:16 PST
(In particular, I wonder whether we could avoid recalculating things, i.e.,
calculating non-auto things and then auto things.)
Comment 23 Mats Palmgren (vacation) 2005-01-24 17:10:30 PST
Hm, I don't see any obvious simplifications. If AdjustComputedWidth/Height
actually changes the computed width/height, then margins (or offsets) have to
be adjusted. Feel free to file a new bug on me if you have any specific
suggestions.

Checked in 2005-01-24 15:24 PDT.

-> FIXED
Comment 24 David Baron :dbaron: ⌚️UTC+1 (mostly busy through August 4; review requests must explain patch) 2006-10-19 18:49:22 PDT
I'm thinking that this may not have done bad things for absolutely positioned replaced elements, although perhaps not.  I also think re-calculate might be the wrong term, since I don't think they were ever calculated before -- they were just left as zero.
Comment 25 David Baron :dbaron: ⌚️UTC+1 (mostly busy through August 4; review requests must explain patch) 2006-10-19 23:20:11 PDT
Anyway, understood now, and rewritten on the reflow branch -- by using the same code whether or not widthIsAuto (and then whether or not heightIsAuto).  And I think there were some bugs here for replaced elements -- and also some bugs in the other codepath (now merged) for one margin auto and one a nonzero length.

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