Just as the summary says
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
Rod: I must be missing something. What exactly isn't working in the attached test case?
Status: NEW → ASSIGNED
As we talked on the phone the text input at the bottom
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
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.
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...
maybe it isn't a regression, I thought this used to work. What aren't the form controls doing that they should?
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?
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
I now have input button and HTML 4 button working.
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; >
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.
The patch above fixes button, html 4 button, selects (combo & listbox) eric has added the patch for text field/area
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);
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
r=evaughan for Rods changes.
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
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.
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.
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. firstname.lastname@example.org
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
Target Milestone: --- → Future
Summary: min-width and max-width not supported on form controls → min-width and max-width not supported on <label>
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.
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.