Last Comment Bug 291060 - table cells containing empty blocks have the wrong baseline
: table cells containing empty blocks have the wrong baseline
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: Tables (show other bugs)
: Trunk
: All All
: -- normal with 3 votes (vote)
: ---
Assigned To: Bernd
:
Mentors:
Depends on: 444561 464872
Blocks: acid2 146146
  Show dependency treegraph
 
Reported: 2005-04-19 18:32 PDT by David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
Modified: 2014-04-25 15:15 PDT (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
initial patch (11.92 KB, patch)
2005-05-04 02:37 PDT, Bernd
no flags Details | Diff | Review
changes catched by rtest illustration (7.52 KB, image/png)
2005-05-05 11:32 PDT, Bernd
no flags Details
revised patch (12.68 KB, patch)
2005-07-17 01:34 PDT, Bernd
dbaron: review+
dbaron: superreview+
Details | Diff | Review
screenshot (74.18 KB, image/png)
2005-10-04 06:02 PDT, Zibi Braniecki [:gandalf][:zibi]
no flags Details

Description David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-04-19 18:32:37 PDT
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.
Comment 1 Hixie (not reading bugmail) 2005-04-25 07:18:03 PDT
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."
Comment 2 Bernd 2005-05-02 09:31:16 PDT
this is rather a table code problem
Comment 3 Bernd 2005-05-02 09:32:26 PDT
somewhere the buck needs to stop
Comment 4 Bernd 2005-05-03 10:17:22 PDT
>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?
Comment 5 Hixie (not reading bugmail) 2005-05-03 10:57:18 PDT
The spec doesn't really cover row-spanning cells, but I think the answer is no.
Comment 6 Bernd 2005-05-04 02:37:26 PDT
Created attachment 182575 [details] [diff] [review]
initial patch
Comment 7 Bernd 2005-05-05 11:32:02 PDT
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.
Comment 8 Bernd 2005-05-05 11:35:15 PDT
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.
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-06-27 17:04:40 PDT
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.
Comment 10 Bernd 2005-07-17 01:34:40 PDT
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.
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-07-21 16:40:45 PDT
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.
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-09-28 17:08:10 PDT
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.
Comment 13 amano 2005-10-02 16:10:36 PDT
Hmm. Did that get checked in onto the trunk?
Comment 14 Zibi Braniecki [:gandalf][:zibi] 2005-10-04 06:02:17 PDT
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)
Comment 15 Bernd 2005-10-04 09:45:43 PDT
I checked the patch in. I opened the followup bug 311076 to address davids
comment 9 

Note You need to log in before you can comment on or make changes to this bug.