Closed Bug 484825 Opened 16 years ago Closed 13 years ago

Border weirdness when table cell is display:inline

Categories

(Core :: CSS Parsing and Computation, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Unassigned)

References

Details

(Keywords: regression)

Attachments

(3 files)

Attached file Testcase (deleted) —
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?
Oh, hmm. This seems to be a regression from fx3. I'll dig into that.
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.
Keywords: regression
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
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
Attached file Shows issue in Fx2 and Fx3 (deleted) —
Depends on: 43178
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.
-// 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 ?
But the code is just moved, not removed, no?
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 ;-))
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?
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; +}
>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)
the borders on the td are now inset but the missing border-spacing on the last td remains.
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.
How is the new behavior incorrect?
Attached file reference (deleted) —
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.)
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
>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.

Attachment

General

Created:
Updated:
Size: