The default bug view has changed. See this FAQ.

table cells containing empty blocks have the wrong baseline

RESOLVED FIXED

Status

()

Core
Layout: Tables
RESOLVED FIXED
12 years ago
3 years ago

People

(Reporter: dbaron, Assigned: Bernd)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
Table cells containing empty blocks have the wrong baseline.  There are also a
bunch of other related bugs -- we should write good tests for the text that will
be in the next draft of CSS 2.1.

Testcases forthcoming, eventually, but getting a bug on file so that the acid2
test bug (bug 289480) has accurate dependencies.
For those of you without W3C member access, the resolution that the CSS working
group made on this issues was as follows:

In CSS2.1 17.5.3, replace:

"The baseline of a cell is the baseline of the first line box in the
cell. If there is no text, the baseline is the baseline of whatever
object is displayed in the cell, or, if it has none, the bottom of the
cell box."

...with:

"The baseline of a cell is the baseline of the first in-flow line box
in the cell, or the first in-flow table-row in the cell, whichever
comes first. If there is no such line box or table-row, the baseline
is the bottom of content edge of the cell box. For the purposes of
finding a baseline, in-flow boxes with a scrolling mechanisms (see the
'overflow' property) must be considered as if scrolled to their origin
position. Note that the baseline of a cell may end up below its bottom
border, see the example below."

...and take the rest of the paragraph ("The maximum distance between
the top of the cell box and the baseline over all cells that have
'vertical-align: baseline' is used to set the baseline of the row.
Here is an example:") and put it into a new paragraph.

Then add this example at the bottom of the section:

   The cell in this example has a baseline below its bottom border,
   which leaves an empty space below the cell in its table row:

      div { height: 0; overflow: hidden; }

      <table>
       <tr>
        <td>
         <div> Test </div>
        </td>
       </tr>
      </table>

...then change "Note that if there is no cell box aligned at its
baseline, the row will not have (nor need) a baseline." to be its own
paragraph and say:

"If a row has no cell box aligned to its baseline, the baseline of
that row is the bottom content edge of the lowest cell in the row."
(Assignee)

Comment 2

12 years ago
this is rather a table code problem
Component: Layout: Block and Inline → Layout: Tables
QA Contact: layout.block-and-inline → layout.tables
(Assignee)

Comment 3

12 years ago
somewhere the buck needs to stop
Assignee: nobody → bernd_mozilla
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 4

12 years ago
>If a row has no cell box aligned to its baseline, the baseline of
that row is the bottom content edge of the lowest cell in the row.

Ian, does that also apply to rowspanning cells?
The spec doesn't really cover row-spanning cells, but I think the answer is no.
(Assignee)

Comment 6

12 years ago
Created attachment 182575 [details] [diff] [review]
initial patch
(Assignee)

Comment 7

12 years ago
Created attachment 182694 [details]
changes catched by rtest illustration 

I did the large rtest and did only find 2 real visual discrepancies:
http://www.hixie.ch/tests/adhoc/css/box/table/002.html failed
http://dbaron.org/css/test/sec170201 failed
both seem like improvements to me.
(Assignee)

Comment 8

12 years ago
Comment on attachment 182575 [details] [diff] [review]
initial patch

David, while I am pretty sure what to do with tables there is one change to
nsBlockFrame where I am not confident.
Attachment #182575 - Flags: superreview?(dbaron)
Attachment #182575 - Flags: review?(dbaron)
(Reporter)

Comment 9

12 years ago
Comment on attachment 182575 [details] [diff] [review]
initial patch

We'll probably have to revisit the nsBlockFrame changes when we implement
'inline-block'.  Could you add a comment above both of them saying so?	

Also, the "future version of CSS 2.1" is no longer future.

According to the spec text, though, an empty table has no baseline at all,
rather than its bottom, so this still isn't *quite* right.  (Ian wrote it, and
he has a very different mental model of how baselines should work than the one
our code has or the one I have.)

nsTableRowFrame::GetAscent should probably explain that it's getting the lowest
content edge, and how given the placement of the border and padding on a table
cell on the cell frame rather than the inner block frame, using the rect of the
inner block frame is correct and you don't need to subtract out the padding.
(Assignee)

Comment 10

12 years ago
Created attachment 189587 [details] [diff] [review]
revised patch

This patch has only updated comments with respect to the initial one. I have to
admit that I had difficulties to parse the sentences in the review comment
before. I hope this is what the comment asked for.
Attachment #182575 - Attachment is obsolete: true
Attachment #189587 - Flags: superreview?(dbaron)
Attachment #189587 - Flags: review?(dbaron)
(Assignee)

Updated

12 years ago
Attachment #182575 - Flags: superreview?(dbaron)
Attachment #182575 - Flags: review?(dbaron)
(Reporter)

Comment 11

12 years ago
So what I was trying to say in this part of comment 9:

> According to the spec text, though, an empty table has no baseline at all,
> rather than its bottom, so this still isn't *quite* right.  (Ian wrote it, and
> he has a very different mental model of how baselines should work than the one
> our code has or the one I have.)

was that this patch is incorrect in the case where a table cell begins with an
empty table.  This patch treats some part of the empty table as the baseline for
the outer cell, but the correct behavior according to Ian's text is that the
baseline should come from whatever follows the empty table.
(Reporter)

Comment 12

12 years ago
Comment on attachment 189587 [details] [diff] [review]
revised patch

This should be closer than what we currently have, so we may as well get it
checked in, at least for 1.9.
Attachment #189587 - Flags: superreview?(dbaron)
Attachment #189587 - Flags: superreview+
Attachment #189587 - Flags: review?(dbaron)
Attachment #189587 - Flags: review+

Comment 13

12 years ago
Hmm. Did that get checked in onto the trunk?
Created attachment 198441 [details]
screenshot

this screenshot shows the difference in Acid2 test with and without this patch
(Firefox on the left is with this patch, Seamonkey on the right is today's
nightly without it)
(Assignee)

Comment 15

12 years ago
I checked the patch in. I opened the followup bug 311076 to address davids
comment 9 
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Blocks: 146146
Depends on: 444561
Depends on: 464872
You need to log in before you can comment on or make changes to this bug.