543791 - [BC] Inconsistent layout, removing frame="border" from table with rules="all"

Status: NEW

(Core :: Layout: Tables, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Comment 1

[Jesse Ruderman]

Attachment: reference (static)

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Nominating to get this on the radar to maybe fix random orange in bug 540360

Comment 3

[Bernd]

Attachment: testcase to show that the broken behavior is not new

Comment 4

[Bernd]

the following code from nsTableFrame.cpp

5286 void
5287 BCMapCellInfo::SetRightBorderWidths(BCPixelSize aWidth)
5288 {
5289    // update the borders of the cells and cols affected
5290   if (mCell) {
5291     mCell->SetBorderWidth(mEndSide, NS_MAX(aWidth,
5292                           mCell->GetBorderWidth(mEndSide)));
5293   }
5294   if (mRightCol) {
5295     BCPixelSize half = BC_BORDER_LEFT_HALF(aWidth);
5296     mRightCol->SetRightBorderWidth(NS_MAX(nscoord(half),
5297                                    mRightCol->GetRightBorderWidth()));
5298   }
5299 }
 
is broken for two reasons:
1. it looks for  the previous set border width to make sure that for colspans/rowspans the widest border segment is taken into account.  
However GetBorderWidth returns only a half of the border width

1112 BCPixelSize
1113 nsBCTableCellFrame::GetBorderWidth(PRUint8 aSide) const
1114 {
1115   switch(aSide) {
1116   case NS_SIDE_TOP:
1117     return BC_BORDER_BOTTOM_HALF(mTopBorder);
1118   case NS_SIDE_RIGHT:
1119     return BC_BORDER_LEFT_HALF(mRightBorder);
1120   case NS_SIDE_BOTTOM:
1121     return BC_BORDER_TOP_HALF(mBottomBorder);
1122   default:
1123     return BC_BORDER_RIGHT_HALF(mLeftBorder);
1124   }
1125 }

2.) the code should reset the border width once a new computation starts.
otherwise the old border width will impose a lower limit on the new border.

In this case the original border width was 10 so we limit it to 5px and since the cell owns only a half we have 2px on the left and 3px on the right of exessive border, the same happens for top and down border.

It really sucks to invest time on this instead of getting rid of all this in bug 540256

Comment 5

[Jesse Ruderman]

> It really sucks to invest time on this instead of getting rid of all this in
> bug 540256

Ok, let's just mark this as depending on bug 540256, unless you think this is urgent.  I'll disable the reftest that's failing randomly in bug 540360.

Comment 6

[Bernd]

Attachment: testcase to demonstrate half border problem

The test case produces pretty funny results in any major browser

Comment 7

[Bernd]

Attachment: patch

The patch applies on top of bug 484256

Comment 8

[Bernd]

> It really sucks to invest time on this instead of getting rid of all this in
> bug 540256

but getting some more restraints for the rewrite into the tree is fun ;-)

Comment 9

[Jesse Ruderman]

As part of your patch, please revert
http://hg.mozilla.org/mozilla-central/rev/4cabe72ad940

Comment 10

[Bernd]

the removal of /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ is an accident and will be fixed before checkin.

Comment 11

[fantasai]

   // Get the *inner half of the border only*, in pixels.
   BCPixelSize GetBorderWidth(PRUint8 aSide) const;
+  BCPixelSize GetFullBorderWidth(PRUint8 aSide) const;

Like the method above it, this needs a comment.

+  BCMapCellIterator resetIter(this, damageArea);
+  resetIter.ResetCellBorderWidths();
+
   BCMapCellIterator iter(this, damageArea);

Why isn't this just
   BCMapCellIterator iter(this, damageArea);
   iter.ResetCellBorderWidths();
?

+    if ((info.mColIndex == 0 && mAreaStart.x == 0) ||
+        (info.mColIndex > mAreaStart.x)) {
+      info.mCell->SetBorderWidth(info.mStartSide, 0);
+    }

Ok, so, here, if mAreaStart.x is zero, and our mColIndex matches mAreaStart.x, we execute. If our mColIndex is greater than mAreaStart.x, we execute. But if mAreaStart.x is *not* zero, and our mColIndex matches mAreaStart.x, we don't execute.

Seems a little weird, just wanted to check if that was intentional?

Comment 12

[fantasai]

Comment on attachment 428060 [details] [diff] [review]
patch

Still waiting for answers to questions in comment 11.