Closed Bug 1186998 Opened 9 years ago Closed 9 years ago

Block direction overflowing content in table-cell uses cell's border-box for block alignment.

Categories

(Core :: Layout: Tables, defect)

Product:
Core
Component:
Layout: Tables
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla45
Tracking Flags:
Tracking Status
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: zentner.kyle, Assigned: zentner.kyle, Mentored)

Details

Attachments

(3 files, 2 obsolete files)

table-cell-block-overflow.html
974 bytes, text/html
Details
table-cell-block-overflow-ref.html
1001 bytes, text/html
Details
TableCellContentClipping [r=dholbert]
4.05 KB, patch
zentner.kyle
: review+
Details | Diff | Splinter Review 
When the content of a table-cell is larger than the cell's content box in the block direction, and the cell is either 'vertical-align: middle' or 'vertical-align: bottom', the content must be clipped to avoid intruding above the cell's border-box.

Currently, the content is clipped against the cell's *border-box.* This results in the border-box of the content overlapping the top of the border-box of the cell. It should be clipped against the cell's *content-box* instead, to prevent this from occurring.

The content of a table-cell can be larger than the cell in the block direction if the content has (non-zero margin + border + padding, and a height set to a percentage), or if the table-cell has layout containment.
Component: CSS Parsing and Computation → Layout: Tables
Version: unspecified → Trunk
Can you attach a testcase that demonstrates the bug please?
Flags: needinfo?(kzentner)
This is a test case which reproduces the bug.
Flags: needinfo?(kzentner)
Here is a matching reference for what I think the previous attachment should look like.
Attached patch TableCellContentClipping (obsolete) — — Splinter Review 
Here's a patch which has the previous html files added as a reftest.

It also contains the (one line) fix.
Attachment #8638133 - Flags: review?(dholbert)
Attached patch TableCellContentClipping (obsolete) — — Splinter Review 
As per our discussion, here's a patch where the reftest uses my personal email instead.
Attachment #8638133 - Attachment is obsolete: true
Attachment #8638133 - Flags: review?(dholbert)
Attachment #8638136 - Flags: review?(dholbert)
(In reply to Kyle Zentner from comment #5)
> Created attachment 8638136 [details] [diff] [review]
> TableCellContentClipping
> 
> As per our discussion, here's a patch where the reftest uses my personal
> email instead.

Ah, thanks -- so, you only really need an author email-address for tests that we're submitting to w3c (in the w3c-css subdirectory).  For tests that live in our testsuite, "hg log" / "hg blame" gives us enough author information.

So for this bug, you could also just drop that field.
Comment on attachment 8638136 [details] [diff] [review]
TableCellContentClipping

Review of attachment 8638136 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with notes below addressed:

> Bug 1186998: Clip overflowing table-cell content to content-box.

This commit message doesn't seem quite right. (We're not doing any clipping here -- just alignment.)

Maybe: "Align overflowing table-cell content to the block-start edge of cell's content-box (instead of border-box)

::: layout/reftests/table-overflow/table-cell-block-overflow-ref.html
@@ +1,5 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <meta charset="utf-8">
> +  <title>CSS Test: table-cell content overflowing in the block direction</title>

The title here seems stale. Replace with "CSS Reference"?

@@ +12,5 @@
> +    height:  54px;
> +    background: purple;
> +  }
> +  .fake-td {
> +   display: inline-block;

Might be simpler to just make the referece case be a copy of the testcase, but with "vertical-align: top" (which already renders correctly) on all of the cells. (Then you don't have to hack up all the pixel values to be exactly right, between block vs. table.)

Alternately, if you want to avoid tables in the reference case (which seems reasonable): it's a bit cleaner to use "float: left" on .fake-td, instead of display:inline-block.  That's an easier way to get things to line up horizontally like table-cells. (and it means you can get rid of your 'font-size:0' hack.)

::: layout/reftests/table-overflow/table-cell-block-overflow.html
@@ +4,5 @@
> +  <meta charset="utf-8">
> +  <title>Test table-cell content overflowing in the block direction for each
> +    vertical-align</title>
> +  <link rel="author" title="Kyle Zentner" href="mailto:zentner.kyle@gmail.com">
> +  <link rel="match" href="table-cell-block-overflow-ref.html">

As noted above, you can drop both of these <link> tags -- they only matter for tests in the official w3c testsuite.

::: layout/tables/nsTableCellFrame.cpp
@@ +627,5 @@
>        // Align the middle of the child frame with the middle of the content area,
>        kidBStart = (bSize - childBSize - bEndInset + bStartInset) / 2;
>    }
> +  // if the content is larger than the cell bsize, align from bStartInset
> +  kidBStart = std::max(bStartInset, kidBStart);

Add a parenthetical like "(cell's content-box bstart edge)" at the end, to clarify what "align from bStartInset" actually means.

(This probably needs to go on a 2nd line to avoid being too long)
Attachment #8638136 - Flags: review?(dholbert) → review+
I think this addresses everything.
Attachment #8638136 - Attachment is obsolete: true
Attachment #8638168 - Flags: review+
The last two test runs for this patch (above and https://treeherder.mozilla.org/#/jobs?repo=try&revision=df9b6b7aaed6) look good, so I'm marking this checkin-needed.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1e5910f48b8f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: