1379306 - Position absolute not work as expected in table with border collapse

Status: RESOLVED FIXED

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

Description

[Robert]

Attachment: collapse.html

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170628075643

Steps to reproduce:

Table with border-collapse: collapse
Td with border
Div with position: absoluted


Actual results:

Div moves right-down half pixels of td border

Comment 1

[YF (Yang)]

Regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cc473fe5dc512c450634506f68cbacfb40a06a23&tochange=3cc3b1968524248450c465c4ea2ee5596ffa65f2

Comment 2

[Gerry Chang [:gchang]]

Hi Astley,
Can you help find someone to look at this issue?

Comment 3

[Astley Chen (inactive)]

Keep ni myself before it's been assigned.

Comment 4

[Astley Chen (inactive)]

TY, before Ya-Chien starts to work on the investigation, any idea about this bug?

Comment 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Beside the observation described in comment 0, I don't have any idea off the top of my head.

Comment 6

[Ya-Chieh Wu (inactive)]

It seems that when we have a border-collapse table, the element in td's *margin* is twice than expected. As I set the value of *border.IStart(outerWM)* and *border.BStart(outerWM)* [1] to half, the position of the element in border-collapse table seems right. However, I haven't sorted out what is the right fix for this.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsAbsoluteContainingBlock.cpp#734

Comment 7

[Ya-Chieh Wu (inactive)]

Attachment: fix-wrong-position-absolute-block-in-tabl.patch

Comment 8

[Astley Chen (inactive)]

Too late for 56.

Comment 9

[Astley Chen (inactive)]

Too late for 56.

Comment 10

[Ya-Chieh Wu (inactive)]

Attachment: fix-wrong-position-for-a-block-with-absol.patch

Comment 11

[Ya-Chieh Wu (inactive)]

Attachment: reftest-for-a-block-with-absolute-positi.patch

Comment 12

[Ya-Chieh Wu (inactive)]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d61f74b7b56904bea9a06216eb31502ef69247c3

Comment 13

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

Comment on attachment 8891224 [details] [diff] [review]
fix-wrong-position-for-a-block-with-absol.patch

As far as I can tell, this should *always* be GetUsedBorder, not just for BCTableCellFrame.

The other cases where there's a difference that should matter seem to be:
 * if the frame has a -moz-appearance that changes the border
 * if the frame is a *table* frame in border-collapse mode
 * if the frame is a table row or row group

I think you should update the patch to *always* use GetUsedBorder, and add tests for the above cases as well that fail without the patch and pass with the patch.

Otherwise I think this looks reasonable, but I'd like to look at the revised version.

Comment 14

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

Comment on attachment 8891225 [details] [diff] [review]
reftest-for-a-block-with-absolute-positi.patch

This reftest seems rather indirect and complex for testing the underlying problem.  It uses:
 * invalid HTML markup (missing <tr>)
 * anonymous table box generation to replace a table cell that's converted to block by being position:absolute
 * equivalent of a border in border-collapse mode with a regular block border (since the td no longer becomes a table-cell when it's position:absolute)

so it's far from trivial to verify that the equivalence that the test is asserting is actually the correct behavior.


It seems like it would be better to test the desired behavior somewhat more directly, e.g., by using valid markup, and using a position:absolute block that in the test is relative to the table-cell, but in the reference is relative to either (a) a div that contains the table or (b) a div inside the table-cell.

Comment 15

[Ya-Chieh Wu (inactive)]

Attachment: Bug1379306-fix-wrong-position-of-absolute-block-in-b.patch

Comment 16

[Ya-Chieh Wu (inactive)]

Attachment: 0002-Bug1379306-reftest-for-absolute-block-in-border-coll.patch

Comment 17

[Ya-Chieh Wu (inactive)]

(In reply to David Baron :dbaron: ⌚️UTC+2 from comment #13) 
> The other cases where there's a difference that should matter seem to be:
>  * if the frame has a -moz-appearance that changes the border
>  * if the frame is a *table* frame in border-collapse mode
>  * if the frame is a table row or row group
 
Firstly, dbaron, thank you for helping with the review and giving a lot of inputs. 
I have updated my patches but I don't know how to make such tests. 
I tried to put a position:absolute block that in the test is relative to the table 
but I can't see the |aDelegatingFrame| to be *table* frame or *table row* or *row group*.
Do I misunstand anything? thx for helping here.

Comment 18

[Ya-Chieh Wu (inactive)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9048000cabed659f4a7ed26436e2724ca5ac173&selectedJob=119489937
try looks ok.

Comment 19

[Ya-Chieh Wu (inactive)]

Comment on attachment 8892252 [details] [diff] [review]
Bug1379306-fix-wrong-position-of-absolute-block-in-b.patch

Hi dbraon,
I tried to make tests but I can't see the |aDelegatingFrame| to be *table* frame or *table row* or *row group*. I think we can give it a try since |try| looks fine.

Comment 20

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

Attachment: a testcase showing incorrect behavior with table rows

The position of the absolutely positioned element honors the border, even though the border is otherwise ignored.

Comment 21

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

Attachment: a testcase showing incorrect behavior with table row groups

Comment 22

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

Comment on attachment 8892253 [details] [diff] [review]
0002-Bug1379306-reftest-for-absolute-block-in-border-coll.patch

This still doesn't address these comments:
> * equivalent of a border in border-collapse mode with a regular block border (since the td no longer becomes a table-cell when it's position:absolute)

> It seems like it would be better to test the desired behavior somewhat more directly, e.g., by using valid markup, and using a position:absolute block that in the test is relative to the table-cell, but in the reference is relative to either (a) a div that contains the table or (b) a div inside the table-cell.

nor does it test any of the other cases that probably should be tested here.

Comment 23

[Ya-Chieh Wu (inactive)]

Attachment: Bug1379306-reftest-for-position-absolute-block-that-.patch

try looks ok.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3aac76058678de66293d3710c6622da9d46dc04&selectedJob=120864513

Comment 24

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

Comment on attachment 8893650 [details] [diff] [review]
Bug1379306-reftest-for-position-absolute-block-that-.patch

r=dbaron

Comment 25

[Astley Chen (inactive)]

The fix is probably worth uplifting to beta 56.

Comment 26

[Ya-Chieh Wu (inactive)]

Attachment: 0001-Bug1379306-Fix-the-wrong-position-when-we-calculate-.patch

Comment 27

[Ya-Chieh Wu (inactive)]

Attachment: 0002-Bug1379306-Reftest-for-position-absolute-block-that-.patch

Comment 28

[Ya-Chieh Wu (inactive)]

try: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=285f7fc5033081ede500f45ca2069fc501908339&selectedJob=121553062

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe69c6a1b4a3
Fix the wrong position when we calculate the position for position:absolute child. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cc10ab1ca28
Reftest for position:absolute block that is relative to table cell, row, row group. r=dbaron

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fe69c6a1b4a3
https://hg.mozilla.org/mozilla-central/rev/2cc10ab1ca28

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 32

[Ya-Chieh Wu (inactive)]

I don't think this worth uplifting. Let's ride the train as normal. Thx Ryan.

Comment 33

[Chris Mills [:cmills]]

I have documented this by adding a note to the browser compat section of the position reference page:

https://developer.mozilla.org/en-US/docs/Web/CSS/position#Browser_compatibility