Closed Bug 248239 Opened 20 years ago Closed 12 years ago

borders and padding on the top and bottom of table cells reduce the height

Categories

(Core :: Layout: Tables, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla16

People

(Reporter: TailsTheKitsune, Assigned: tal.aloni.il)

References

Details

(Keywords: dev-doc-needed, testcase)

Attachments

(10 files, 11 obsolete files)

testcase
580 bytes, text/html; charset=UTF-8
Details
testcase
909 bytes, text/html
Details
padding testcase
599 bytes, text/html
Details
Padding testcase #2
602 bytes, text/html
Details
Research on table, row, cell and height
7.48 KB, text/html
Details
Caption Test Case 1
876 bytes, text/html
Details
Caption Test Case 1, in quirks mode
774 bytes, text/html
Details
test for caption/cell width/height, standards mode
801 bytes, text/html
Details
test for caption/cell width/height, quirks mode
785 bytes, text/html
Details
Table Cell Height Patch v8
4.72 KB, patch
dbaron
: review+
Details | Diff | Splinter Review 
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.
The <td> created in this attached page is supposed to be square but the border
types aren't treated the same.
WFM 20040622 PC/WinXP
Assignee: general → dbaron
Component: Browser-General → Style System (CSS)
QA Contact: general → ian
Attachment #151498 - Attachment mime type: text/html → application/xhtml+xml
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 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.
Could you actually attach the testcase that shows the problem?
Attached file TESTCASE (obsolete) — 
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  .
Attachment #151498 - Attachment is obsolete: true
Attached file 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.
Attachment #152897 - Attachment is obsolete: true
Assignee: dbaron → nobody
Status: UNCONFIRMED → NEW
Component: Style System (CSS) → Layout: Tables
Ever confirmed: true
QA Contact: ian → core.layout.tables
Summary: CSS border-left / right px add to width but top / bottom do not.add to height → bottom and top borders on table cells reduce the height
Looks like making table cells have content-box box-sizing wasn't quite
successful....
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.
Attached file testcase — 
These boxes should be the same height.
*** Bug 351489 has been marked as a duplicate of this bug. ***
Attached file padding testcase — 
From Bug 351489, this also happens with padding. This is attachment 152910 [details] with the border changed to padding.
Summary: bottom and top borders on table cells reduce the height → borders and padding on the top and bottom of table cells reduce the height
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)
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9-
Attached file 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).
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).
Keywords: testcase
(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.
Flags: wanted1.9.2?
OS: Windows XP → All
Hardware: x86 → All
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.
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.
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)
Flags: wanted1.9.2?
Flags: wanted1.9.2-
Flags: blocking1.9.2-
Attached patch Table Height Bugfix Patch (obsolete) — — Splinter Review 
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 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?
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).
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
Attachment #631395 - Attachment is obsolete: true
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.)
Attachment #631434 - Flags: review?(dbaron)
(Yes, I should have realized that the last time; sorry.)
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.
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.
Attachment #631434 - Attachment is obsolete: true
Attachment #631434 - Flags: review?(dbaron)
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.
Attached file Caption Test Case 1 — 

    
    

    
  

      
      
      
      

        Attached file
          
        Table Test Case 1 (obsolete)
        — 
      

    


  

    
    

    
  
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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Table Cell Height Patch v4 (obsolete)
        — 
          — Splinter Review
      

    


  
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.

        Attachment #631619 -
        Attachment is obsolete: true

        Attachment #631631 -
        Attachment is obsolete: true

    
  

        Attachment #631636 -
        Attachment is patch: true

    
  

        Attachment #631636 -
        Attachment description: Table Cell Height Patch → Table Cell Height Patch v4

    
  

        Attachment #631630 -
        Attachment mime type: text/plain → text/html

    
    

    
  
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.

    
    

    
  
If the tests are not correct fix them, they are not carved in stone

    
    

    
  
(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.

    
    

    
  
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

    
    

    
  


  

        Attachment #631636 -
        Attachment is obsolete: true

        Attachment #631636 -
        Flags: review?(dbaron)

        Attachment #633497 -
        Flags: review?(dbaron)

    
    

    
  
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

        Attachment #633497 -
        Flags: review?(dbaron) → review+

    
    

    
  
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.)

        Attachment #633497 -
        Flags: review+ → review-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Table Cell Height Patch v5 (obsolete)
        — 
          — Splinter Review
      

    


  
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.

        Attachment #633497 -
        Attachment is obsolete: true

        Attachment #636514 -
        Flags: review?(dbaron)

    
    

    
  
in the above paragraph: "table width" should be "table cell width".

    
    

    
  
(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.

    
    

    
  
(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.

    
    

    
  
(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.

    
    

    
  
(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.

    
    

    
  
in the paragraph above: ..<td> *width* was and still is...

    
    

    
  
(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.

    
    

    
  
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.

    
    

    
  
Sorry, not *padding-box* but border-box.

    
    

    
  
(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?

    
    

    
  
(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.

    
    

    
  
(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.

    
    

    
  
(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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Table Cell Height Patch v6 (obsolete)
        — 
          — Splinter Review
      

    


  
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.

        Attachment #636514 -
        Attachment is obsolete: true

        Attachment #636514 -
        Flags: review?(dbaron)

        Attachment #636852 -
        Flags: review?(dbaron)

    
    

    
  
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

        Attachment #636852 -
        Flags: review?(dbaron) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Table Cell Height Patch v7 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #636852 -
        Attachment is obsolete: true

        Attachment #636886 -
        Flags: review?(dbaron)

    
    

    
  
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.

    
    

    
  
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).
Assignee: nobody → tal.aloni.il
Target Milestone: --- → mozilla16

    
    

    
  
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).

        Attachment #636886 -
        Flags: review?(dbaron) → review+

    
    

    
  
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

    
    

    
  
Sorry about that, I thought that running 'make reftest' covers all tests, I'll review the Mochitests.

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
(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.

    
    

    
  


  
Removed the bug workaround from the MochiKit library.

        Attachment #636886 -
        Attachment is obsolete: true

        Attachment #637085 -
        Flags: review?(dbaron)

    
    

    
  
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.

        Attachment #637085 -
        Flags: review?(dbaron) → review+

    
    

    
  
Sure thing David,
I've sent an email to Bob Ippolito, the maintainer.

    
  
Keywords: checkin-needed

    
    

    
  
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
Flags: wanted1.9.2-
Flags: in-testsuite+
Flags: blocking1.9.2-
Flags: blocking1.9-
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/4dd209b82d90
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED

    
  
Depends on: 770014

    
  
Depends on: 770045

    
  
Depends on: 770106

    
  
No longer depends on: 770106

    
    

    
  
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

    
    

    
  
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

    
    

    
  
(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

    
    

    
  
That's even more accurate, thanks!

    
  
Blocks: 338554

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: