363573 - Table cells don't expand to contain <pre cols> ('ch' unit is broken)

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[[e-mail]]

Steps to reproduce problem:
1. Load the attached test case

Expected results: horizontal rule extends to the right of the text

Actual result: table cell has incorrect width

Comment 1

[Mats Palmgren (inactive)]

Patch coming up...

Comment 2

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Comment 3

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

The 'cols' attribute is mapped to a 'ch' unit width.

This patch implements the 'ch' unit generally for 'width', 'min-width',
'max-width', 'padding' and 'margin'.
I'm leaving 'border'/'outline' for now since it needs style struct changes.
I will file a separate bug that.

I also noted that "CSS3 Values and Units" now has a 'ch' unit
that is the width of a "0" (zero) whereas we use "M".
http://www.w3.org/TR/css3-values/#relative0
We should probably fix that separately though since it may involve
adjusting INPUT and/or TEXTAREA code.

Comment 4

[[e-mail]]

Comment on attachment 248510 [details] [diff] [review]
Patch rev. 1

>+static PRBool GetAbsoluteCoord(const nsStyleCoord& aStyle,
>+                               nsIRenderingContext* aRenderingContext,
>+                               nsIFrame* aFrame,
>+                               nscoord& aResult)
>+{
>+  nsStyleUnit unit = aStyle.GetUnit();
>+  if (eStyleUnit_Coord == unit) {
>+    aResult = aStyle.GetCoordValue();
>+    return PR_TRUE;
>+  }
>+  if (eStyleUnit_Chars == unit) {
>+    SetFontFromStyle(aRenderingContext, aFrame->GetStyleContext());
>+    nscoord fontWidth;
>+    aRenderingContext->GetWidth('M', fontWidth);
>+    aResult = aStyle.GetIntValue() * fontWidth;
>+    return PR_TRUE;
>+  }
>+  return PR_FALSE;
>+}
This looks suspiciously similar to ComputeHorizontalValue() which is unfortunately declared *after* IntrinsicForContainer :-/

Comment 5

[Mats Palmgren (inactive)]

Attachment: Testcase #3 (vertical properties)

Comment 6

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Ok, I made both ComputeHorizontalValue and ComputeVerticalValue use
GetAbsoluteCoord, the latter made 'ch' work for 'height', 'min-height'
and 'max-height' (to my surprise it seems to be working for vertical
padding and margin already).

Comment 7

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

(In reply to comment #6)
> Ok, I made both ComputeHorizontalValue and ComputeVerticalValue use
> GetAbsoluteCoord, the latter made 'ch' work for 'height', 'min-height'
> and 'max-height' (to my surprise it seems to be working for vertical
> padding and margin already).

Vertical padding and margin use ComputeHorizontalValue; ComputeHorizontalValue really means "Compute value where percentages are based on width" and ComputeVerticalValue really means "Compute value where percentages are based on height".  If someone can think of short names that represent those concepts, they're probably worth renaming.

Comment 8

[Boris Zbarsky [:bzbarsky]]

ComputeWidthDependentValue?  And same for height?

Comment 9

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

Comment on attachment 248557 [details] [diff] [review]
Patch rev. 2

r+sr=dbaron.

Let's do the renaming in a separate patch after this one lands.  (You can if you want, or I can...)

Comment 10

[Mats Palmgren (inactive)]

Checked in to trunk at 2006-12-17 06:06 PST.

Filed bug 364131 on renaming the methods.

The first round of T* stats from Tinderbox looks stable, I'll continue to
monitor it for a few more rounds.

-> FIXED

Comment 11

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20061218 Minefield/3.0a1 ID:2006121804 [cairo]
I still see some red areas in the two testcases. Is this expected?

Comment 12

[[e-mail]]

(In reply to comment #11)
>I still see some red areas in the two testcases. Is this expected?
The border widths look miscomputed, but that might be a separate bug.

Comment 13

[Mats Palmgren (inactive)]

(In reply to comment #11)
> I still see some red areas in the two testcases. Is this expected?

Yes, it's expected. It's the border part of bug 363706.
We should probably remove the border test (the last <div class="error">)
from these testcases to make them pass and use the testcase in bug 363706
for border/outline instead...