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)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bzbarsky, Unassigned)
References
Details
(Keywords: regression)
Attachments
(3 files)
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?
Reporter | ||
Comment 1•16 years ago
|
||
Oh, hmm. This seems to be a regression from fx3. I'll dig into that.
Reporter | ||
Comment 2•16 years ago
|
||
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.
Updated•16 years ago
|
Keywords: regression
Reporter | ||
Comment 3•16 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
Reporter | ||
Comment 4•16 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
Reporter | ||
Comment 5•16 years ago
|
||
This is fixed in tryserver builds for bug 43178 https://build.mozilla.org/tryserver-builds/2009-03-07_13:53-bmlk@gmx.de-1236462779/
Reporter | ||
Comment 7•16 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.
-// 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 ?
Reporter | ||
Comment 9•16 years ago
|
||
But the code is just moved, not removed, no?
Comment 10•16 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 ;-))
Reporter | ||
Comment 11•16 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•16 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•16 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•15 years ago
|
||
the borders on the td are now inset but the missing border-spacing on the last td remains.
Comment 15•13 years ago
|
||
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•13 years ago
|
||
How is the new behavior incorrect?
Comment 17•13 years ago
|
||
I believe the testcase and the reference should render identical but they differ below the last row, the testcase eats the cellspacing
Comment 18•13 years ago
|
||
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
Comment 19•13 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.
Description
•