Last Comment Bug 484825 - Border weirdness when table cell is display:inline
: Border weirdness when table cell is display:inline
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on: 43178 211636
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-23 10:44 PDT by Boris Zbarsky [:bz]
Modified: 2011-08-22 10:12 PDT (History)
6 users (show)
bernd_mozilla: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (294 bytes, text/html)
2009-03-23 10:44 PDT, Boris Zbarsky [:bz]
no flags Details
Shows issue in Fx2 and Fx3 (155 bytes, text/html)
2009-03-24 08:32 PDT, Boris Zbarsky [:bz]
no flags Details
reference (387 bytes, text/html)
2011-08-22 08:03 PDT, Bernd
no flags Details

Description Boris Zbarsky [:bz] 2009-03-23 10:44:59 PDT
Created attachment 368918 [details]
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 [:bz] 2009-03-23 10:45:48 PDT
Oh, hmm.  This seems to be a regression from fx3.  I'll dig into that.
Comment 2 Boris Zbarsky [:bz] 2009-03-23 10:54:23 PDT
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 [:bz] 2009-03-23 13:04:17 PDT
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 [:bz] 2009-03-24 08:21:16 PDT
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 [:bz] 2009-03-24 08:32:35 PDT
Created attachment 369086 [details]
Shows issue in Fx2 and Fx3
Comment 6 Bernd 2009-03-24 13:13:41 PDT
This is fixed in tryserver builds for bug 43178 https://build.mozilla.org/tryserver-builds/2009-03-07_13:53-bmlk@gmx.de-1236462779/
Comment 7 Boris Zbarsky [:bz] 2009-03-24 14:50:16 PDT
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 2009-03-24 22:43:53 PDT
-// 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 [:bz] 2009-03-24 22:47:03 PDT
But the code is just moved, not removed, no?
Comment 10 Bernd 2009-03-24 23:20:45 PDT
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 [:bz] 2009-03-24 23:37:03 PDT
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 2009-03-25 13:44:26 PDT
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 2009-03-25 13:54:23 PDT
>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 2010-01-10 08:06:13 PST
the borders on the td are now inset but the missing border-spacing on the last td remains.
Comment 15 Bernd 2011-08-21 22:50:00 PDT
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+2 (mostly busy through August 4; review requests must explain patch) 2011-08-22 07:07:42 PDT
How is the new behavior incorrect?
Comment 17 Bernd 2011-08-22 08:03:22 PDT
Created attachment 554867 [details]
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+2 (mostly busy through August 4; review requests must explain patch) 2011-08-22 09:28:55 PDT
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 2011-08-22 10:12:36 PDT
>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

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