Open Bug 1020208 Opened 6 years ago Updated 5 years ago

Collapsed table border glitch with spanned cells

Categories

(Core :: Layout: Tables, defect)

x86
Windows XP
defect
Not set

Tracking

()

REOPENED

People

(Reporter: andershol, Unassigned)

References

Details

(Keywords: testcase, Whiteboard: [dupeme])

Attachments

(5 files, 6 obsolete files)

Attached file Test-case
The test-case have a table with 5 cells in 4 rows (cell 1 and 2 starts on the same row, cell 2, 3 and 4 spans two rows):
+-----+-----+
|  1  |  2  |
+-----+     |
|  3  +-----+
+-----+  4  |
|  5  |     |
+-----+-----+
Cell 2 and 3 have a border and the table have border-collapse enabled. The background gradient in the test-case is only to illustrate the location of the cells.

The problem is that the horizontal border between cell 3 and 5 (the bottom border on cell 3) continues to the right through cell 4. This only happens if the bottom border on cell 2 and 3 have the same color.
I can reproduce the issue as described above on Win 7 x64 with:
- Firefox 30 Release Candidate - Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 - BuildID: 20140603140158
- Latest Firefox 31 Aurora - Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 - BuildID: 20140604004003
- Latest Firefox 32 Nightly - Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:32.0) Gecko/20100101 Firefox/32.0 - BuildID: 20140602030202
Status: UNCONFIRMED → NEW
Ever confirmed: true
I tried to run mozregression on it. The earliest nightly builds on the ftp-server are currently from 2004-02-10. I did not find any good builds.
Component: General → Layout: Tables
Keywords: testcase
Product: Firefox → Core
Whiteboard: [dupeme]
Version: 31 Branch → Trunk
The issue is not a regression since it shows even in Firefox 4.0.1. Also, the issue does not show on Chrome or IE.
At least it haven't been proven (nor claimed) to be a regression. The nightly from 2004-02-10, which also has the issue, is branded as Firebird 0.7.
The bottom horizontal borders of two adjacent cells are joined and drawn as one, if the borders have the same color/width/style. The ensures e.g. that dashed borders look continuous, I guess. But it is not checked whether the cells have rowspan such that their bottom borders do not actually align.

Not sure what test suites to run to check that I didn't break anything (layout/table/reftests/ only have one test).

Also fixes some typos/copy-pastes in comments.
Attachment #8550702 - Flags: review?(bernd.mielke)
Note that even without the patch some borders are not merged if the happen to align *because* of rowspan. For example in this test-case note the "stuttering" in the dashing (sign of un-merged border, I believe) in the bottom border that should appear as one because the rowspan in the second column make the bottom borders align, whereas the top border is draw as one continuous border.
This indicate that CalcBCBorders have a more fundamental problem with rowspan.
I am haven't coded this area for quite some time (2 years) so I am not sure that my review counts.
(In reply to Bernd from comment #8)
> I am haven't coded this area for quite some time (2 years) so I am not sure
> that my review counts.

Okay, you where just one Bugzilla suggested and one with few pending requests. Thanks for responding. :)
... well, I had also checked that you at least had worked in the function at some point.
Attachment #8550702 - Flags: review?(bernd.mielke) → review?(roc)
Robert, I can review. The problem that this patch if fixing is described in  http://lxr.mozilla.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#4051. Basically the patch is saying: do not compare apples to pears. The starting of a new BC segment is uncritical. I would however prefer a separate if case for this check with a decent comment that both borders can not join as they are in different positions. As usual with bad code it needs comments.
Anders, can you please check if your patch would fix bug 322810, bug 332740, bug 332977, bug 1000435, bug 935914, bug 195299.
If so take the test cases from there an incorporate them in the patch.
The border collapse reftests are in http://lxr.mozilla.org/mozilla-central/source/layout/reftests/table-bordercollapse/
Attachment #8550702 - Flags: review?(roc)
Comment on attachment 8550702 [details] [diff] [review]
Ensure adjacent cells bottom border are aligned before merging

Review of attachment 8550702 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me, and it sounds like Bernd likes it. (Hi Bernd! Nothing much has changed in this code so you'd be welcome to r+ here :-).)

A fix for the case where borders align due to rowspans (comment #7) would be nice but not necessary in this bug.

Thanks!
Attachment #8550702 - Flags: review+
The patch looks fine, but comment 6 indicates that the test has to undergo some additional testing although the try server should catch this.

But this is more a grumpy Statler & Waldorf comment.
(In reply to Bernd from comment #11)
> I would however prefer a separate if case for this check with a decent
> comment that both borders can not join as they are in different positions.
> As usual with bad code it needs comments.

Is this something like what you meant?
I also moved the test-case to the layout/reftests/table-bordercollapse/ folder with the others (the last one from layout/table/reftests/ should probably also be moved or all the others should)
Attachment #8550702 - Attachment is obsolete: true
Attachment #8558102 - Flags: review?(bernd.mielke)
n(In reply to Bernd from comment #12)
> Anders, can you please check if your patch would fix bug 322810, bug 332740,
> bug 332977, bug 1000435, bug 935914, bug 195299.

Thanks for the pointers to bugs. As far as I can see there where these test-cases in the bugs (names: bug_id-comment_idx-attachment_id):
- bug 322810
  - 322810-c6-212411.html    Not fixed
  - 322810-c16.html          Fixed somewhat, but corner is missing
- bug 332740
  - 332740-c1-217197.html    Not fixed
  - 332740-c5-252154.html    Not fixed
  - 332740-c8-288066.html    Not fixed
  - 332740-c10-309485.html   Fixed!
  - 332740-c11-348003.html   Not fixed
  - 332740-c27-787103.html   Fixed!
  - 332740-c30-790496.html   Fixed!
- bug 332977
  - 332977-c3-343738.html    Not fixed
  - 332977-c4-343739.html    Not fixed
  - 332977-c5-343740.html    No problem
- bug 1000435
  - 1000435-c0-8411252.xhtml Not fixed (vertical borders)
- bug 935914
  - 935914-c4-828886.html    Fixed
- bug 195299
  - 195299-c4-123341.html    Could not reproduce

Do you want the fixed ones included (minimized and with referees)? Since the fix is so local I suspect they just test the same code path.

I tried to have a look at CalcBCBorders and its helpers. It seems quite clear that my patch is not the real solution, but fixes the symptom (). The code seems quite "unrolled" to me in the sense that there are four similar code blocks (one to handle each direction) and most of them have two similar cases for handling e.g. the first row and the other rows respectively. So there are a lot of similar code that because of non-straightforward rules for what cells updates which borders aren't identical.

It seems to me that the code instead of iterating over the cells and updating certain borders trying to update all exactly once, iterated over the num-cols*(num-rows+1) horizontal border coordinates and then the (num-cols+1)*num-rows vertical border coordinates, in each case looking up the two adjoining cells. It doesn't seem it would be slower, as the BCMapCellIterator already go by coordinates internally. But I can't quite get my head around the corner handling (why is it important? what signifies an corner owner or dominant border? why are beveled edges sometimes special?). The corner info only seems to be used for offset calculations in CalcVerCornerOffset/CalcHorCornerOffset/CalcHorCornerOffset. I might give it a few more tries. But then again maybe bug 203686 is a even better fix.
(In reply to Bernd from comment #15)
> The patch looks fine, but comment 6 indicates that the test has to undergo
> some additional testing although the try server should catch this.

I ran the layout/reftests/table-bordercollapse/ and it didn't catch anything. The test cases from the bugs you linked to seem to have either been fixed or be unchanged (with the annoying exception of 322810-c16.html which at least looks better).
(In reply to andershol from comment #17)
> corner handling (why is it important? what signifies an corner owner
> or dominant border? why are beveled edges sometimes special?)

I realized as soon as I had written the above. Sorry for the spam.
please give me a few days for review, at least over the weekend I will look at it.
Comment on attachment 8558102 [details] [diff] [review]
Ensure adjacent cells bottom border are aligned before merging (v2)

Review of attachment 8558102 [details] [diff] [review]:
-----------------------------------------------------------------

looks good to me, Robert can you push this over the try server as I don't have a working environment
Attachment #8558102 - Flags: review?(bernd.mielke) → review+
(In reply to Bernd from comment #21)
> Comment on attachment 8558102 [details] [diff] [review]
> Ensure adjacent cells bottom border are aligned before merging (v2)
> 
> Review of attachment 8558102 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> looks good to me, Robert can you push this over the try server as I don't
> have a working environment
Flags: needinfo?(roc)
Thanks for the review, but I'm not sure how to interpret the output from treeherder. Sorry it took so long to respond, I took a stab at the approach mentioned in comment 17 and got distracted.

Could I perhaps bother you again and have you take a look at this too? It fixes all the cases mentioned in comment 17 (although I didn't create references for 174470-c24-155229 and 1000435-c0-8411252). To check for bugs that would influence both the test- and ref-documents (easy to accidentally add such), I created static png's of the ref-documents in layout/table-bordercollapse with the code without the patch and compared those against the test-documents generated with the patch. The patch is somewhat bigger (1687 insertions, 1640 deletions), but 1064 insertions are the new reftests, so there are a bit less code and, since it has fewer special cases, easier to follow I think (but I'm of course biased).

The old coded only executed the contents of the main loop once per cell, although the iterator still walked over every cell coordinate and the main loop did have loops of its own (to travel along the edges of the cell). The new code executes the main loop for every cell coordinate. I'm not sure if that will be a performance problem (e.g. for <td colspan=1000 rowspan=1000>). There where some innocent-looking functions (like GetRowCount, GetStartRowIndex) that are sometimes non-constant-time, that I tried to avoid.

I didn't quite get my head around the code to "Find border sizes and set them on table/colgroup/col/rowgroup/row/cell". It seems to me to have to many special cases.
Attachment #8571178 - Flags: review?(bernd.mielke)
This tries to simplify the size calculation mentioned last in comment 24. It also passes the table-bordercollapse tests. It did seem that the old code explicitly did not do it this simply, but I can't see what it tried to guard against.

It also changes some "setValue(max(newValue, getValue()))"-constructs to "updateValue(newValue)".
Doesn't seem to be a problem anymore.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WORKSFORME
Sorry wrong bug.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Attachment #8571178 - Flags: review?(bernd.mielke) → review?(bernd_mozilla)
Sorry for being so late, but real life did take its toll.  And this changes the code significantly so I need some to read.
Comment on attachment 8571178 [details] [diff] [review]
Fix border drawing for tables with collapsed borders

Roc, you briefly looked at this bug earlier and, if I read the annotated file correctly, one of very few people that have touched the affected code (other than crosscutting mass-changes). Would you mind having a look at these patches or do you have an idea of who might be able to? or is this just to high-risk/low-benefit (as e.g. the bugs have been open so long, so they can't be critical, and servo is the way forward)? Also, as I am new around here, the patches might just be fubar.
Attachment #8571178 - Flags: review?(bernd_mozilla) → review?(roc)
Comment on attachment 8571178 [details] [diff] [review]
Fix border drawing for tables with collapsed borders

Review of attachment 8571178 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/reftests/table-bordercollapse/reftest.list
@@ +103,5 @@
> +== 332740-c8-288066.html 332740-c8-288066-ref.html
> +== 332740-c10-309485.html 332740-c10-309485-ref.html
> +== 332740-c11-348003.html 332740-c11-348003-ref.html
> +== 332740-c27-787103.html 332740-c27-787103-ref.html
> +== 332740-c30-790496.html 332740-c30-790496-ref.html

Bug 322740 doesn't seem related to BC-borders

@@ +105,5 @@
> +== 332740-c11-348003.html 332740-c11-348003-ref.html
> +== 332740-c27-787103.html 332740-c27-787103-ref.html
> +== 332740-c30-790496.html 332740-c30-790496-ref.html
> +== 332977-c3-343738.html 332977-c3-343738-ref.html
> +== 332977-c4-343739.html 332977-c4-343739-ref.html

Nor does bug 332977

@@ +108,5 @@
> +== 332977-c3-343738.html 332977-c3-343738-ref.html
> +== 332977-c4-343739.html 332977-c4-343739-ref.html
> +== 935914-c4-828886.html 935914-c4-828886-ref.html
> +== 1020208-c0-8434027.html 1020208-c0-8434027-ref.html
> +== 1020208-c7-8550703.html 1020208-c7-8550703-ref.html

Instead of including attachment numbers, I'd just do what we normally and number tests 1020208-1.html, 1020208-2.html, etc. You can include the attachment number in comments in the tests if you want.
Comment on attachment 8571178 [details] [diff] [review]
Fix border drawing for tables with collapsed borders

Review of attachment 8571178 [details] [diff] [review]:
-----------------------------------------------------------------

I don't really know the BC border drawing code at all.

I'm going to give this to Xidorn, who also doesn't know it, but is doing some work in that area and hopefully will be able to learn more about it :-).
Attachment #8571178 - Flags: review?(roc) → review?(quanxunzhen)
Sorry, but it would probably take an extensive time to learn that. I'll try to finish that in this quarter.

A quick scan on the current patch seems to me that it has a wide range of conflict because of bug 1157569. I bet there would be more conflict later with other changes to table for supporting vertical writing mode.

I feel sorry that you will need to update your patch later. Thanks for your work.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (mostly offline May 30 - June 7) from comment #30)
> Comment on attachment 8571178 [details] [diff] [review]
> Fix border drawing for tables with collapsed borders
> 
> Review of attachment 8571178 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/reftests/table-bordercollapse/reftest.list
> @@ +103,5 @@
> > +== 332740-c8-288066.html 332740-c8-288066-ref.html
> > +== 332740-c10-309485.html 332740-c10-309485-ref.html
> > +== 332740-c11-348003.html 332740-c11-348003-ref.html
> > +== 332740-c27-787103.html 332740-c27-787103-ref.html
> > +== 332740-c30-790496.html 332740-c30-790496-ref.html
> 
> Bug 322740 doesn't seem related to BC-borders

I think you typo'ed the bug-number. It is  "Bug 332740 - [BC] rowspan and border-collapse: collapse breaks border rendering".

> Nor does bug 332977

Why not? The problem is BC in combination with rowspan. It is so related, that it is probably redundant.

> Instead of including attachment numbers, I'd just do what we normally and
> number tests 1020208-1.html, 1020208-2.html, etc. You can include the
> attachment number in comments in the tests if you want.

Okay, I'll remove the attachment numbers and make the subscripts consecutive. It just seemed like a good idea at the time.
Not sure if the bug was waiting on me. But I just tried to fix the bit-rot. There had been a lot of churn in the files (that didn't seem to ease understanding). Might be useful to diff the two versions of the patch to check the renaming.
Carry the review-request.
Attachment #8571178 - Attachment is obsolete: true
Attachment #8571178 - Flags: review?(quanxunzhen)
Attachment #8616547 - Flags: review?(quanxunzhen)
Fix bit-rot.
Attachment #8571186 - Attachment is obsolete: true
Attachment #8616549 - Flags: review?(quanxunzhen)
(In reply to andershol from comment #34)
> Not sure if the bug was waiting on me.

No, not at all.

I'm still not familiar enough with the code to review your patch, sorry :/
Comment on attachment 8616547 [details] [diff] [review]
Fix border drawing for tables with collapsed borders v2

Review of attachment 8616547 [details] [diff] [review]:
-----------------------------------------------------------------

I'm still not familiar enough with the code, sorry. But some suggestions first.

::: layout/tables/nsCellMap.cpp
@@ +1076,5 @@
>    }
>  
> +  NS_ASSERTION(xPos >= 0, "Invalid xPos");
> +  NS_ASSERTION(xPos <= GetColCount(), "Invalid xPos");
> +  NS_ASSERTION(yPos >= 0, "Invalid yPos");

Please use MOZ_ASSERT instead of NS_ASSERTION here. MOZ_ASSERT would cause crash when the assertions are broken on debug build (and is ignored on release build), hence more loudly.

::: layout/tables/nsTableFrame.cpp
@@ +4798,5 @@
>      return; // nothing to do
>  
> +  // table ltr information, the border collapse code swaps the sides
> +  // to account for rtl tables, this is done through startSide and endSide
> +  WritingMode writingMode = this->GetWritingMode();

The variable `writingMode` could be renamed to `wm`, it has been a convention for layout code. Also, why do we need "this->" for calling method on the object itself?

@@ +4799,5 @@
>  
> +  // table ltr information, the border collapse code swaps the sides
> +  // to account for rtl tables, this is done through startSide and endSide
> +  WritingMode writingMode = this->GetWritingMode();
> +  bool tableIsLTR = StyleVisibility()->mDirection == NS_STYLE_DIRECTION_LTR;

`tableIsLTR` can be replaced with `wm.IsBidiLTR()`.

@@ +4801,5 @@
> +  // to account for rtl tables, this is done through startSide and endSide
> +  WritingMode writingMode = this->GetWritingMode();
> +  bool tableIsLTR = StyleVisibility()->mDirection == NS_STYLE_DIRECTION_LTR;
> +  mozilla::LogicalSide startSide = tableIsLTR ? eLogicalSideIStart : eLogicalSideIEnd;
> +  mozilla::LogicalSide endSide = tableIsLTR ? eLogicalSideIEnd : eLogicalSideIStart;

Actually, I'd suggest you ignore the {start,end}Side, and just use eLogicalSideI{Start,End} everywhere. I don't see there is any reftest which is testing border-collapse for RTL tables, so breaking a bit on that should be fine. The existing code doesn't seem to handle RTL table correctly anyway.

@@ +4814,5 @@
> +  // segments on the table border edges need to be initialized only once
> +  if (damageArea.StartRow() == 0) propData->mTopBorderWidth = 0;
> +  if (damageArea.StartCol() == 0) propData->mLeftBorderWidth = 0;
> +  if (damageArea.StartCol() + damageArea.ColCount() == numCols) propData->mRightBorderWidth = 0;
> +  if (damageArea.StartRow() + damageArea.RowCount() == numRows) propData->mBottomBorderWidth = 0;

The current coding style guide [1] says we should always add curly brackets around code blocks of the flow control statements, and always put them in a new line.

Also, we should keep the line length be equal or less than 80 characters. [2] (This rule seems to be broken in various places in your patches.)

I understand that our old code have some different coding styles, but let's fix them gradually (in new code).

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Line_Length

@@ +4819,5 @@
> +
> +  int32_t damageX = std::max(0, damageArea.StartCol() - 1);
> +  int32_t damageY = std::max(0, damageArea.StartRow() - 1);
> +  int32_t damageXMax = damageArea.StartCol() + damageArea.ColCount() - 1;
> +  int32_t damageYMax = damageArea.StartRow() + damageArea.RowCount() - 1;

As you are rewriting almost the whole function, could you help follow the new logical terms (for table, it should be col and row) instead of physical ones (x and y) in the new code? Here I'd suggest the names be `damage{Start,End}{Col,Row}`.

It seems to me this code (along with `colX <= damageXMax + 1` below) is meant to expand the damage area. As far as changing `damage*Max` to `damageEnd*`, I suggest to use "+ 1" instead "- 1" here, as "end" in C++ usually means the position just after the last one, i.e. start + count = end. (I know that in the current table code, it is a bit confused, but I hope we could follow that in new code).

Also, we probably should check whether the end values exceed the boundary of the table. (std::min them with Get{Col,Row}Count())

@@ +4821,5 @@
> +  int32_t damageY = std::max(0, damageArea.StartRow() - 1);
> +  int32_t damageXMax = damageArea.StartCol() + damageArea.ColCount() - 1;
> +  int32_t damageYMax = damageArea.StartRow() + damageArea.RowCount() - 1;
> +  BCCellBorder emptyBorder;
> +  BCCellBorder* topLeftBorders = new BCCellBorder[damageXMax - damageX + 2];

With the change I proposed above, the "+ 2" here would no longer be needed,

@@ +4828,5 @@
> +  OrderRowGroups(rowGroups);
> +  BCRowInfo rowInfoTop;
> +  BCRowInfo rowInfo;
> +
> +  for (int32_t rowY = damageY; rowY <= damageYMax + 1; rowY++) {

and the condition here would be `row < damageEndRow`.

@@ +4905,5 @@
> +      corner.Update(NS_SIDE_BOTTOM, leftBorder);
> +      corner.Update(NS_SIDE_LEFT, leftTopBorder);
> +      tableCellMap->SetBCBorderCorner(eTopLeft,
> +        rowInfo.mCellMap, rowInfo.mRowGroupStart, rowY, colX,
> +        mozilla::css::Side(corner.ownerSide), corner.subWidth, corner.bevel);

This doesn't really match the coding style as well. You probably want to move `eTopLeft` to the next line, or you should align all lines after the bracket.

@@ +4912,5 @@
> +      BCPixelSize widthTop = (rowY == 0 ?
> +        CompareBordersWidth(this,
> +          info.mColGroup, info.mCol,
> +          rowInfo.mRowGroup, rowInfo.mRow,
> +          info.mCell, writingMode, eLogicalSideBStart, !ADJACENT) : 0);

I don't think we need the outer brackets, could you remove them?

@@ +4928,5 @@
> +        CompareBordersWidth(this,
> +          info.mColGroup, info.mCol,
> +          rowInfo.mRowGroup, rowInfo.mRow,
> +          info.mCell, writingMode, eLogicalSideIEnd, ADJACENT) : 0);
> + 

A tailing space, please remove.

@@ +4945,5 @@
> +            info.mCell, writingMode, eLogicalSideIStart, !ADJACENT),
> +          !HORIZONTAL).width;
> +      }
> +      BCPixelSize widthTopCollapsed = 0;
> +      

Several tailing spaces.
(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #37)
> Comment on attachment 8616547 [details] [diff] [review]
> Fix border drawing for tables with collapsed borders v2
> 
> Review of attachment 8616547 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm still not familiar enough with the code, sorry. But some suggestions
> first.
Thanks for having a look.

> ::: layout/tables/nsTableFrame.cpp
> @@ +4801,5 @@
> > +  // to account for rtl tables, this is done through startSide and endSide
> > +  WritingMode writingMode = this->GetWritingMode();
> > +  bool tableIsLTR = StyleVisibility()->mDirection == NS_STYLE_DIRECTION_LTR;
> > +  mozilla::LogicalSide startSide = tableIsLTR ? eLogicalSideIStart : eLogicalSideIEnd;
> > +  mozilla::LogicalSide endSide = tableIsLTR ? eLogicalSideIEnd : eLogicalSideIStart;
> 
> Actually, I'd suggest you ignore the {start,end}Side, and just use
> eLogicalSideI{Start,End} everywhere. I don't see there is any reftest which
> is testing border-collapse for RTL tables, so breaking a bit on that should
> be fine. The existing code doesn't seem to handle RTL table correctly anyway.

I had used the test case from 174470 comment 24 to check that RTL wasn't to badly broken. I tried adding "td{border-width:40px !important;} to it and it turned out that part1 had some regression that part2 fixed, so I folded the two patches. Hope that doesn't disturb to much.
Independent of my patches there also seem to be some regressions that seem to be caused by some rounding error (text is shifted 1 pixel the the side compared with current Aurora).

The "XXX"-commentat the top of the function, that I left in there, should probably go.

> Also, we should keep the line length be equal or less than 80 characters.
> [2] (This rule seems to be broken in various places in your patches.)

I had tried to squeeze it down to 80 chars, but left a few lines overflow as to not hurt readability to much. Now I have squeezed it some more.

> @@ +4819,5 @@
> > +
> > +  int32_t damageX = std::max(0, damageArea.StartCol() - 1);
> > +  int32_t damageY = std::max(0, damageArea.StartRow() - 1);
> > +  int32_t damageXMax = damageArea.StartCol() + damageArea.ColCount() - 1;
> > +  int32_t damageYMax = damageArea.StartRow() + damageArea.RowCount() - 1;
> 
> Here I'd suggest the names be `damage{Start,End}{Col,Row}`.

I changed colX/rowY to col/row as well. But I don't think, I agree that col/row is much more logical than x/y.

> Also, we probably should check whether the end values exceed the boundary of
> the table. (std::min them with Get{Col,Row}Count())

The std:max(0, ...) in the previous line is not there to guard against invalid damageArea values. It is an optimization. Because, even though we expand the damage area (not the best name) to the previous col/row such that leftTopBorder/topLeftBorder will be initialized when we enter the damage area, we don't need to expand it outside the edge of the table (we know there is no borders out there, so we just initilize to "no border"). In the other direction, however, we also expand the damage area, but this is because we only look at borders on the top and at the left, but still need to handle the borders at the right and bottom edge of the damage area, and in the case the damange area touches the right or bottom edge of the table, we actually need to go outside the table. So it would be wrong to clamp it to Get{Col,Row}Count(). This is hopefully already explained in the block comment at the top of the function.
If the damageArea should be checked or clamped, it should probably be done right after the variable is created before it is used for the first time.

> @@ +4912,5 @@
> > +      BCPixelSize widthTop = (rowY == 0 ?
> I don't think we need the outer brackets, could you remove them?

They weren't needed. I just thought they helped readability.
Attachment #8616547 - Attachment is obsolete: true
Attachment #8616549 - Attachment is obsolete: true
Attachment #8616547 - Flags: review?(quanxunzhen)
Attachment #8616549 - Flags: review?(quanxunzhen)
Attachment #8623490 - Flags: review?(quanxunzhen)
There are several coding styles in table code which do not match our current style guides. Hopefully you can fix them in the new code:
* "}" and the following "else {" or "else if {" should be in the same line
* body of if-statement should be in its own line, and should always be wrapped with "{" "}" even if it is very short.
Comment on attachment 8623490 [details] [diff] [review]
Fix border drawing for tables with collapsed borders v3

Review of attachment 8623490 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/tables/nsTableFrame.cpp
@@ +4858,5 @@
> +  int32_t damageStartRow = std::max(0, damageArea.StartRow() - 1);
> +  int32_t damageEndCol = damageArea.StartCol() + damageArea.ColCount() + 1;
> +  int32_t damageEndRow = damageArea.StartRow() + damageArea.RowCount() + 1;
> +  BCCellBorder emptyBorder;
> +  auto topLeftBorders = new BCCellBorder[damageEndCol - damageStartCol];

This is a memory leak. The allocated array is never released.

For local dynamic array, we usually use nsTArray (or nsAutoTArray if a usual upper bound is predictable). So here, it should be:
> nsTArray<BCCellBorder> topLeftBorders;
> topLeftBorders.SetLength(damageEndCol - damageStartCol);

Please note that, even if you sometimes really need to use "new" to allocate memory, always try to use smart pointers, e.g. UniquePtr or nsRefPtr, to hold them, instead of raw pointer.

@@ +4882,5 @@
> +      info.SeekCol(tableCellMap, rowInfo, col);
> +
> +      // Calculate the cells top border, i.e. shared with the cell above
> +      const bool atTabHEdge = col >= 0 && col < numCols &&
> +        (row == 0 || row == numRows);

This variable name is not immediately clear to me. I'd suggest you change its name, or at least leave a comment there saying something like: This variable indicates whether the top border of the current cell is collapsed with the top or bottom border of the table.

@@ +4907,5 @@
> +      nextLeftTopBorder = topBorder;
> +
> +      // Calculate the cells left border, i.e. shared with the cell to the left
> +      const bool atTabVEdge = (col == 0 || col == numCols) &&
> +        row >= 0 && row < numRows;

Same here as "atTabHEdge" above.
(In reply to andershol from comment #38)
> Created attachment 8623490 [details] [diff] [review]
> Fix border drawing for tables with collapsed borders v3
> 
> > Here I'd suggest the names be `damage{Start,End}{Col,Row}`.
> 
> I changed colX/rowY to col/row as well. But I don't think, I agree that
> col/row is much more logical than x/y.

The "logical" here is a directional term, which corresponds to "flow-relative" in CSS terms. [1]

So x and y, and width and height, and horizontal and vertical, are physical terms. They refers to how an element is sized and positioned physically.

Originally, there is only one coordinate system, which is physical. However, it is no longer true with the introduction of CSS writing modes. (Actually it had been not true when we have RTL)

In short, for example, if we specify "writing-mode: vertical-rl;" to the table, rows would be vertical, and columns would be horizontal. And x and y, as its original meaning, the distance to the left edge and top edge, is no longer match row and column.

[1] http://dev.w3.org/csswg/css-writing-modes-3/#logical-directions
Thanks for the review.
I tried fixing the control structure formatting, but limited to the functions touched by the patch as to introduce to much noise.
Quite a blatant memory leak I had made.
Had considered other names for atTabHEdge/atTabHEdge, but this was the least bad. So I have added a comment.
Carrying the open review request.
Attachment #8623490 - Attachment is obsolete: true
Attachment #8623490 - Flags: review?(quanxunzhen)
Attachment #8625148 - Flags: review?(quanxunzhen)
Comment on attachment 8625148 [details] [diff] [review]
8623490: Fix border drawing for tables with collapsed borders v4

Review of attachment 8625148 [details] [diff] [review]:
-----------------------------------------------------------------

Currently, it is in progress of converting table layout to support writing mode. jfkthame has started working on the conversion of code handling border-collapse, which will have wide conflict with this patch.

Previously, I planned to finish reviewing this patch before that happens, but I failed to do so. Sorry about that.

Now, it would probably be better to wait until that finishes, and rewrite this patch on top of that. That work will hopefully finish in the next week.

Since jfkthame is working on that, redirect the review request to him.

Sorry again about not reviewing this patch in time, and thanks jfkthame to take this review.
Attachment #8625148 - Flags: review?(quanxunzhen) → review?(jfkthame)
Depends on: 1157569
Comment on attachment 8625148 [details] [diff] [review]
8623490: Fix border drawing for tables with collapsed borders v4

Review of attachment 8625148 [details] [diff] [review]:
-----------------------------------------------------------------

I'm clearing the review? flag here because as Xidorn noted, there has been a lot of change in the table code during the past couple of weeks in order to support writing-mode. This means that the patch here will need a substantial update before it will be usable -- sorry about that.

The writing-mode conversion in table code has not (intentionally, at least) changed the behavior in other respects, so the bug here remains in the current code and a fix would be very welcome.

Most of the writing-mode code churn in the table code is complete at this point, I think, except for nsTableOuterFrame which wasn't touched here anyway. There will no doubt be some bug-fixes still to come as we discover issues with the conversion, but it should be largely stable. So if you're able to update this patch to work with current mozilla-central code, I hope we'd be able to move forward with it. Thanks! And apologies once again for the extra work, due to the unfortunate timing here.
Attachment #8625148 - Flags: review?(jfkthame)
One other comment: for review purposes, it'd be helpful to split the patch into distinct stages where possible: e.g. something like replacing get/compare/set... sequences with update... (as mentioned in comment 25) where you're just simplifying the code without changing behavior, this could be done in a separate patch.
(In reply to Jonathan Kew (:jfkthame) from comment #45)
> One other comment: for review purposes, it'd be helpful to split the patch
> into distinct stages where possible: e.g. something like replacing
> get/compare/set... sequences with update... (as mentioned in comment 25)
> where you're just simplifying the code without changing behavior, this could
> be done in a separate patch.

In theory you would be correct (why I initially split it up that way). But in practice the original code is long, convoluted and wrong, so not really worth chasing. Even split up it was hard and not very enlightening to follow the path through the stages.
Also, this bug and its duplicates are so low priority that it is probably not worth chasing all the renamings.
You need to log in before you can comment on or make changes to this bug.