53259 - min-width and max-width not supported on form controls

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[rods (gone)]

Just as the summary says

Comment 1

[rods (gone)]

I think this really needsa to be fixed, both because it is a regression and 
because it is a pretty basic CSS1 feature.

Comment 2

[rods (gone)]

Attachment: testcase

Comment 3

[buster]

Rod: I must be missing something.  What exactly isn't working in the attached 
test case?

Comment 4

[rods (gone)]

As we talked on the phone the text input at the bottom

Comment 5

[buster]

raising to P2 since this is a CSS1 conformance issue

Comment 6

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

Min and max width are CSS2.  Is it only in some cases that they still no longer
work?

Comment 7

[buster]

My thoughts exactly, David...
Rod: I just want to verify: as far as you know, text controls are the only
element not respecting min-width, right? The summary makes it sound like a
system-wide regression, but the div's in your test case work.

Comment 8

[buster]

I'm confused about how this could be a regression.  I did a search through all
the source code in mozilla/layout/html/forms/src, and I see no references to
mComputedMinWidth or mComputedMaxWidth.  This tells me that none of the form
control frames account for min or max width at all, or if they do they are doing
it totally the wrong way.

I'd hate to go out with partial support for min and max width.  The block code
supports it, so anything based on area|block frame gets it for free.  Other
frames need to be checked.

Could somebody whip up a quick test case that applied min/max to all the various
html elements, so we could validate our (lack of) support?  Ian or David, this
sounds right up your alley if you have a few minutes.

Comment 9

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

Our support for *lots* of things in all those frames is probably very broken. 
That's been one issue in some of the "redesigning layout" discussions...

Comment 10

[rods (gone)]

Attachment: testcase show no form controls obey min-width/maxwidth

Comment 11

[rods (gone)]

maybe it isn't a regression, I thought this used to work. What aren't the form 
controls doing that they should?

Comment 12

[buster]

it's a CSS2-ism, so we aren't technically bound to support it.  However, the
block code does support it, and I'd hate to turn that off.  If it's easy to get
in, we should consider gettting it in. Rod, can you estimate how much work we're
talking about?  Basically, the code template would simply be to do whatever we
already do to determine the size of the form controls, then do a quick compare
against nsHTMLReflowState.mComputedMinWidth and mComputedMaxWidth (and for extra
credit, mComputedMinHeight and mComputedMaxHeight).  So we should only need to
add a couple of comparisons per reflow method.  The complications could include:
1) some form controls might have multiple reflow code paths (text control looks
like it does different things based on quirks vs. standard mode, for example)
2) some form elements are containers:  we'd have to think about what the right
thing to do with them is.  For example, what does a min-width on a file upload
control mean?

Comment 13

[Eric Vaughan]

As I remember I thought the nsHTMLReflowState was supposed to look at the min 
and max sizes and set the mComputed 
appropriately. Perhaps that got broken. Boxes will obey this because they size 
just like regular frames based on mComputed.

If this is not the case I can easily fix boxes to look at the mComputedMax and 
Min.

-E

Comment 14

[rods (gone)]

I now have input button and HTML 4 button working.

Comment 15

[Eric Vaughan]

I have a fix. That fixes the <input> as well as anyone else subclassed off boxes 
in the HTML world. This will make boxes adhear to reflowstate min and max sizes.

Index: nsBoxFrame.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/xul/base/src/nsBoxFrame.cpp,v
retrieving revision 1.128
diff -r1.128 nsBoxFrame.cpp
766a767,779
>   // handle reflow state min and max sizes
>   if (computedSize.width < aReflowState.mComputedMinWidth)
>     computedSize.width = aReflowState.mComputedMinWidth;
>
>   if (computedSize.height < aReflowState.mComputedMinHeight)
>     computedSize.height = aReflowState.mComputedMinHeight;
>
>    if (computedSize.width > aReflowState.mComputedMaxWidth)
>     computedSize.width = aReflowState.mComputedMaxWidth;
>
>    if (computedSize.height > aReflowState.mComputedMaxHeight)
>     computedSize.height = aReflowState.mComputedMaxHeight;
>

Comment 16

[buster]

r=buster.  looks fine, eric

Comment 17

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

It seems backwards to me -- I think you should be doing the max tests before the 

min tests, because min should override max (according to my interpretation of 

CSS2 10.4 and 10.7):

http://www.w3.org/TR/REC-CSS2/visudet.html#min-max-widths

http://www.w3.org/TR/REC-CSS2/visudet.html#min-max-heights



Also, "diff -u" output would be nice if someone wants to apply or compare the 

patch to a different version of the file.

Comment 18

[rods (gone)]

Attachment: patch file for form controls

Comment 19

[rods (gone)]

The patch above fixes button, html 4 button, selects (combo & listbox)
eric has added the patch for text field/area

Comment 20

[Eric Vaughan]

We can do max before min doesn't matter to me. Here is the diff with -u

S:\mozilla\layout\xul\base\src>cvs diff -u nsBoxFrame.cpp
Index: nsBoxFrame.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/xul/base/src/nsBoxFrame.cpp,v
retrieving revision 1.128
diff -u -r1.128 nsBoxFrame.cpp
--- nsBoxFrame.cpp      2000/09/20 01:02:04     1.128
+++ nsBoxFrame.cpp      2000/10/03 19:11:39
@@ -764,6 +764,20 @@
     computedSize.height += m.top + m.bottom;
   }

+  // handle reflow state min and max sizes
+
+  if (computedSize.width > aReflowState.mComputedMaxWidth)
+    computedSize.width = aReflowState.mComputedMaxWidth;
+
+  if (computedSize.height > aReflowState.mComputedMaxHeight)
+    computedSize.height = aReflowState.mComputedMaxHeight;
+
+  if (computedSize.width < aReflowState.mComputedMinWidth)
+    computedSize.width = aReflowState.mComputedMinWidth;
+
+  if (computedSize.height < aReflowState.mComputedMinHeight)
+    computedSize.height = aReflowState.mComputedMinHeight;
+
   nsRect r(mRect.x, mRect.y, computedSize.width, computedSize.height);

   SetBounds(state, r);

Comment 21

[buster]

removing regression keyword, this never worked.
adding rtm keyword.  2 partial patches are attached, a=buster.  eric and rod,
can you review each other's portion of the patch, and Rod please attach a single
patch that incorportates both sets of changes?  Be sure we're giving precedence
min-width over max-width.

PDT: although the diffs look big, they are very simple math.  No flow of control
change, no change to allocation patterns, no interesting logic added, etc.
Without this fix, we have only partial support for min and max width.  Core
layout already support min and max width, so these CSS properties work on P,
DIV, TABLE, HR, IMG, etc.  But unless we get this checked in, min and max width
won't work on form controls, an inconsistancy that's hard to justify given the
low risk and simplicity of the fix.

Comment 22

[Eric Vaughan]

r=evaughan for Rods changes.

Comment 23

[buster]

a=buster
rod, can you get this all checked in if PDT approves?
PDT: don't be put off by the size of the patch:  it's very simple and repetitive
code, with no scary changes.

karnaze: rtm+ por favor

Comment 24

[karnaze (gone)]

Marking rtm+, although PDT may not be taking this kind of P2 fix any longer. The 
change is local to certain form controls which (if tested adequately) is 
probably low risk.

Comment 25

[Phil Peterson]

Unless this causes some top pages to behave really badly, this is [rtm-]

Comment 26

[Hixie (not reading bugmail)]

Upon managerial request, adding the "testcase" keyword to 84 open layout bugs that
do not have the "testcase" keyword and yet have an attachement with the word
"test" in the description field. Apologies for any mistakes.

Comment 27

[Kevin McCluskey (gone)]

patch looks good to me.
r=[e-mail]

Comment 28

[rods (gone)]

fixed and checked in everything but labels, I am not closing this out, but I 
will mark as future

Comment 29

[Greg K.]

Reconfirmed using FizzillaCFM/2002071208. Labels still don't honor min-width.
Setting All/All.

Comment 30

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

Why should it work at all for labels?  They're inline elements, so the 'width'
property doesn't apply.

Comment 31

[Tom Gilder]

Attachment: Easier-to-follow testcase

All tests with min-width and max-width on <label> and input elements work for
me on Win98/2002090408. Attached a quick demo file which is easier to test than
the other testcases.

Comment 32

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

Restoring original summary and marking fixed.  Labels were fixed by bug 96813. 
(I think that makes more sense than marking as duplicate.)  See comment 28.