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

RESOLVED FIXED in Future

Status

()

Core
Layout
RESOLVED FIXED
18 years ago
16 years ago

People

(Reporter: rods (gone), Assigned: rods (gone))

Tracking

({css2, testcase})

Trunk
Future
css2, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

18 years ago
Just as the summary says
(Assignee)

Comment 1

18 years ago
I think this really needsa to be fixed, both because it is a regression and 
because it is a pretty basic CSS1 feature.
Keywords: css1, nsbeta3, regression
(Assignee)

Comment 2

18 years ago
Created attachment 15044 [details]
testcase

Comment 3

18 years ago
Rod: I must be missing something.  What exactly isn't working in the attached 
test case?
Status: NEW → ASSIGNED
(Assignee)

Comment 4

18 years ago
As we talked on the phone the text input at the bottom

Comment 5

18 years ago
raising to P2 since this is a CSS1 conformance issue
Priority: P3 → P2
Min and max width are CSS2.  Is it only in some cases that they still no longer
work?
Keywords: css1 → css2

Comment 7

18 years ago
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

18 years ago
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.
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...
(Assignee)

Comment 10

18 years ago
Created attachment 15836 [details]
testcase show no form controls obey min-width/maxwidth
(Assignee)

Comment 11

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

Comment 12

18 years ago
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

18 years ago
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
(Assignee)

Comment 14

18 years ago
I now have input button and HTML 4 button working.

Comment 15

18 years ago
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

18 years ago
r=buster.  looks fine, eric
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.

(Assignee)

Comment 18

18 years ago
Created attachment 16043 [details] [diff] [review]
patch file for form controls
(Assignee)

Comment 19

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

Comment 20

18 years ago
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

18 years ago
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.
Keywords: regression → rtm
Summary: min-width and max-width no longer works → min-width and max-width not supported on form controls

Comment 22

18 years ago
r=evaughan for Rods changes.

Comment 23

18 years ago
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
Assignee: buster → rods
Status: ASSIGNED → NEW

Comment 24

18 years ago
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.
Whiteboard: [rtm+]

Comment 25

18 years ago
Unless this causes some top pages to behave really badly, this is [rtm-]
Whiteboard: [rtm+] → [rtm-]
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.
Keywords: testcase
(Assignee)

Updated

18 years ago
Status: NEW → ASSIGNED
Priority: P2 → P1
(Assignee)

Updated

18 years ago
Summary: min-width and max-width not supported on form controls → [MF][FIX]min-width and max-width not supported on form controls
patch looks good to me.
r=kmcclusk@netscape.com
(Assignee)

Comment 28

18 years ago
fixed and checked in everything but labels, I am not closing this out, but I 
will mark as future
Priority: P1 → P3
Summary: [MF][FIX]min-width and max-width not supported on form controls → min-width and max-width not supported on form controls
Whiteboard: [rtm-]
Target Milestone: --- → Future
Attachment #16043 - Attachment is obsolete: true
Summary: min-width and max-width not supported on form controls → min-width and max-width not supported on <label>
(Assignee)

Updated

17 years ago
Priority: P3 → --

Comment 29

16 years ago
Reconfirmed using FizzillaCFM/2002071208. Labels still don't honor min-width.
Setting All/All.
OS: Windows NT → All
Hardware: PC → All
Why should it work at all for labels?  They're inline elements, so the 'width'
property doesn't apply.

Comment 31

16 years ago
Created attachment 97889 [details]
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.
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Summary: min-width and max-width not supported on <label> → min-width and max-width not supported on form controls
You need to log in before you can comment on or make changes to this bug.