1379038 - Border Collapse causing 1px off

Status: RESOLVED DUPLICATE of bug 688556

(Core :: Layout: Tables, defect, P2)

Description

[Hugh Nougher [:Hughman]]

Attachment: test.html

Affects Firefox 54.0.1 (x64) and Nightly 56 (x64) on Windows.

Attached test case shows when border collapse is applied there is 1px issues.

In the test.html:
1. Div in Div baseline. Cyan border all round both times.
2. Div in Table (no collapse). Cyan border all round both times.
3. Div in Table with collapse.
   - First has red instead of Cyan on right and bottom borders.
   - Second has 1px red inside top/left border and yellow instead of right/bottom border.
4. Div in Table with collapse and offset -1,-1 on the div.
   - First shows same as 3A.
   - Second shows yellow box filling exactly where its supposed to be.
   - Second has red border issue like 3A.

Comment 1

[Hugh Nougher [:Hughman]]

Attachment: Screenshot of Test.png

Comment 2

[Ryan Hunt [:rhunt]]

It looks like chrome is exhibiting problems on 3.b. My guess is this is a layout problem. Feel free to move it back if it is not.

Comment 3

[Ya-Chieh Wu (inactive)]

I just do a quick look from the test case and it seems that we get two issues here.
(1) when the child's style is *position absolute* and td's style is *position relative*, the child's position is wrong.
(2) when the td's style is relative, the background size of td seems wrong.

For (1), after Bug1379306, I think it will go away.
For (2), we have to handle it here.

Comment 4

[Astley Chen (inactive)]

(In reply to Ya-Chieh Wu from comment #3)
> For (2), we have to handle it here.

Case 2 seems affected by bug 1223232. You might be able to take a look.

Comment 5

[Ya-Chieh Wu (inactive)]

Attachment: test3.html

As far as I can tell, the backgound size of td is the same whether you have "position:relative" style on td or not. I am seeing that if we have "position:relative" on td, then the background of td is on top of the boder. If we don't have "position:relative" on td, then then the boder is on top of the background of td. 

As you can see from the testcase.

Comment 6

[Ya-Chieh Wu (inactive)]

Hi David,

I would like to know how you think about this since border-collapse's behavior is complicated.
As the attachment on Comment 5, I am seeing that the background of td is laying on top/bottom of the border when we have "position:relative"/"position:static". Thus, when we have only "1px" on the border, it is obvious that the border will overlap with the background.

I am thinking if
(1) we should adjust the background to align the td's border. // size is correct but we sholud shift the background to top and left to half boder when border-collapse. 
Or (2) we should change the size of the background. 
Or (3) we should just modify the laying order. // this seems less possible

thx!

Comment 7

[Ya-Chieh Wu (inactive)]

It seems that comment 6  would only happen when border is "1px" width. 
As I tried to use wider boder, everything looks normal.
clear david's needinfo first.

Comment 8

[Ya-Chieh Wu (inactive)]

so it seems that whenever we have a border-collapse table and the td's border use an odd length of the pixel, like 1px or 3px or 5px..., the background of td will shift half length of |nsPresContext::AppUnitsPerCSSPixel| to right and bottom.  I think I have to try to shift the background back.

Comment 9

[Ya-Chieh Wu (inactive)]

Attachment: wip patch

Hi David,

As the test case, I tried to shift the background of td to align the "logic" border for a border-collapse table as it seems that we don't perfectly divide the border to half for a border-collapse table. any suggestions about this?

Comment 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 8896851 [details] [diff] [review]
wip patch

What it is that you're trying to do here?  It's not clear to me.

Remember that background-clip's initial value is border-box:
https://drafts.csswg.org/css-backgrounds/#the-background-clip
while background-origin's initial value is padding-box:
https://drafts.csswg.org/css-backgrounds/#the-background-origin

Are we aligning one of them incorrectly as a result of the rounding we do for border-collapse borders?  (I'd think that the override of nsIFrame::GetUsedBorder would help get this right, but I could be wrong.)

Comment 11

[Ya-Chieh Wu (inactive)]

I have seen that whenever we set an "odd" px of the border size, we don't cut the border equally on the border-collapse table. 
For example, if the border is "1px", you can see the |GetUsedBorder| of td get
|mozilla::gfx::BaseMargin<int, nsMargin> = (top = 0, right = 60, bottom = 60, left = 0)|. // 60 is an app unit

I think this is because of the calculation of |DivideBCBorderSize|.
I think this is why the background of the td has shifted half app unit to right and bottom as the test case.
And if the size of the border is an "even" px, the background of td will act as expected. 

So I think that when we encounter an "odd" px,
we might need to shift the background to half app unit to left and top.
How do you think about this?

Comment 12

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

So a background doesn't have a single position:  it has two, one from background-origin and the other from background-clip.  Both of those properties can also be set to border-box, padding-box, or content-box.  So I think it would be more useful if you describe what it is you want to change in terms of where we consider the border-box and padding-box to be during background drawing of border-collapse cells, or if there's some other change in the model that you're proposing.

Comment 13

[Ya-Chieh Wu (inactive)]

Attachment: wip

Hi David,

In this patch, I tried to change the background's range during background drawing of border-collapse cells. Because in gecko, we don't divide the border to equally half when we have an "odd" width of the border. It is particularly obvious that the background will eat out the right border when we have a 1px width border. I think, to take care of this we only need to change the background's range when border-box is set to background-clip or background-origin.

Comment 14

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 8898592 [details] [diff] [review]
wip

I don't understand why you're adding so much code.

I would expect this to just work, because nsCSSRendering::GetImageLayerClip calls nsIFrame::GetUsedBorder, which nsBCTableCellFrame::GetUsedBorder() overrides to account for the border rounding.

It's also still not clear to me what you're trying to fix -- which of background-origin or background-clip is wrong without the patch?  (And isn't mask-origin and/or mask-clip wrong in the same way -- in a way that this patch wouldn't fix?)

I'd expect a patch to fix this to be 1-3 lines of code, not 20, if it's done in the right place.

Comment 15

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(In reply to Ya-Chieh Wu from comment #5)
> As far as I can tell, the backgound size of td is the same whether you have
> "position:relative" style on td or not. I am seeing that if we have
> "position:relative" on td, then the background of td is on top of the boder.
> If we don't have "position:relative" on td, then then the boder is on top of
> the background of td. 

This makes me suspect that the position may be correct, and maybe the only problem is the z-ordering.

Comment 16

[Ya-Chieh Wu (inactive)]

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #15)
> (In reply to Ya-Chieh Wu from comment #5)
> > As far as I can tell, the backgound size of td is the same whether you have
> > "position:relative" style on td or not. I am seeing that if we have
> > "position:relative" on td, then the background of TD is on top of the boder.
> > If we don't have "position:relative" on td, then then the boder is on top of
> > the background of td. 
> 
> This makes me suspect that the position may be correct, and maybe the only
> problem is the z-ordering.

z-ordering is different with and without "position:relative". But this seems to be another issue(or not a issue).
What I find here is I see the |right| and |bottom| border are covered by background when the border's size is odd pixel. 
Although there is only |half pixel| shifted.  
This is what I tried to fix here and I choose to change background's position because I think
if I touch |nsBCTableCellFrame::GetUsedBorder()|, this will affect how border is drawn. 

And I had tried to touch |nsCSSRendering::GetImageLayerClip| but since |nsBCTableCellFrame::GetUsedBorder()| 
didn't store the right(the size we think) size of the border as we think so it didn't work there.
/* 1px border. nsBCTableCellFrame::GetUsedBorder()=|mozilla::gfx::BaseMargin<int, nsMargin> = (top = 0, right = 60, bottom = 60, left = 0)|.*/ I know that if I shift the background, it will mismatch background to who is actually own
the border but this seems because of how we divide the border. Or we can change |BCPixelSize|'s type?
In that patch, I tried to fix both background-origin and background-clip when border-box is set to them. 
Is this description ok for you? Or I miss anything? thx!

Comment 17

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Remember that if:
 * the borders are dashed, dotted, partially opaque, or transparent
 * the 'background-clip' is still 'border-box'
then you should be able to see that the backgrounds are all adjacent, with no gaps between them.

When 'background-clip' is 'border-box', the background should be drawn under the border -- or in the border-collapse case, the half of the border associated with the cell.  Are you trying to describe that as a bug?

Comment 18

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

The fact that things are visibly off when very zoomed is bug 895096 -- we should be doing the pixel snapping to device pixels rather than CSS pixels.

Comment 19

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

The z-ordering issue with relative positioning in border-collapse tables is bug 688556.

Comment 20

[Ya-Chieh Wu (inactive)]

I tested the test cases and I think this bug could be fixed by Bug 895096.
And I am about to land Bug 895096.

Comment 21

[Ya-Chieh Wu (inactive)]

This bug should be fixed by Bug 895096.

Comment 22

[Hugh Nougher [:Hughman]]

I have checked the original test case again on the current Nightly (2017-09-24) and although there is improvements to the positioning, its still wrong for some of the original test cases.

Cases 1, 2 are fine.
Case 4 is invalid now as the yellow box is in the right place.

Case 3 still shows the background covering the right and bottom borders.
I'll attach an updated screenshot.

I'll just reopen this bug since it has not had any patches committed from it.

Comment 23

[Hugh Nougher [:Hughman]]

Attachment: Screenshot of Test 170924.png

Comment 24

[Ya-Chieh Wu (inactive)]

As the comments before, there are two issues here. 

(1) The borders are visibly off when very zoomed in border-collapse tables.
(2) The z-ordering issue with relative positioning in border-collapse tables.

The z-ordering issue is bug 688556.
For the borders are visibly off, we have been fixed it in Bug 895096.

Comment 25

[Ya-Chieh Wu (inactive)]

since we fixed Bug 895096, this bug will be a duplicate of bug 688556.

Comment 26

[Hugh Nougher [:Hughman]]

Oh I see!
That's were the odd-even findings came into it!
With even border width, it looks like the test case in bug 688556 while odd has one extra px on top/left and one lost on right/bottom.