1393907 - Add handling for border bevels

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P1)

Description

[Kartikaya Gupta (email:[e-mail])]

There's a couple of TODOs in the code for handling border bevels. We should implement that.

[1] http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/layout/tables/nsTableFrame.cpp#7441
[2] http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/layout/tables/nsTableFrame.cpp#7699

Comment 1

[Kartikaya Gupta (email:[e-mail])]

I looked into this a little bit and since understanding the code is nontrivial here's an explanation that hopefully makes it easier.

Normally borders are rectangles that have four sides. However in the case of tables, borders get collapsed with other borders and need to be drawn one "side" at a time. Additionally, because of vertical/horizontal writing mode genericness, a lot of the table code deals with "block" and "inline" directions, which are logical abstractions over the physical "horizontal" and "vertical" directions. So when BCBlockDirSeg::CreateWebRenderCommands is called, it's supposed to draw one border segment in the "block" direction, and when BCInlineDirSeg::CreateWebRenderCommands is called, it's supposed to draw one border segment in the "inline" direction.

I used the layout/reftests/writing-mode/tables/vertical-border-collapse-1.html and vertical-border-collapse-1-ref.html files as examples here. In these pages, the outermost table border (the thick light/dark gray one) has two horizontal segments (top and bottom sides) which are considered the "block" direction and two vertical segments (left and right sides) which are considered the "inline" direction.

The way we are currently drawing these segments is just creating a "full" border (4 sides) but setting three of the sides to border-style:none so that they don't get drawn. We set the color and thickness on the remaining side (we use the left side for "block" and top side for "inline") and create a display item from that. The intent is that WR will draw just the one side of the border, which will correspond to the segment that we want to be drawing.

This works, except for border-beveling. That's when two colors meet at a corner, and the corner is supposed to be drawn with a 45-degree angle split between the two colors. (See the above test pages in Gecko for the visuals). WR does beveling internally, if you give it a border to draw with different colors on adjacent edges. However with what we're doing here (just asking it to draw one edge) it doesn't do beveling, so we end up with un-beveled borders, which is wrong. That's what the TODO is for.

The beveling information is provided in the param->*Bevel* fields. For example in the test case above, the top border needs beveling on the "bottom" side of the border segment (the bottom-left and bottom-right corners need to be shifted inward horizontally) and so that is represented by param->mStartBevelSide = eSideBottom, param->mStartBevelOffset = 480 (8 CSS px), param->mEndBevelSide = eSideBottom, param->mEndBevelOffset = 480. This indicates the bottom "start" (left in this case) side needs to be shifted in by 8 CSS pixels, and the bottom "end" side likewise.

The existing Gecko code to do this is at [1]. The way it works is turns that border segment rectangle into a Point[4] and then fiddles with the corner coordinates to do the bevelling. In order to do this in WR we have three options:
(1) Don't send a border display item to WR, but instead send a quad that already had the bevel adjustment applied
(2) Send the border display item with the bevel information to WR, and have WR apply the bevel adjustment before drawing
(3) Do some crazy hacking where instead of setting the other three border sides to border-style:none, set them to be transparent with appropriate widths, which presumably will make WR do the correct beveling. I'm not sure if this will even work, and it might not be able to handle all the different beveling scenarios that Gecko wants to handle.

[1] http://searchfox.org/mozilla-central/rev/cd82cacec2cf734768827ff85ba2dba90a534c5e/layout/painting/nsCSSRendering.cpp#3398-3432

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

This is still not working correctly, right?

Comment 3

[Kartikaya Gupta (email:[e-mail])]

To be knowledge, yeah, still not working correctly.

Comment 4

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Looking.

Comment 5

[Peter Chang[:pchang]]

This should block wr-nightly.

Comment 6

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Handle bevel borders.

Since bevel only appear outmost border of table. I collect all bevel border
and combined them as a single border with four sides.

MozReview-Commit-ID: Bvu8Zf56YDF

Comment 7

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

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

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8943171 [details] [diff] [review]
Handle bevel borders.

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

::: layout/tables/nsTableFrame.cpp
@@ +6652,5 @@
>  
> +  ~BCPaintBorderAction()
> +  {
> +    if (mMode == Mode::CREATE_WEBRENDER_COMMANDS) {
> +      mCreateWebRenderCommandsData.~BCCreateWebRenderCommandsData();

This is a little scary, it at least needs a comment to explain that mCreateWebRenderCommandsData is inside a union and so doesn't get destroyed normally, but the BCBorderParameters need to get destroyed.

@@ +7477,5 @@
>      return;
>    }
>  
> +  if (param->mStartBevelOffset != 0 || param->mEndBevelOffset != 0) {
> +    mozilla::Side bevelSide = param->mStartBevelOffset != 0 ? param->mStartBevelSide : param->mEndBevelSide;

Isn't it possible for both the start and end to be nonzero? How does this deal with that scenario? Or are you assuming that if both are nonzero, then the two bevel side parameters will be the same? I think that might be a valid assumption, but it should have a comment and/or an assert.

@@ +7480,5 @@
> +  if (param->mStartBevelOffset != 0 || param->mEndBevelOffset != 0) {
> +    mozilla::Side bevelSide = param->mStartBevelOffset != 0 ? param->mStartBevelSide : param->mEndBevelSide;
> +
> +    // Left border has right side bevel and vice versa.
> +    // Top border has bottom side bevel and vice versa.

This comment needs a bit more explanation. I'd say:

// The left border is going to be beveled on its right edge because that's the edge that intersects other borders (in this case the top and bottom borders). Correspondingly, if the bevel side is "right" that means we are operating on the left border, and so store the parameters for that entry in aBevelBorders. Same goes for the other directions.

@@ +7764,5 @@
> +  if (param->mStartBevelOffset != 0 || param->mEndBevelOffset != 0) {
> +    mozilla::Side bevelSide = param->mStartBevelOffset != 0 ? param->mStartBevelSide : param->mEndBevelSide;
> +
> +    // Left border has right side bevel and vice versa.
> +    // Top border has bottom side bevel and vice versa.

// See detailed comment on equivalent code in BCBlockDirSeg::CreateWebRenderCommands

@@ +8053,5 @@
> +      borderRect = LayoutDeviceRect::FromUnknownRect(NSRectToRect(param->mBorderRect + aOffsetToReferenceFrame,
> +                                                                  param->mAppUnitsPerDevPixel));
> +      borderColor = param->mBorderColor;
> +      borderStyle = param->mBorderStyle;
> +      backfaceIsVisible = param->mBackfaceIsVisible;

This should probably be |= to combine the different backfaceIsVisible properties properly and not just pick up the last one. (And the variable would need to be initialized to false)

Comment 9

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Handle bevel borders.

Since bevel only appear outmost border of table. I collect all bevel border
and combined them as a single border with four sides.

MozReview-Commit-ID: Bvu8Zf56YDF

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8945009 [details] [diff] [review]
Handle bevel borders.

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

Looks good, and thank you for finishing this off! I can fix the typo and land it for you.

::: layout/tables/nsTableFrame.cpp
@@ +6653,5 @@
> +  ~BCPaintBorderAction()
> +  {
> +    // mCreateWebRenderCommandsData is in a union which means the destructor
> +    // wouldn't be called when BCPaintBorderAction get destroyed. So call the
> +    // destroctor here explicitly.

typo: destructor

Comment 11

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> Comment on attachment 8945009 [details] [diff] [review]
> Handle bevel borders.
> 
> Review of attachment 8945009 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, and thank you for finishing this off! I can fix the typo and
> land it for you.

Thank you very much!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/366bca651d89
Handle bevel borders. r=kats

Comment 13

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/366bca651d89