Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Border weirdness when table cell is display:inline




CSS Parsing and Computation
9 years ago
6 years ago


(Reporter: bz, Unassigned)



Mac OS X
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)



(3 attachments)



9 years ago
Created attachment 368918 [details]

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

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

Comment 2

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

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

Comment 3

9 years ago
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.
Component: Layout: Tables → Style System (CSS)
QA Contact: layout.tables → style-system

Comment 4

8 years ago
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.
Depends on: 211636

Comment 5

8 years ago
Created attachment 369086 [details]
Shows issue in Fx2 and Fx3

Comment 6

8 years ago
This is fixed in tryserver builds for bug 43178


8 years ago
Depends on: 43178

Comment 7

8 years ago
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

8 years ago
-// 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

8 years ago
But the code is just moved, not removed, no?

Comment 10

8 years ago
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

8 years ago
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

8 years ago
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

8 years ago
>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

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

Comment 15

6 years ago
the fix for bug 211636 makes the inner borders as they are in other browsers this fixes comment 13 but the cellspacing remains.
How is the new behavior incorrect?

Comment 17

6 years ago
Created attachment 554867 [details]

I believe the testcase and the reference should render identical but they differ below the last row, the testcase eats the cellspacing
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.)
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 19

6 years ago
>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
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.