365932 - [FIX]Factor out code that handles nsStyleCoords in computed style

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

There's a lot of very similar code in computed style that switches on the unit of an nsStyleCoord and sets a nsROCSSPrimitiveValue to various things depending on that unit.  Factoring out that logic into a helper method allows removal of a bunch of lines of code and reduces codesize some.

Substantive changes in this patch:

1) Do the refactoring mentioned above.
2) Change column-gap percentages to use the frame's content
   rect width as the percentage base, like layout does, instead
   of using the border rect width.
3) Change text-indent percentages to use the containing block's
   content rect width as the percentage base, like layout does,
   instead of using the border rect width.
4) Change max-height percentages to use the containing block's
   content rect height, like layout does, instead of using the
   border rect height.  Also change the min-height calculation
   in GetMaxHeight use the content rect height of the CB as the
   percentage base.
5) Change max-width percentages to use the containing block's
   content rect width, like layout does, instead of using the
   border rect width.  Also change the min-width calculation
   in GetMaxWidth use the content rect width of the CB as the
   percentage base.
6) Change min-height percentages to use the containing block's
   content rect height as the percentage base, like layout does,
   instead of using the border rect height.
7) Change min-width percentages to use the containing block's
   content rect width as the percentage base, like layout does,
   instead of using the border rect width.

All that said, I'm not convinced that we should keep our existing "sorta used value" behavior for min-width/max-width/min-height/max-height.  If we do, we need to change it for heights for sure; for example, a percent max height is only non-auto as a used-value if the parent has a non-auto height...  As a result, I haven't really written tests for those changes (items 4, 5, 6, 7 above).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

See comment 0, though, for my concerns about whether this is really the behavior we want for min/max-width/height.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to tip

Comment 3

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

Comment on attachment 255008 [details] [diff] [review]
Merged to tip

> nsComputedDOMStyle::GetColumnGap(nsIDOMCSSValue** aValue)

>+  // XXXbz Should eStyleUnit_Normal be a length or the keyword?

>-    case eStyleUnit_Normal:
>-      val->SetAppUnits(GetStyleFont()->mFont.size);
>-      break;

>+  SetValueToCoord(val, GetStyleColumn()->mColumnGap,
>+                  &nsComputedDOMStyle::GetFrameContentWidth);

Seems like you're actually regressing this one, and you should put the normal check back.

> nsresult
> nsComputedDOMStyle::GetLineHeight(nsIDOMCSSValue** aValue)

>+  if (GetLineHeightCoord(lineHeight)) {
>     val->SetAppUnits(lineHeight);
>   } else {

How could GetLineHeightCoord ever fail such that the else does something useful?

> nsresult
> nsComputedDOMStyle::GetMaxHeight(nsIDOMCSSValue** aValue)
>+  nscoord minHeight =
>+    StyleCoordToNSCoord(positionData->mMinHeight,
>+                        &nsComputedDOMStyle::GetCBContentHeight);
> 
>-    break;
>-  }
>+  SetValueToCoord(val, positionData->mMaxHeight,
>+                  &nsComputedDOMStyle::GetCBContentHeight,
>+                  nsnull, minHeight);

I think including min-height in computed max-height is bogus.  I think they should both be included in computed height, though (as you suggested in XXX comments for the null aFrame case).  And same for widths.

>+PRBool
>+nsComputedDOMStyle::GetLineHeightCoord(nscoord& aCoord)

Seems like this needs to handle 'normal' line-heights somehow.  Isn't this rather bad for percentage vertical-align to computed value in the normal case?

>+void
>+nsComputedDOMStyle::SetValueToCoord(nsROCSSPrimitiveValue* aValue,
>+                                    nsStyleCoord aCoord,
>+                                    PercentageBaseGetter aPercentageBaseGetter,
>+                                    const PRInt32 aTable[],
>+                                    nscoord aMinVal)

Maybe call aMinVal aMinAppUnits, since it only affects SetAppUnits values?

>+    case eStyleUnit_Chars:
>+      // XXX we need a frame and a rendering context to calculate this, bug 281972, bug 282126.

The first bug number doesn't look related, although you did just move that comment.

Could you file a bug on actually fixing this case?  (It seems like what you really need is an nsStyleFont and a rendering context.)

>+    case eStyleUnit_Null:
>+      aValue->SetIdent(nsGkAtoms::none);
>+      break;

I wonder how portable this is.  Were the only callers ones that weren't supposed to need it anyway?

>+nscoord
>+nsComputedDOMStyle::StyleCoordToNSCoord(nsStyleCoord aCoord,
>+                                        PercentageBaseGetter aPercentageBaseGetter)
>+{
>+  NS_PRECONDITION(aPercentageBaseGetter, "Must have a percentage base getter");

Perhaps a little strong; it could be used for a value that doesn't take percents.  But probably good to keep it there while you can.


I'll trust you on the tests, but please run them before checking in.

r+sr=dbaron

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Seems like you're actually regressing this one

I checked with roc and he's ok either way.  Do you have a preference?  It's basically a computed-vs-used issue...

> How could GetLineHeightCoord ever fail such that the else does something
> useful?

"line-height: normal"

> I think including min-height in computed max-height is bogus.

OK.  Do you want me to do that in this patch, or a followup?

> Seems like this needs to handle 'normal' line-heights somehow.

Right.  Basically, I didn't change the behavior here (or for vertical-align percentages).  Again, it's a "computed vs used" kinda thing.  I suppose we could make 'normal' line-heights compute to something sane.  Again, do you want that in this bug, or a followup?

> Maybe call aMinVal aMinAppUnits, since it only affects SetAppUnits values?

Will do.

> Could you file a bug on actually fixing this case?

Sure.

> I wonder how portable this is

The only eStyleUnit_Null we used to handle is for marker-offset.  I checked, and that's the only eStyleUnit_Null we end up with in the nsStyleStruct structs at the moment.

But no, I don't think it's that portable at all.  I'd almost rather we computed marker-offset to something else in that case and asserted here that the unit is not null.

Let me know on the above issues, ok?

Comment 5

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

(In reply to comment #4)
> > Seems like you're actually regressing this one
> 
> I checked with roc and he's ok either way.  Do you have a preference?  It's
> basically a computed-vs-used issue...

I think I prefer it the way it was -- getComputedStyle has always been about what's now the used value.

> OK.  Do you want me to do that in this patch, or a followup?

> Again, do you want
> that in this bug, or a followup?

Followup bugs are fine.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to review comments

Change the Normal thing, renamed the argument, moved the tests to final locations.

I'll file followup bugs on GetLineHeightCoord, max-height stuff, eStyleUnit_Chars.

I did run the tests; they basically pass except for a transient failure in reftest that I hit all the time -- sometimes identical images (according to GIMP) end up with different data: URIs...

Comment 7

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Filed bug 371041 on GetLineHeightCoord.
Filed bug 371042 on max/min stuff.
Filed bug 371043 on eStyleUnit_Chars.

Comment 9

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

And, for what it's worth, we might want to try to write some mochitests for the output of getComputedStyle.  (And we should be sure to test visible frames, frames with 'display:none', and descendants of frames with 'display:none'.)

Comment 10

[Boris Zbarsky [:bzbarsky]]

Yeah, I landed mochitests for some properties in this bug, but we could use a lot more...