1177690 - Tables with vertical writing mode and border-collapse: collapse misrender the beveled ends of horizontal borders

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Simon Montagu :smontagu]

Attachment: Testcase

Without border-collapse: collapse, the testcase works as expected.

Comment 1

[Jonathan Kew [:jfkthame]]

This may be fixed by the pending patches in bug 1157569.

Comment 2

[Jonathan Kew [:jfkthame]]

The testcase here now renders correctly, thanks to bug 1157569. However, it turns out there is still incorrect rendering of collapsed borders in certain cases: in vertical writing modes, bevels are applied to the wrong ends of horizontal border segments. (This doesn't show up in the original testcase here because it has symmetrically-beveled corners.)

I'm morphing this bug to address this remaining issue with collapsed border rendering; additional testcase coming.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: trickier testcase, showing that beveled corners may render incorrectly

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Bevel the correct ends of horizontal collapsed-border segments in vertical-rl writing mode

Turns out we need a direction test here in BCBlockDirSeg::Paint, similar to the one in BCInlineDirSeg::Paint, because nsCSSRendering::DrawTableBorderSegment is currently physical. This fixes the vertical-rl example in the testcase. The code to fix the vertical-* examples with dir=rtl is disabled because although this fixes the borders in the testcase, it actually makes things worse because we don't currently order cells from bottom to top (as we should). Until we fix that (as part of full vertical-bidi support), there's no point trying to fix the last two examples in this testcase.

Comment 5

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f2ad7e8c681

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: Reftests for bevelled corners on collapsed table border

Simple test for bevelled corners of collapsed borders in various writing modes. The fuzz on these tests is because collapsed borders render slightly differently from 'normal' borders at the diagonal joins; looks like this is probably a difference in how we're handling pixel-snapping, or something like that. This discrepancy already exists in release Firefox (it's not a new issue resulting from the patches here), so we should file a separate bug to investigate further.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8628913 [details] [diff] [review]
Bevel the correct ends of horizontal collapsed-border segments in vertical-rl writing mode

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

::: layout/tables/nsTableFrame.cpp
@@ +6998,5 @@
> +         order the actual table cells correctly for vertical bidi-rtl, so the
> +         result is just a mess. Probably not worth fixing until we address
> +         bug 1131451 more fully. */
> +  // workaround lack of full vertical-bidi support (bug 1131451)
> +  if (aIter.mTableWM.IsVertical() && !aIter.mTableWM.IsBidiLTR()) {

Best to avoid dead code, if possible; it's prone to bitrot & becoming vestigial...  so I'd rather we just drop this "#if 0" chunk. (Disabled workarounds don't really to do us much good)

If you want to leave something here, I'd prefer just an XXX comment saying e.g. "For reversed vertical writing-modes (with direction:rtl), we need to invert physicalRect's y-position here, with respect to the table".

(But really, this should Just Work once we've got more robust functions for converting from logical to physical coordinates, taking the containerWidth *and* the containerHeight. So we shouldn't ultimately need a special case here.)

@@ +7002,5 @@
> +  if (aIter.mTableWM.IsVertical() && !aIter.mTableWM.IsBidiLTR()) {
> +    physicalRect.y = aIter.mTable->GetRect().height - physicalRect.YMost();
> +  }
> +#endif
> +  if (aIter.mTableWM.IsVerticalRL()) {

Could you add a comment to (1) explain that we're just swapping the sides in this special-case (with respect to the general case), and (2) hint at why?  (I'm not 100% clear, offhand, partly because DrawTableBorderSegment doesn't have much documentation. :))

Comment 8

[Daniel Holbert [:dholbert]]

(Also, half of this patch seems to be unrelated whitespace changes. Those should probably land on their own; otherwise, they make it harder to see the actual relevant changes here.  rs=me on just landing those as their own patch, e.g. as "part 0" for this bug, if you like)

Comment 9

[Daniel Holbert [:dholbert]]

..and if possible, it'd really be nice to end up with just a single call to nsCSSRendering::DrawTableBorderSegment() here (instead of 2 near-identical calls back-to-back), with appropriately-named local variables for the last 4 args. (which we stomp on if we're vertical-rl, with a comment explaining why)

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8629275 [details] [diff] [review]
Reftests for bevelled corners on collapsed table border

>+++ b/layout/reftests/writing-mode/tables/reftest.list
>@@ -77,8 +77,13 @@ fuzzy-if(winWidget,48,600) fuzzy-if(coco
>+
>+fuzzy-if(cocoaWidget,128,162) fuzzy-if(winWidget,48,100) fuzzy-if(gtk2Widget|Android|B2G|Mulet,40,80) == border-collapse-bevels-1.html border-collapse-bevels-1-ref.html
>+fuzzy-if(cocoaWidget,128,162) fuzzy-if(winWidget,48,100) fuzzy-if(gtk2Widget|Android|B2G|Mulet,40,80) == border-collapse-bevels-2.html border-collapse-bevels-1-ref.html
>+fuzzy-if(cocoaWidget,128,162) fuzzy-if(winWidget,48,100) fuzzy-if(gtk2Widget|Android|B2G|Mulet,40,80) == border-collapse-bevels-3.html border-collapse-bevels-1-ref.html
>+fuzzy-if(cocoaWidget,128,162) fuzzy-if(winWidget,48,100) fuzzy-if(gtk2Widget|Android|B2G|Mulet,40,80) == border-collapse-bevels-4.html border-collapse-bevels-1-ref.html

Few things:
 (1) Testcases should be named "-1a", "-1b", etc., instead of "-1", "-2", etc., since they all share "-1-ref" as their reference case.  (That way it's easier, when you're viewing a testcase in isolation, to guess what its testcase's name is and just directly jump to it by editing your URL bar.)

 (2) We're needing to use some pretty serious fuzzy-if() on every platform (tens-to-hundreds of pixels, off by tens-to-hundreds of color-channel-value).  That seems bad... I'm guessing this is because all of the testcases use "border-collapse:collapse", and that does slightly different antialiasing at border-edges, perhaps?  Could we get a better match here that by getting rid of the current (non-collapsed) reference case and just promoting border-collapse-bevels-1.html to be the reference case?  (Do the other testcases render the same as that file? Or is each one subtly different?)

 (3) Per your notes in the "#if 0" section -- it's probably worth including "direction:rtl" variants here as well. (I expect a testcase with direction:rtl and horizontal-tb should pass, whereas a test with a vertical writing-mode & direction:rtl should be marked "fails" with a comment pointing to bug 1177690.)

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: part 0 - Tidy up some erratic whitespace

You're right, this could be made a lot tidier. Let's try again...

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: part 1 - Bevel the correct ends of horizontal collapsed-border segments in vertical-rl writing mode

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - Tidy up the use of DrawTableBorderSegment similarly in BCInlineDirSeg::Paint

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: Reftests for bevelled corners on collapsed table border

There'll still be some fuzz here, because we get a pixel of overlap at the corners (this is a pre-existing issue, not addressed here), and then the different painting order of vertical-mode vs horizontal-mode results in a difference along the diagonal. But it should be a lot less than before. I'll push a try job to see what the various platforms report.

Comment 15

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aca4a57c6724

Comment 16

[Jonathan Kew [:jfkthame]]

Well, to my surprise it seems that we don't need fuzz, except what I already found I needed locally on OS X. Must be some minor platform graphics-backend differences in how the adjacent/overlapping colors blend along the diagonals.

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8630382 [details] [diff] [review]
part 1 - Bevel the correct ends of horizontal collapsed-border segments in vertical-rl writing mode

Thanks, this is much more elegant than the previous version! Nice use of Swap() too. :)

Though, per IRC:
>+  // In vertical-rl mode, the 'start' and 'end' of the block-dir (horizontal)
>+  // border segment need to be swapped because DrawTableBorderSegment will
>+  // apply the 'start' bevel at the left edge, and 'end' at the right.
>+  if (aIter.mTableWM.IsVerticalRL()) {
>+    Swap(startBevelSide, endBevelSide);
>+    Swap(startBevelOffset, endBevelOffset);
>+  }

This comment may really be indicative of a DrawTableBorderSegment bug. It doesn't make sense that this function always draws "start" on the left, because we're *telling* it which side to apply the start bevel on.

r- on this version for the moment, since it seems like we may really want to fix whatever's causing trouble inside of DrawTableBorderSegment.

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 8630386 [details] [diff] [review]
Reftests for bevelled corners on collapsed table border

r=me on the tests -- just one nit:

>Bug 1177690 - Reftests for bevelled corners on collapsed table border. r?dholbert

Maybe add "part 3" to commit message, to go along with "part 0", "part 1", "part 2" in previous patches' commit messages.

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8630382 [details] [diff] [review]
part 1 - Bevel the correct ends of horizontal collapsed-border segments in vertical-rl writing mode

>+  // In vertical-rl mode, the 'start' and 'end' of the block-dir (horizontal)
>+  // border segment need to be swapped because DrawTableBorderSegment will
>+  // apply the 'start' bevel at the left edge, and 'end' at the right.

OK, this is clearer to me now that you explained it in IRC. :)

To avoid "don't we already know the sides" confusion like mine in comment 17, please broaden this comment a bit, e.g. with a parenthetical like:

(Note: In this case, startBevelSide/endBevelSide will usually both be "top" or "bottom". DrawTableBorderSegment expects startBevelOffset to be the indentation-from-the-left for the start edge of the border-segment, and endBevelOffset is the indentation-from-the-right for the "end" edge of the border-segment. We've got them reversed, since we're RTL, so we have to swap them here.)

r+ with some sort of clarification like that.

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 8630383 [details] [diff] [review]
part 2 - Tidy up the use of DrawTableBorderSegment similarly in BCInlineDirSeg::Paint

>+  nscoord endBevelOffset = mIEndBevelOffset;
>+  // With RTL directionality, the 'start' and 'end' of the inline-dir
>+  // border segment need to be swapped because DrawTableBorderSegment will
>+  // apply the 'start' bevel physically at the left or top edge, and 'end' at
>+  // the right or bottom.
>+  if (!aIter.mTableWM.IsBidiLTR()) {
>+    Swap(startBevelSide, endBevelSide);
>+    Swap(startBevelOffset, endBevelOffset);
>+  }
>+  nsCSSRendering::DrawTableBorderSegment(aRenderingContext, style, color,

Shouldn't this check be the same in part 1 & part 2?  (part 1 just checks for "vertical-rl", whereas here we're checking for !IsBidiLTR)

I know this patch is just simplifying existing code -- but I'm curious why the thing we're checking here differs from the thing we're (newly) checking in part 1.

Comment 21

[Jonathan Kew [:jfkthame]]

(In reply to Daniel Holbert [:dholbert] from comment #20)
> Comment on attachment 8630383 [details] [diff] [review]
> part 2 - Tidy up the use of DrawTableBorderSegment similarly in
> BCInlineDirSeg::Paint
> 
> >+  nscoord endBevelOffset = mIEndBevelOffset;
> >+  // With RTL directionality, the 'start' and 'end' of the inline-dir
> >+  // border segment need to be swapped because DrawTableBorderSegment will
> >+  // apply the 'start' bevel physically at the left or top edge, and 'end' at
> >+  // the right or bottom.
> >+  if (!aIter.mTableWM.IsBidiLTR()) {
> >+    Swap(startBevelSide, endBevelSide);
> >+    Swap(startBevelOffset, endBevelOffset);
> >+  }
> >+  nsCSSRendering::DrawTableBorderSegment(aRenderingContext, style, color,
> 
> Shouldn't this check be the same in part 1 & part 2?  (part 1 just checks
> for "vertical-rl", whereas here we're checking for !IsBidiLTR)
> 
> I know this patch is just simplifying existing code -- but I'm curious why
> the thing we're checking here differs from the thing we're (newly) checking
> in part 1.

This is dealing with segments in the inline direction: horizontal borders for horizontal writing mode, and vertical borders for vertical writing mode. In both cases, it's when dir=rtl that the "start" and "end" of the computed border segment will be reversed from the physical coordinates used in drawing.

Part 1 was different because it was dealing with the block direction: in horizontal writing mode, the block direction is always downwards (we don't have a horizontal-bt mode), so reversal is never needed; it's only in vertical-rl mode that the block direction (leftwards) is reversed from physical coords.

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 8630383 [details] [diff] [review]
part 2 - Tidy up the use of DrawTableBorderSegment similarly in BCInlineDirSeg::Paint

Ah, ok - thanks.  r=me then. (though whatever comment-tweaks you make in part 1 probably want to be synced to the comment in part 2)

Comment 23

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c4f2d9775cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/efe1ad2424c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/11001b64e4cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/05238a141bed

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5c4f2d9775cd
https://hg.mozilla.org/mozilla-central/rev/efe1ad2424c2
https://hg.mozilla.org/mozilla-central/rev/11001b64e4cf
https://hg.mozilla.org/mozilla-central/rev/05238a141bed