182748 - min/max-width/height not considered in computation of auto abs. pos. margins

Status: RESOLVED FIXED

(Core :: Layout: Positioned, defect, P4)

Description

[Stanimir Stamenkov]

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]

Attachment: Absolutely positioned box which should be centered in the browser window

Comment 2

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

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]

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]

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-4, no longer working on Mozilla)]

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]

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-4, no longer working on Mozilla)]

http://www.w3.org/TR/CSS21/visudet.html

Comment 8

[Stanimir Stamenkov]

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 (inactive)]

Taking...

Comment 10

[Mats Palmgren (inactive)]

Taking...

Comment 11

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Comment 12

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

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-4, no longer working on Mozilla)]

Or, rather, you need to base it on direction *if* margin-right is not 'auto'.

Comment 14

[Mats Palmgren (inactive)]

Attachment: Another testcase

Comment 15

[Mats Palmgren (inactive)]

Attachment: Testcase #3 - the overconstrained case

Comment 16

[Mats Palmgren (inactive)]

Comment on attachment 168736 [details]
Testcase #3 - the overconstrained case

No, the description is not correct of what should happen...

Comment 17

[Mats Palmgren (inactive)]

Attachment: Testcase #4, overconstrained, RTL

Comment 18

[Mats Palmgren (inactive)]

Attachment: Testcase #5, overconstrained, LTR

Comment 19

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Comment 20

[Mats Palmgren (inactive)]

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-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

(In particular, I wonder whether we could avoid recalculating things, i.e.,
calculating non-auto things and then auto things.)

Comment 23

[Mats Palmgren (inactive)]

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-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

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.