Closed Bug 363573 Opened 14 years ago Closed 14 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: neil, Assigned: mats)

References

()

Details

(Keywords: regression, testcase)

Attachments

(3 files, 1 obsolete file)

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
Summary: [Reflow refactor] Table cells don't expand to contain <pre cols> → Table cells don't expand to contain <pre cols>
Patch coming up...
Assignee: nobody → mats.palmgren
Keywords: testcase
OS: Windows XP → All
Hardware: PC → All
Attached file Testcase #2
Blocks: 282126
Summary: Table cells don't expand to contain <pre cols> → Table cells don't expand to contain <pre cols> ('ch' unit is broken)
Attached patch Patch rev. 1 (obsolete) — Splinter Review
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.
Attachment #248510 - Flags: superreview?(dbaron)
Attachment #248510 - Flags: review?(dbaron)
Blocks: 363706
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 :-/
Flags: in-testsuite?
Flags: blocking1.9?
Attached patch Patch rev. 2Splinter Review
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).
Attachment #248510 - Attachment is obsolete: true
Attachment #248557 - Flags: superreview?(dbaron)
Attachment #248557 - Flags: review?(dbaron)
Attachment #248510 - Flags: superreview?(dbaron)
Attachment #248510 - Flags: review?(dbaron)
(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.
ComputeWidthDependentValue?  And same for height?
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...)
Attachment #248557 - Flags: superreview?(dbaron)
Attachment #248557 - Flags: superreview+
Attachment #248557 - Flags: review?(dbaron)
Attachment #248557 - Flags: review+
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
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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?
(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.
(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...
Flags: blocking1.9?
You need to log in before you can comment on or make changes to this bug.