484825 - Border weirdness when table cell is display:inline

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Testcase attached.  There should be no outset borders around the cells...  This is a modified version of the testcase from bug 156888 which is not dynamic.

I have no idea yet what's going on here.  Bernd, do you?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Oh, hmm.  This seems to be a regression from fx3.  I'll dig into that.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Regressed between 2008-12-23 (rev b2479ac7eab7) and 2008-12-24 (rev df94feb90a4f).  Pushlog:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b2479ac7eab7&tochange=df94feb90a4f

I'm guessing one of dbaron's changes in that range is the issue here.

Comment 3

[Boris Zbarsky [:bzbarsky]]

hg bisect pins the regression on bug 468473.  Commenting out the code added to nsFrame::DidSetStyleContext fixes the bug.  Leaving the GetStyleBorder() call but commenting out everything else makes the bug reappear.

So the issue is the GetStyleBorder() call.  Presumably we're somehow managing to end up with a struct cached in the ruletree and applying to various style contexts it shouldn't apply to.  Or something.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hmm.  So the issue is pretty clearly that the MapAttributesIntoRule in nsHTMLTableElement.cpp treats this case as the "non-cell" case and calls MapTableBorderInto.   See bug 211636.

The reason dynamically toggling display from cell to block doesn't show the problem is that in that case we first compute the border using the cell display type and cache it in the ruletree.  Then we toggle, and when resolving style find the cached data in the ruletree and use it.

The reason this used to work before the GetStyleBorder() call was added to DidSetStyleContext() likely has to do with the precise order in which the GetStyleBorder() calls happened on the ruletree or something.  Testcase coming up that shows the problem in Fx3 as well.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Shows issue in Fx2 and Fx3

Comment 6

[Bernd]

This is fixed in tryserver builds for bug 43178 https://build.mozilla.org/tryserver-builds/2009-03-07_13:[e-mail]-1236462779/

Comment 7

[Boris Zbarsky [:bzbarsky]]

Hmm.  I read that patch, and it didn't look like it should affect the codepath that causes this bug, since we still do weird mapping for @border...  So sounds like it changes the ordering on the testcases somehow or something.

Comment 8

[Bernd]

-// XXX The two callsites care about the two different halves of this
-// function, so split it, probably by just putting it in inline at the
-// callsites.
-static void 
-MapTableBorderInto(const nsMappedAttributes* aAttributes,
-                   nsRuleData* aData, PRUint8 aBorderStyle)
-{
-  const nsAttrValue* borderValue = aAttributes->GetAttr(nsGkAtoms::border);
-  if (!borderValue && !aAttributes->GetAttr(nsGkAtoms::frame))
-    return;
-
-  // the absence of "border" with the presence of "frame" implies
-  // border = 1 pixel
-  PRInt32 borderThickness = 1;
-
-  if (borderValue && borderValue->Type() == nsAttrValue::eInteger)
-    borderThickness = borderValue->GetIntegerValue();
-
-  if (aData->mTableData) {
-    if (0 != borderThickness) {
-      // border != 0 implies rules=all and frame=border
-      aData->mTableData->mRules.SetIntValue(NS_STYLE_TABLE_RULES_ALL, eCSSUnit_Enumerated);
-      aData->mTableData->mFrame.SetIntValue(NS_STYLE_TABLE_FRAME_BORDER, eCSSUnit_Enumerated);
-    }
-    else {
-      // border = 0 implies rules=none and frame=void
-      aData->mTableData->mRules.SetIntValue(NS_STYLE_TABLE_RULES_NONE, eCSSUnit_Enumerated);
-      aData->mTableData->mFrame.SetIntValue(NS_STYLE_TABLE_FRAME_NONE, eCSSUnit_Enumerated);
-    }
-  }
-
-  if (aData->mMarginData) {
-    // by default, set all border sides to the specified width
-    if (aData->mMarginData->mBorderWidth.mLeft.GetUnit() == eCSSUnit_Null)
-      aData->mMarginData->mBorderWidth.mLeft.SetFloatValue((float)borderThickness, eCSSUnit_Pixel);
-    if (aData->mMarginData->mBorderWidth.mRight.GetUnit() == eCSSUnit_Null)
-      aData->mMarginData->mBorderWidth.mRight.SetFloatValue((float)borderThickness, eCSSUnit_Pixel);
-    if (aData->mMarginData->mBorderWidth.mTop.GetUnit() == eCSSUnit_Null)
-      aData->mMarginData->mBorderWidth.mTop .SetFloatValue((float)borderThickness, eCSSUnit_Pixel);
-    if (aData->mMarginData->mBorderWidth.mBottom.GetUnit() == eCSSUnit_Null)
-      aData->mMarginData->mBorderWidth.mBottom.SetFloatValue((float)borderThickness, eCSSUnit_Pixel);
-
-    // now account for the frame attribute
-    MapTableFrameInto(aAttributes, aData, aBorderStyle);
-  }
-}
Might be responsible ?

Comment 9

[Boris Zbarsky [:bzbarsky]]

But the code is just moved, not removed, no?

Comment 10

[Bernd]

Most parts are removed, thats the whole point of the exercise, especially the mapping of border values to rules all which creates a border for the table cell, which plays a key role in this bug. OK its a cheap shot to kill this bug in this way, but I can't miss any opportunity to get bug 43178 reviewed, David  ;-))

Comment 11

[Boris Zbarsky [:bzbarsky]]

But the relevant code is the code that sets the border width and style, no?  And with that patch applied, the code for the width is still there in the "else { // table" branch in MapAttributesIntoRule:

+        // by default, set all border sides to the specified width

etc.  I guess the code for the border-style is gone because MapTableFrameInto went away?  If you set "border-style: solid" on that <td> in the testcase, does it get a 5px wide border?

Comment 12

[Bernd]

the border style code has been replaced by the style rules in html.css
 the relevant section is:

+/* Internal Table Borders */
+
+  /* 'border' cell borders first */
+table[border]:not([border="0"])> tr > td,
+table[border]:not([border="0"])> * > tr > td,
+table[border]:not([border="0"])> tr > th,
+table[border]:not([border="0"])> * > tr > th,
+table[border]:not([border="0"]) > td,
+table[border]:not([border="0"])> th,
+table[border]:not([border="0"])> * > td,
+table[border]:not([border="0"]) > * > th {
+  border: thin inset;
+}

Comment 13

[Bernd]

>If you set "border-style: solid" on that <td> in the testcase, does
>it get a 5px wide border?

yes its gets a 5px wide solid black border, there is no difference to the current trunk behavior (alhtough no other browser does that - IE,webkit,opera)

Comment 14

[Bernd]

the borders on the td are now inset but the missing border-spacing on the last td remains.

Comment 15

[Bernd]

the fix for bug 211636 http://hg.mozilla.org/mozilla-central/rev/4738b38a2f3c makes the inner borders as they are in other browsers this fixes comment 13 but the cellspacing remains.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

How is the new behavior incorrect?

Comment 17

[Bernd]

Attachment: reference

I believe the testcase and the reference should render identical but they differ below the last row, the testcase eats the cellspacing

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think that difference is correct behavior, because borders on inlines don't take up space in the inline box model -- the things that determine the height of the line are the font metrics and the 'line-height', not the borders.

If you change 'display:inline' to 'display:block' it does match the reference, which is as I'd expect.

So I claim this bug is fixed by bug 211636.  (Thanks for finishing that up and landing it, by the way.)

Comment 19

[Bernd]

>So I claim this bug is fixed by bug 211636.
thats fine with me, I looked up how it reflows and it happens as you said.
attachement 369086 needs to be converted into a reftest