Last Comment Bug 248239 - borders and padding on the top and bottom of table cells reduce the height
: borders and padding on the top and bottom of table cells reduce the height
Status: RESOLVED FIXED
: dev-doc-needed, testcase
Product: Core
Classification: Components
Component: Layout: Tables (show other bugs)
: Trunk
: All All
: -- normal with 8 votes (vote)
: mozilla16
Assigned To: Tal Aloni
:
Mentors:
: 295315 351489 364585 (view as bug list)
Depends on: 770014 770045
Blocks: 338554
  Show dependency treegraph
 
Reported: 2004-06-22 19:57 PDT by XFox
Modified: 2014-11-19 17:00 PST (History)
20 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Shows an example of how border-top / bottom don't match border-left / right's behavior (476 bytes, application/xhtml+xml)
2004-06-22 20:01 PDT, XFox
no flags Details
TESTCASE (466 bytes, text/html)
2004-07-11 19:30 PDT, XFox
no flags Details
testcase (580 bytes, text/html; charset=UTF-8)
2004-07-11 23:50 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details
testcase (909 bytes, text/html)
2004-09-27 17:41 PDT, Mike Mimic
no flags Details
padding testcase (599 bytes, text/html)
2006-09-05 18:27 PDT, Brian Polidoro
no flags Details
Padding testcase #2 (602 bytes, text/html)
2007-12-30 13:40 PST, T. Chan
no flags Details
Research on table, row, cell and height (7.48 KB, text/html)
2009-04-25 13:54 PDT, Daniel.S
no flags Details
Table Height Bugfix Patch (1.04 KB, patch)
2012-06-08 07:54 PDT, Tal Aloni
no flags Details | Diff | Review
Table Height Height Bugfix Patch v2 (1.27 KB, patch)
2012-06-08 09:39 PDT, Tal Aloni
no flags Details | Diff | Review
Table Cell Height Bugfix Patch v3 (2.02 KB, patch)
2012-06-08 23:26 PDT, Tal Aloni
no flags Details | Diff | Review
Caption Test Case 1 (876 bytes, text/html)
2012-06-09 01:40 PDT, Tal Aloni
no flags Details
Table Test Case 1 (814 bytes, text/plain)
2012-06-09 01:41 PDT, Tal Aloni
no flags Details
Table Cell Height Patch v4 (1.85 KB, patch)
2012-06-09 02:05 PDT, Tal Aloni
no flags Details | Diff | Review
Table Cell Height Patch v4 (With updated reftests) (3.78 KB, patch)
2012-06-15 06:35 PDT, Tal Aloni
dbaron: review-
Details | Diff | Review
Caption Test Case 1, in quirks mode (774 bytes, text/html)
2012-06-25 13:11 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details
Table Cell Height Patch v5 (3.53 KB, patch)
2012-06-25 15:43 PDT, Tal Aloni
no flags Details | Diff | Review
Table Cell Height Patch v6 (3.54 KB, patch)
2012-06-26 13:31 PDT, Tal Aloni
dbaron: review+
Details | Diff | Review
test for caption/cell width/height, standards mode (801 bytes, text/html)
2012-06-26 13:48 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details
test for caption/cell width/height, quirks mode (785 bytes, text/html)
2012-06-26 13:48 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details
Table Cell Height Patch v7 (4.00 KB, patch)
2012-06-26 14:55 PDT, Tal Aloni
dbaron: review+
Details | Diff | Review
Table Cell Height Patch v8 (4.72 KB, patch)
2012-06-27 05:33 PDT, Tal Aloni
dbaron: review+
Details | Diff | Review

Description XFox 2004-06-22 19:57:47 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a2) Gecko/20040621
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a2) Gecko/20040621

If you create a td that is styled to be 16x16 and give it a 4px border-right,
the td will be 20px in width (try this with a background-color to verify). Same
goes for border-left. However, if you apply a 4px border-top or border-bottom,
the td will still be 16px in height. The 4px border will overlap in the 16px
height rather than add the 4px to it.

Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Actual Results:  
border-top and border-bottom border lengths don't add the necessary height.

Expected Results:  
Giving a 16x16 td a 4px border-top / border-bottom should add 4px to the height
in the same manner that border-left /border-right add to the width.

Seems to be a flaw in the CSS Box Model. If you specify both width and height
length and color through the shorthand border: property, it is handled properly.
Comment 1 XFox 2004-06-22 20:01:13 PDT
Created attachment 151498 [details]
Shows an example of how border-top / bottom don't match border-left / right's behavior

The <td> created in this attached page is supposed to be square but the border
types aren't treated the same.
Comment 2 Bill Mason 2004-06-22 23:51:26 PDT
WFM 20040622 PC/WinXP
Comment 3 XFox 2004-06-23 09:28:26 PDT
Comment on attachment 151498 [details]
Shows an example of how border-top / bottom don't match border-left / right's behavior

<?xml version="1.0" encoding="ISO-8859-1"?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en">
<head>
<title> New Document </title>
<style type="text/css">
td{background-color:red;width:16px;height:16px}
</style>
</head>

<body>

<table cellspacing="0" cellpadding="0">
<tr><td style="border-right:4px outset blue;border-top:4px outset
green"></td></tr>
</table>

</body>
</html>
Comment 4 XFox 2004-06-23 09:31:35 PDT
Comment on attachment 151498 [details]
Shows an example of how border-top / bottom don't match border-left / right's behavior

Uploading a new attachment (the current one had all borders. I meant to undo
these changes before uploading.
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-07-10 23:00:58 PDT
Could you actually attach the testcase that shows the problem?
Comment 6 XFox 2004-07-11 19:30:18 PDT
Created attachment 152897 [details]
TESTCASE

My original attachment was intended to be a textcase (before I know what a
textcase was). I tried editing it since I applied the values to all borders,
but that resulted in making a reply to the thread with the text of the
attachment. This time I'll create a new attachment from scratch. This will be
my first testcase and I don't know if I'm doing it right (all I know is that
Keywords aren't in my permissions yet, so I'll put TESTCASE as the comment).

In the attachment, there is a table with 0 for cellspacing and cellpadding.
There is no border specified for the table. The td tag has no content. Through
CSS, the td is told to be 32x32 and colored gray. Also through CSS, it has an
8px solid red top and right border. The result is a 40x32 cell. 8px  of the
40px (this is expected). The height is 32px instead of the expected 40px. Of
that 32px, 8px is the red border and 24px is the gray. The height of the top
border was not added to the height, but it has overwritten the top 8px of the
existing 32px  .
Comment 7 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2004-07-11 23:50:13 PDT
Created attachment 152910 [details]
testcase

That certainly shows the bug, which is what we need.  But it's even better to
have testcases like this, where it's immediately obvious whether the bug is
still present.
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2004-07-13 17:59:06 PDT
Looks like making table cells have content-box box-sizing wasn't quite
successful....
Comment 9 Mike Mimic 2004-09-27 17:37:02 PDT
I also found this bug. I noticed that attached testcase IE 6 and Mozilla 1.7.3
render differently. Mozilla counts top and bottom border 50% into height (so
that box height is 40px) and IE does not (it draws 10px border around 30px high
content space what makes box height 50px). I thought - IE is wrong. But it is not.

From http://www.w3.org/TR/REC-CSS2/images/tbl-spacing.gif it is clearly
visible that border is not counted into cell width (so it is probably
the same with height).

I added this attachment because it works in IE and does not work in Mozilla.
Comment 10 Mike Mimic 2004-09-27 17:41:01 PDT
Created attachment 160280 [details]
testcase

These boxes should be the same height.
Comment 11 Brian Polidoro 2006-09-05 18:24:11 PDT
*** Bug 351489 has been marked as a duplicate of this bug. ***
Comment 12 Brian Polidoro 2006-09-05 18:27:59 PDT
Created attachment 236904 [details]
padding testcase

From Bug 351489, this also happens with padding. This is attachment 152910 [details] with the border changed to padding.
Comment 13 T. Chan 2007-11-07 13:26:05 PST
Still broken in Camino 2007102517 (1.5.3), Firefox (Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-GB; rv:1.8.1.9) Gecko/20071025 Firefox/2.0.0.9)
Comment 14 T. Chan 2007-12-30 13:40:05 PST
Created attachment 294911 [details]
Padding testcase #2

There's a bug in Attachment 236904 [details] (padding testcase):
Firefox seems to assume a default padding of 1px (or so), which makes the boxes 34 pixels wide. Of course, adjusting padding-top from 1px to 12px is only an increase of 11px.

Fixed version sets the default padding (and border) to 0px (I suspect the standard doesn't specify a default padding or border).
Comment 15 Bernd 2007-12-31 05:00:51 PST
This goes wrong at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsTableRowFrame.cpp&rev=3.406&mark=637#617
where the style height is passed, while border and padding are ignored. The current behavior is basically corresponding to IE rendering in quirks mode. http://developer.mozilla.org/en/docs/CSS:-moz-box-sizing (see IE7 for reference).
Comment 16 Dave Townsend [:mossop] 2008-03-18 08:46:58 PDT
*** Bug 364585 has been marked as a duplicate of this bug. ***
Comment 17 Daniel.S 2009-04-19 02:20:38 PDT
(In reply to comment #15)
> The current behavior is basically corresponding to IE rendering in quirks mode.

In IEs Standards Mode, the cell height is calculated using the content-box model.
The same is true for Safari and seems to be in line with CSS 2.1;

Our current behavior should probably be quirks only. An alternate would be to define this on the UA style sheet level, so authors can overwrite the current layout which is not possible at the moment.
Comment 18 Bernd 2009-04-19 09:01:08 PDT
Daniel, a set of reftests and clear description how IE8, Safari behaves
+ testing of quirks mode /standards mode
+ rowspans
+ interference of row height and cell height
+ testing of the box-sizing https://developer.mozilla.org/en/CSS/box-sizing in different browsers for table cells and rows

and the code changes should be fairly easy.
Comment 19 Daniel.S 2009-04-25 13:54:55 PDT
Created attachment 374610 [details]
Research on table, row, cell and height

Bernd, this is only partially what you asked for, but maybe it's useful.
Unfortunately, I won't have the time to complete this in the near future.
Comment 20 Bernd 2009-04-26 21:33:19 PDT
Daniel, its indeed useful. Don't feel pressured, this is table land where bugs have time to mature ;-) .

side note:

1. I am very grateful for the work that you are doing.
2. QA with the ability to read a spec together with a working build environment is a dream combination (read the early bugs of bz and dbaron and you will recognize the path)
Comment 21 Tal Aloni 2012-06-08 07:54:16 PDT
Created attachment 631395 [details] [diff] [review]
Table Height Bugfix Patch

This patch fixes the incompatiblity problem with the CSS standard.
The height calculation now uses the '-moz-box-sizing' property to determine which box-sizing model to use,  and we use the content-box model if none is specified, as mandated by CSS specs.

p.s.
The file titled 'padding testcase' is an invalid test case, and there should be a difference of 1px in height between the cells (because by default firefox uses 1px padding for each table cell)
Comment 22 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-08 08:47:46 PDT
Comment on attachment 631395 [details] [diff] [review]
Table Height Bugfix Patch

>--- C:/FirefoxOriginalLayout/layout/tables/nsTableRowFrame.cpp	Fri Jun 01 14:04:13 2012
>+++ C:/firefox/mozilla-release/layout/tables/nsTableRowFrame.cpp	Fri Jun 08 15:46:26 2012
>@@ -636,11 +636,24 @@
>   PRInt32 rowSpan = tableFrame->GetEffectiveRowSpan(*aCellFrame);
>   
>   switch (position->mHeight.GetUnit()) {
>-    case eStyleUnit_Coord:
>+    case eStyleUnit_Coord: {
>+      nscoord outsideBoxSizing = 0;
>+      switch (position->mBoxSizing) {
>+        case NS_STYLE_BOX_SIZING_CONTENT:
>+          outsideBoxSizing = aCellFrame->GetUsedBorderAndPadding().TopBottom();
>+          break;
>+        case NS_STYLE_BOX_SIZING_PADDING:
>+          outsideBoxSizing = aCellFrame->GetUsedBorder().TopBottom();
>+          break;
>+      }

Some compilers are going to give a warning if you don't have a default: case in this switch, which is probably a good idea for readability since there is in fact another case.


>       specifiedHeight = position->mHeight.GetCoordValue();
>+      specifiedHeight += outsideBoxSizing;

I think we should probably be making this case only for standards mode, not for quirks mode.

Also, you should just change the previous line and use + rather than using +=.

>       if (1 == rowSpan) 
>         SetFixedHeight(specifiedHeight);
>       break;
>+	}

Use spaces here rather than a tab.

Do we need to make any similar changes for widths?
Comment 23 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-08 08:49:26 PDT
Also, you should really do development against mozilla-central, not mozilla-release (if the name of your directory indicates what repository you're working against).
Comment 24 Tal Aloni 2012-06-08 09:39:50 PDT
Created attachment 631434 [details] [diff] [review]
Table Height Height Bugfix Patch v2

Thanks David,
You are correct that we should making this case only for standards mode, not for quirks mode, fixed that in v2.

> Do we need to make any similar changes for widths?
My tests suggests that width is already properly set for <td> elements.
(i.e. set based on the content-box model),
it would be nice to have support for 'border-box' and 'padding-box' for the width as well, but it is a different matter than fixing this CSS violation bug.

Tal
Comment 25 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-08 14:01:41 PDT
Comment on attachment 631434 [details] [diff] [review]
Table Height Height Bugfix Patch v2

Perhaps a better way to make this apply to standards mode only would be to put td { -moz-box-sizing: border-box } in (the existing rule with selector "td") in layout/style/html.css.  (Much like the rule that's there for table.)
Comment 26 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-08 14:02:16 PDT
(Yes, I should have realized that the last time; sorry.)
Comment 27 Tal Aloni 2012-06-08 22:28:04 PDT
David,
Unless I'm mistaken, 'layout/style/html.css' will apply to standards mode as well, and will change the default from the mandated 'content-box' to 'border-box'.

It is my opinion that we should always use 'content-box' in the standards mode.
Comment 28 Tal Aloni 2012-06-08 23:26:21 PDT
Created attachment 631619 [details] [diff] [review]
Table Cell Height Bugfix Patch v3

David,
I believe you were referring to 'quirk.css', which is a good idea,
here is an updated patch.
I also moved two occurences of '-moz-box-sizing: border-box' from 'html.css' to 'quirk.css' to comply with CSS specs.
Comment 29 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-08 23:45:37 PDT
Yes, I did mean quirk.css, sorry.

But let's do one thing at a time -- there's research here saying that based on what other browsers do, we can make the change for table cells.  I'd want to see similar testing of other browsers for the other changes, and they'd probably belong in other bugs.
Comment 30 Tal Aloni 2012-06-09 01:40:44 PDT
Created attachment 631630 [details]
Caption Test Case 1
Comment 31 Tal Aloni 2012-06-09 01:41:56 PDT
Created attachment 631631 [details]
Table Test Case 1
Comment 32 Tal Aloni 2012-06-09 01:44:53 PDT
David,
my standpoint is that we should never violate the CSS specs, regardless of what other browsers do, otherwise it's slippery slope.

I added two additional test cases for the two other violations I've suggested to fix:
1. Table caption test case:
All other major browsers do not violate the CSS specs (i.e. use the content-box model):
- Opera 11.64
- Google Chrome 19.0.1084.52 m
- IE8
- Safari 5.1.7

2. Table test case:
All of the other major browsers DO violate the CSS specs, however, the CSS specs ( http://www.w3.org/TR/CSS2/tables.html#separated-borders ) are crystal clear about what's right, and we must follow the specs.
Comment 33 Tal Aloni 2012-06-09 02:05:03 PDT
Created attachment 631636 [details] [diff] [review]
Table Cell Height Patch v4

Well, never mind about the Table Test Case, The specs are not on par with HTML and XHTML1, and this issue does not belong to the current discussion.

Attached an updated patch, I'm still including the fix for the table caption.
Comment 34 Tal Aloni 2012-06-13 13:14:09 PDT
I just now ran the reftests,
here is my favorite quote, taken from 'bc_borderoffset1_ref.html':
<!-- the height for table cells includes the border -->

:facepalm:
needless to say, my patch did not pass several tests.
Comment 35 Bernd 2012-06-13 13:52:26 PDT
If the tests are not correct fix them, they are not carved in stone
Comment 36 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-13 14:24:10 PDT
(In reply to Tal Aloni from comment #32)
> 2. Table test case:
> All of the other major browsers DO violate the CSS specs, however, the CSS
> specs ( http://www.w3.org/TR/CSS2/tables.html#separated-borders ) are
> crystal clear about what's right, and we must follow the specs.

In that case, we should get the specs fixed.  That's much simpler than getting all browsers to change and has the advantage that it won't break content already on the Web that depends on the current behavior.
Comment 37 Tal Aloni 2012-06-13 14:42:15 PDT
David, I believe the table test issue is going to be resolved with CSS3's box-sizing, with currently accepted behavior going to be the default using User Agent style-sheet, like we already do ATM with -moz-box-sizing

p.s.
I will post fixed reftests as suggested
Comment 38 Tal Aloni 2012-06-15 06:35:03 PDT
Created attachment 633497 [details] [diff] [review]
Table Cell Height Patch v4 (With updated reftests)
Comment 39 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-25 13:11:30 PDT
Created attachment 636464 [details]
Caption Test Case 1, in quirks mode
Comment 40 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-25 13:25:33 PDT
Comment on attachment 633497 [details] [diff] [review]
Table Cell Height Patch v4 (With updated reftests)

There's no basis for moving the caption box-sizing declaration
to quirks.css.  It should just be removed.  As far as I can tell, no
other browsers (tested Chrome, Opera, IE10) have box-sizing: border-box
for captions in either standards mode or quirks mode.  We shouldn't
introduce a new quirks/standards difference if there's no need to do
so.  Therefore, you should remove these added lines in quirks.css:
>+caption {
>+  -moz-box-sizing: border-box;
>+}
>+
and fix any tests as necessary.

You should also add a test to test that captions use content-box
sizing, since there appears to be no test for that.

r=dbaron with that fixed, though if the test changes are nontrivial I'd like to look at them
Comment 41 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-25 14:19:16 PDT
Comment on attachment 633497 [details] [diff] [review]
Table Cell Height Patch v4 (With updated reftests)

Actually, there's a bit more testing that needs to happen here.  In particular, you need to test width behavior for both table cells and table captions (in both standards mode and quirks mode).  Could you attach additional testcases to the bug for those cases?

I fear that this patch may be making table cell width behavior more quirky than it was before, though I could be wrong.

(Changing marking to review- since I want to look at this again once you've responded to this issue; that doesn't mean I won't grant review+ on the next patch.)
Comment 42 Tal Aloni 2012-06-25 15:43:45 PDT
Created attachment 636514 [details] [diff] [review]
Table Cell Height Patch v5

David,
You are correct to point out that the caption box-sizing declaration should just be removed. I have removed it, there is no reftest regression as a result.
Other browsers (Chrome, Safari, Opera) use the content-box for both quirks and standards mode when it comes to table caption.

As I mentioned above, width is already properly set for <td> elements (content box), this patch does not change that.
While it's true that it's somewhat confusing that -moz-box-sizing will only be applied to table cell height after this patch is committed, in my opinion it's not "more quirky than it was before".

I did wrote a separate patch that apply -moz-box-sizing to table width, but I preferred to seperate the two, as this one addresses CSS violation and the other is merely a feature.
Comment 43 Tal Aloni 2012-06-25 15:49:42 PDT
in the above paragraph: "table width" should be "table cell width".
Comment 44 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-25 16:33:49 PDT
(In reply to Tal Aloni from comment #42)
> I did wrote a separate patch that apply -moz-box-sizing to table cell width, but
> I preferred to seperate the two, as this one addresses CSS violation and the
> other is merely a feature.

But if you do that, it will suddenly start applying the declaration that's in quirks.css to table cell widths too, which isn't what we want.

I think we might need to apply -moz-box-sizing separately for widths vs. heights.  And at the very least we'd need to do that before taking both this fix and the other one you describe.  But I suppose we could probably take one of them, as long as we have tests to verify that we don't take both.
Comment 45 Tal Aloni 2012-06-25 16:43:45 PDT
(In reply to David Baron [:dbaron] from comment #44)
> But if you do that, it will suddenly start applying the declaration that's
> in quirks.css to table cell widths too, which isn't what we want.
It's exactly what we want,
in quirks mode, table cell box-sizing should be border-box for both width and height, this is the old html specification, this is the solution suggested by CSS3, and this is how other browsers behave.
Comment 46 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-25 21:25:19 PDT
(In reply to Tal Aloni from comment #45)
> It's exactly what we want,
> in quirks mode, table cell box-sizing should be border-box for both width
> and height, this is the old html specification, this is the solution
> suggested by CSS3, and this is how other browsers behave.

OK, sounds good, then.  I didn't see any tests for that here, though.
Comment 47 Tal Aloni 2012-06-25 23:33:33 PDT
(In reply to David Baron [:dbaron] from comment #46)
> I didn't see any tests for that here, though.
This is because this patch does not affect <td> width, not directly, and not indirectly (because ATM <td> height was and still is content-box, regardless of -moz-box-sizing)

You're going to see the relevant tests with the other patch, I do not want to create a mess and mix the two separate issues.
Comment 48 Tal Aloni 2012-06-25 23:34:51 PDT
in the paragraph above: ..<td> *width* was and still is...
Comment 49 Simon Pieters 2012-06-26 04:54:19 PDT
(In reply to Tal Aloni from comment #45)
> (In reply to David Baron [:dbaron] from comment #44)
> > But if you do that, it will suddenly start applying the declaration that's
> > in quirks.css to table cell widths too, which isn't what we want.
> It's exactly what we want,
> in quirks mode, table cell box-sizing should be border-box for both width
> and height, this is the old html specification, this is the solution
> suggested by CSS3, and this is how other browsers behave.

standards
http://software.hixie.ch/utilities/js/live-dom-viewer/saved/1617

quirks
http://software.hixie.ch/utilities/js/live-dom-viewer/saved/1618

Opera and current Firefox have border-box for height but not for width, in both modes.
Chrome and IE8 have border-box for height but not for width in quirks mode, and content-box for both in standards mode.
Comment 50 Tal Aloni 2012-06-26 06:43:30 PDT
Thanks for pointing that out Simon, regarding cell width in quirks mode, I mixed up between table and the table cell, Sorry and thanks for the correction!

Both Opera and Firefox should use content-box by default for table cell height in standards mode, any other option is a CSS violation.

Quirks mode:
------------
You are correct to suggest that in quirks mode some browsers use different box-sizing method for width and height, IE6 / IE8 / Chrome / Safari use *padding-box* for height and content-box for width. I guess we should thank Microsoft for that.
Comment 51 Tal Aloni 2012-06-26 07:00:29 PDT
Sorry, not *padding-box* but border-box.
Comment 52 Tal Aloni 2012-06-26 07:10:46 PDT
(In reply to David Baron [:dbaron] from comment #44)
> I think we might need to apply -moz-box-sizing separately for widths vs.
> heights.

I'm not sure what you mean by that, it would be great If I could differentiate between the two:
1. -moz-box-sizing was set to content-box (default)
2. -moz-box-sizing was not set at all.

any idea how to do that?
Comment 53 Simon Pieters 2012-06-26 07:53:21 PDT
(In reply to Tal Aloni from comment #50)
> Thanks for pointing that out Simon, regarding cell width in quirks mode, I
> mixed up between table and the table cell, Sorry and thanks for the
> correction!
> 
> Both Opera and Firefox should use content-box by default for table cell
> height in standards mode, any other option is a CSS violation.

Well we could get the CSS spec changed, right? Tables in CSS are pretty underdefined anyway. Is it good to have a difference between quirks and standards here? Are there any Web pages that rely on the IE/Chrome behavior in standards mode?

> Quirks mode:
> ------------
> You are correct to suggest that in quirks mode some browsers use different
> box-sizing method for width and height, IE6 / IE8 / Chrome / Safari use
> *padding-box* for height and content-box for width. I guess we should thank
> Microsoft for that.

If we want to have a difference between quirks and standards mode here, should the quirk apply to only HTML <td> and <th> elements or all CSS 'table-cell's? Only <td> and <th> if they're 'table-cell'? I ask because I'd like to spec this in http://simon.html5.org/specs/quirks-mode if we go this route. Cheers.
Comment 54 Simon Pieters 2012-06-26 08:11:47 PDT
(I filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=17606 about border-box on <table>)
Comment 55 Simon Pieters 2012-06-26 08:15:10 PDT
(In reply to Simon Pieters from comment #53)
> Well we could get the CSS spec changed, right?

Alternatively, if it should only apply to HTML <td> and <th> (in all modes), it could be added to the Rendering section of the HTML spec.
Comment 56 Tal Aloni 2012-06-26 08:17:36 PDT
(In reply to Simon Pieters from comment #53)
> Well we could get the CSS spec changed, right?
Simon, It would be better it you would simply make sure Opera match the standard, as I try to do here for firefox. I strongly believe that this is the right thing to do, Thanks!

> If we want to have a difference between quirks and standards mode here,
> should the quirk apply to only HTML <td> and <th> elements or all CSS
> 'table-cell's? Only <td> and <th> if they're 'table-cell'?
It only really matters for HTML <td> and <th> elements.
Earlier versions of IE do not support 'display:table-cell', so it's not very common with pages that have no DOCTYPE.

> (I filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=17606 about
> border-box on <table>)
Thanks, AFAIK this is already planned for CSS3.
Comment 57 Tal Aloni 2012-06-26 13:31:42 PDT
Created attachment 636852 [details] [diff] [review]
Table Cell Height Patch v6

Note:
This patch is equivalent to v5, the only difference is that now border-box is hard-coded for quirks mode, instead of relying on quirk.css,
This is because there is an historic anomaly in quirks mode: table cell width is content-box, but height is border-box.

I made this change to prepare the ground for a patch that adds support for box-sizing on cell width.
Comment 58 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-26 13:48:08 PDT
Created attachment 636864 [details]
test for caption/cell width/height, standards mode
Comment 59 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-26 13:48:32 PDT
Created attachment 636865 [details]
test for caption/cell width/height, quirks mode
Comment 60 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-26 13:58:48 PDT
Comment on attachment 636852 [details] [diff] [review]
Table Cell Height Patch v6

>+      // In quirks mode, table cell width should be content-box, but height should be border-box
>+      // Because of this historic anomaly, we do not use quirk.css

Could you:
 * wrap this comment before the 80th column (and use a . at the end of the first sentence)
 * add to the second sentence something like "since we can't specify one value of box-sizing for width and another for height"

r=dbaron with that

Also, the patch should have appropriate user and commit message data inside the patch for checkin.  I'd suggest the commit message (all as one line):

Change standards mode height calculations for table cells to use content-box sizing rather than border-box sizing by default (and to honor -moz-box-sizing, which we do not do in quirks mode).  Also remove -moz-box-sizing: border-box from default style for caption element (all modes).  (Bug 248239)  r=dbaron
Comment 61 Tal Aloni 2012-06-26 14:55:38 PDT
Created attachment 636886 [details] [diff] [review]
Table Cell Height Patch v7
Comment 62 Tal Aloni 2012-06-26 15:03:35 PDT
Thanks for your help David,
Since this is a bugfix, I suggest to fast-track this, and push it to central, aurora and beta branch.
Comment 63 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-26 16:28:41 PDT
Pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e2b9cdc486b
from which it will get merged to mozilla-central (unless it gets backed out for test failures).


(In reply to Tal Aloni from comment #62)
> Since this is a bugfix, I suggest to fast-track this, and push it to
> central, aurora and beta branch.

This should not land on aurora or beta.  It's a fix for a longstanding bug, and it has some risk that it will break browser-specific content (for which we should at least give sites the full aurora/beta cycle), or that it actually does something that's not compatible with other browsers and breaks cross-browser content (for which we use the time on aurora or beta to get bug reports).
Comment 64 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-26 16:29:59 PDT
Comment on attachment 636886 [details] [diff] [review]
Table Cell Height Patch v7

Also, you should generate diffs using "hg diff" (or by pulling patches out of a mercurial queue), not with tools that generate diffs relative to a different root (i.e., inside mozilla/layout/, which hg diff would not generate).
Comment 65 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-26 18:16:48 PDT
Backed out of mozilla-inbound due to:

1063 ERROR TEST-UNEXPECTED-FAIL | /tests/MochiKit-1.4.2/tests/test_MochiKit-Style.html | default left table cell content h ok - got 34, expected 30
1065 ERROR TEST-UNEXPECTED-FAIL | /tests/MochiKit-1.4.2/tests/test_MochiKit-Style.html | default middle table cell content h ok - got 34, expected 30
1067 ERROR TEST-UNEXPECTED-FAIL | /tests/MochiKit-1.4.2/tests/test_MochiKit-Style.html | default right table cell content h ok - got 34, expected 30
1069 ERROR TEST-UNEXPECTED-FAIL | /tests/MochiKit-1.4.2/tests/test_MochiKit-Style.html | collapsed left table cell content h ok - got 32, expected 30
1071 ERROR TEST-UNEXPECTED-FAIL | /tests/MochiKit-1.4.2/tests/test_MochiKit-Style.html | collapsed middle table cell content h ok - got 32, expected 30
1073 ERROR TEST-UNEXPECTED-FAIL | /tests/MochiKit-1.4.2/tests/test_MochiKit-Style.html | collapsed right table cell content h ok - got 32, expected 30
Comment 66 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-26 18:17:47 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9126626a41b
Comment 67 Tal Aloni 2012-06-26 23:34:31 PDT
Sorry about that, I thought that running 'make reftest' covers all tests, I'll review the Mochitests.
Comment 68 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-26 23:41:57 PDT
You can run just that one mochitest relatively easily, by setting the TEST_PATH environment variable.

The question is whether the mochitest in question is something that passes in other browsers as well.  Since it's part of the MochiKit tests, I'm worried that it may well be.
Comment 69 Tal Aloni 2012-06-27 02:13:43 PDT
David,
The Mochitest have a specific check for user agent in getElementDimensions,
and for Gecko (and Opera) it compensates for the old (incorrect) behavior,
So the Mochitest javascript library has to be updated (this odd quirk has to be removed), I'll make some more tests and post an updated patch.
Comment 70 Simon Pieters 2012-06-27 05:23:45 PDT
(In reply to Tal Aloni from comment #56)
> It only really matters for HTML <td> and <th> elements.

http://software.hixie.ch/utilities/js/live-dom-viewer/saved/1630
http://software.hixie.ch/utilities/js/live-dom-viewer/saved/1631

AFAICT Opera and Firefox apply the quirk to display:table-cell elements but not to <td> and <th> elements that are display:block. (WebKit doesn't seem to support display:block on <td> in quirks mode.) So I guess I'll spec that.
Comment 71 Tal Aloni 2012-06-27 05:33:04 PDT
Created attachment 637085 [details] [diff] [review]
Table Cell Height Patch v8

Removed the bug workaround from the MochiKit library.
Comment 73 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-29 11:52:21 PDT
Comment on attachment 637085 [details] [diff] [review]
Table Cell Height Patch v8

r=dbaron, but could you get in touch with the authors of MachiKit and let them know they're going to need to change their version-sniffing here.
Comment 74 Tal Aloni 2012-06-29 12:21:47 PDT
Sure thing David,
I've sent an email to Bob Ippolito, the maintainer.
Comment 75 Ryan VanderMeulen [:RyanVM] 2012-06-30 09:04:22 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/4dd209b82d90

Thanks for the patch, Tal! To make life easier for those checking in on your behalf, please follow the directions at the link below for any future patches you submit. Thanks!
https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in
Comment 76 Ryan VanderMeulen [:RyanVM] 2012-06-30 12:40:34 PDT
https://hg.mozilla.org/mozilla-central/rev/4dd209b82d90
Comment 77 j.j. 2012-07-02 12:15:11 PDT
I wrote down the newly introduced quirks/standards mode difference. Please have a look and correct if something is wrong or my wording is ugly.

https://developer.mozilla.org/en/Mozilla_Quirks_Mode_Behavior#Tables
Comment 78 Tal Aloni 2012-07-02 12:25:15 PDT
j.j.
you wrote "...remains as quirk for compatibility with other browsers",
I think a more accurate description would be:
"...remains as quirk for compatibility with *older* browsers",
(which actually refers to old versions of IE)

Thanks,
Tal
Comment 79 j.j. 2012-07-02 12:38:15 PDT
(In reply to Tal Aloni from comment #78)
> j.j.
> you wrote "...remains as quirk for compatibility with other browsers",
> I think a more accurate description would be:
> "...remains as quirk for compatibility with *older* browsers"

No, current WebKit and IE versions have this behaviour in quirks mode and will most likely never change it in future. 
But I will change to 
...remains in quirks mode for compatibility with other browsers quirks mode behaviour
Comment 80 Tal Aloni 2012-07-02 12:45:32 PDT
That's even more accurate, thanks!
Comment 81 j.j. 2012-07-02 12:52:24 PDT
*** Bug 295315 has been marked as a duplicate of this bug. ***
Comment 82 :Gijs Kruitbosch 2014-11-19 17:00:44 PST
*** Bug 501181 has been marked as a duplicate of this bug. ***

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