125778 - Implement getComputedStyle() for {min|max}-{height|width} properties

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect, P2)

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

I'll tackle these as soon as I can.

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch v1.0

Initial stab at this.

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

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...

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Comment 4

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: simple testcase for min value greater than max value.

With attachment 69845 [details] [diff] [review] I get:

max-height: 200px
max-width: 200px
min-height: 400px
min-width: 400px

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

...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?

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

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....

Comment 7

[Boris Zbarsky [:bzbarsky]]

> 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.  :)

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

> 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?

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch v1.1

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.

Comment 11

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

> +#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.

Comment 13

[Christopher Aillon (sabbatical, not receiving bugmail)]

>> +#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.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> I get a compile warning without the cast.

In that case do | nscoord(foo) | instead of | (nscoord)foo |, please.

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch v1.2

Fixes outstanding issues

Comment 16

[Boris Zbarsky [:bzbarsky]]

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

r=bzbarsky

Comment 17

[Marc Attinasi]

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

sr=attinasi

Comment 18

[Christopher Aillon (sabbatical, not receiving bugmail)]

Fix checked in by timeless.