I'll tackle these as soon as I can.

Initial stab at this.

http://www.w3.org/TR/REC-CSS2/visudet.html#propdef-min-height 2. If the computed value of 'min-height' is greater than the value of 'max-height', 'max-height' is set to the value of 'min-height'. Does that affect the actual value, computed value, or both? I might need to adjust for this...

At a guess, that would affect the computed value only, not the actual value... write a testcase?

With attachment 69845 [details] [diff] [review] I get: max-height: 200px max-width: 200px min-height: 400px min-width: 400px

...so I don't really want to compute the Min value while I'm computing the Max value. That would seem weird. Would it be okay if I compared the max value to the frame's height or width? |aFrame->GetRect(myRect);| and then comparing the computed max value to myRect.height (or width)? It seems to me that this would work because the only time the current box's width/height will be larger than it's max- value is if the min- value is larger than than the max- value. In which case all 3 computed values should be the same... thoughts?

Hixie says that in some instances with fixed positioned tables, the min- and max- values can be completely ignored so I can't compare to the width. Where does layout compute this? It might make more sense to deal with this there (store the computed value like we do for font-size). Otherwise I'd have lots of duplicate code. I *could* write a wrapper for this, for instance |nscoord nsComputedDOMStyle::GetMinLength(PRUint8 aMinType, nsIFrame* aFrame, nsIDOMCSSPrimitiveValue*& aValue)| to be called by each of the 4 functions and define 2 constants to determine which min value we're looking for (NS_STYLE_MIN_HEIGHT and NS_STYLE_MIN_WIDTH ??)... but I'm not sure if that's the best solution, though....

> Where does layout compute this? http://lxr.mozilla.org/seamonkey/search?string=maxwidth See the parts about reflow state and tables and the like I'd strongly suggest _not_ touching table layout in the process of working on this bug. Just get both the min and max width from the style struct and compare them... then return the bigger one for max-height. min-height is just what's in the style struct. Check what the spec says for widths. :)

> Just get both the min and max width from the style struct and compare them... Right, but then I need to accomodate for if one is a percentage and the other is a coord. Specifically if the min value is a percentage. that's where the duplicated code comes into play because I'm already doing that for the GetMin function. Unless I'm missing something?

hmm... no, I don't think you are....

Accomodates for min > max. Tested and works. I #define'd MAX at the start of the file as I needed that, and didn't know where else to put it. If there's a better suggestion for it's placement, let me know. I couldn't think of any good way to re-use code unfortunately. But I've been sick and my mind may be cluttered. Any ideas there would be cool too. Otherwise, I think this patch is good.

For more testcases, use inspector's computed style view on one of these pages: Max/Min Widths http://www.people.fas.harvard.edu/~dbaron/css/test/sec1004 http://www.people.fas.harvard.edu/~dbaron/css/test/sec1004b Max/Min Heights http://www.people.fas.harvard.edu/~dbaron/css/test/sec1007 http://www.people.fas.harvard.edu/~dbaron/css/test/sec1007b

> +#define MAX(a,b) ((a > b) ? (a) : (b)) Use PR_MAX from prtypes.h >+ minHeight = (nscoord)(rect.width * > positionData->mMinHeight.GetPercentValue()); Do you really need the cast-like thing? And this should be using rect.height > + if (positionData->mMinHeight.GetUnit() == eStyleUnit_Percent) { > + container = GetContainingBlock(aFrame); ... > + case eStyleUnit_Percent: > + container = GetContainingBlock(aFrame); The second time, you should only do that if container is null, no? If it's not null, you can avoid calling GetContainingBlock and GetRect. Similar comments on the MaxWidth stuff (except there you correctly use rect.width). You probably want to call FlushPendingNotifications() for all of these, since you're looking at sizes of parent frames and such.

>> +#define MAX(a,b) ((a > b) ? (a) : (b)) > Use PR_MAX from prtypes.h Ah, okay. I knew there had to be something I could use. Thanks! :) >>+ minHeight = (nscoord)(rect.width * >> positionData->mMinHeight.GetPercentValue()); > Do you really need the cast-like thing? And this should be using rect.height I get a compile warning without the cast. As far as rect.height, yes I had that fixed locally after my tests but I guess I forgot to re-diff before uploading. Oops. Your other comments make sense. I'll get those fixed in my next patch.

> I get a compile warning without the cast. In that case do | nscoord(foo) | instead of | (nscoord)foo |, please.

Fixes outstanding issues

Comment on attachment 70117 [details] [diff] [review] Patch v1.2 r=bzbarsky

Comment on attachment 70117 [details] [diff] [review] Patch v1.2 sr=attinasi

Fix checked in by timeless.