Position absolute not work as expected in table with border collapse

RESOLVED FIXED in Firefox 57

Status

()

Core
Layout: Tables
P2
normal
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: Robert, Assigned: Ya-Chieh Wu)

Tracking

({dev-doc-complete, regression, testcase})

45 Branch
mozilla57
dev-doc-complete, regression, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox54 wontfix, firefox55 wontfix, firefox56 wontfix, firefox57 fixed)

Details

(Whiteboard: [parity-chrome][webcompat])

Attachments

(5 attachments, 6 obsolete attachments)

676 bytes, text/html
Details
416 bytes, text/html
Details
425 bytes, text/html
Details
1.71 KB, patch
Ya-Chieh Wu
: review+
Details | Diff | Splinter Review
6.85 KB, patch
Ya-Chieh Wu
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 months ago
Created attachment 8884452 [details]
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

4 months ago
Regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cc473fe5dc512c450634506f68cbacfb40a06a23&tochange=3cc3b1968524248450c465c4ea2ee5596ffa65f2
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout
Ever confirmed: true
Keywords: regression, testcase
Product: Firefox → Core
Version: 54 Branch → 45 Branch
status-firefox54: --- → affected
status-firefox55: --- → affected
status-firefox56: --- → affected

Comment 2

4 months ago
Hi Astley,
Can you help find someone to look at this issue?
Flags: needinfo?(aschen)
Flags: needinfo?(aschen)
Priority: -- → P2
Keep ni myself before it's been assigned.
Flags: needinfo?(aschen)
Whiteboard: [parity-chrome]
TY, before Ya-Chien starts to work on the investigation, any idea about this bug?
Assignee: nobody → ywu
Status: NEW → ASSIGNED
Flags: needinfo?(aschen) → needinfo?(tlin)
Beside the observation described in comment 0, I don't have any idea off the top of my head.
Flags: needinfo?(tlin)
status-firefox54: affected → wontfix
status-firefox55: affected → fix-optional
status-firefox-esr52: --- → wontfix
status-firefox55: fix-optional → wontfix
Component: Layout → Layout: Tables
Whiteboard: [parity-chrome] → [parity-chrome][webcompat]
See Also: → bug 1223232
See Also: → bug 1379038
(Assignee)

Comment 6

4 months ago
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
(Assignee)

Updated

4 months ago
Blocks: 1379038
See Also: bug 1379038
(Assignee)

Comment 7

4 months ago
Created attachment 8891156 [details] [diff] [review]
fix-wrong-position-absolute-block-in-tabl.patch
Too late for 56.
status-firefox56: affected → wontfix
Too late for 56.
See Also: bug 1223232
(Assignee)

Comment 10

4 months ago
Created attachment 8891224 [details] [diff] [review]
fix-wrong-position-for-a-block-with-absol.patch
Attachment #8891156 - Attachment is obsolete: true
Attachment #8891224 - Flags: review?(dbaron)
(Assignee)

Comment 11

4 months ago
Created attachment 8891225 [details] [diff] [review]
reftest-for-a-block-with-absolute-positi.patch
Attachment #8891225 - Flags: review?(dbaron)
(Assignee)

Comment 12

4 months ago
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d61f74b7b56904bea9a06216eb31502ef69247c3
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.
Attachment #8891224 - Flags: review?(dbaron) → review-
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.
Attachment #8891225 - Flags: review?(dbaron) → review-
(Assignee)

Comment 15

4 months ago
Created attachment 8892252 [details] [diff] [review]
Bug1379306-fix-wrong-position-of-absolute-block-in-b.patch
Attachment #8891224 - Attachment is obsolete: true
(Assignee)

Comment 16

4 months ago
Created attachment 8892253 [details] [diff] [review]
0002-Bug1379306-reftest-for-absolute-block-in-border-coll.patch
Attachment #8891225 - Attachment is obsolete: true
(Assignee)

Comment 17

4 months ago
(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.
Flags: needinfo?(dbaron)
(Assignee)

Comment 18

4 months ago
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9048000cabed659f4a7ed26436e2724ca5ac173&selectedJob=119489937
try looks ok.
(Assignee)

Comment 19

4 months ago
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.
Flags: needinfo?(dbaron)
Attachment #8892252 - Flags: review?(dbaron)
(Assignee)

Updated

4 months ago
Attachment #8892253 - Flags: review?(dbaron)
Created attachment 8892758 [details]
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.
Created attachment 8892760 [details]
a testcase showing incorrect behavior with table row groups
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.
Attachment #8892253 - Flags: review?(dbaron) → review-
Attachment #8892252 - Flags: review?(dbaron) → review+
(Assignee)

Comment 23

4 months ago
Created attachment 8893650 [details] [diff] [review]
Bug1379306-reftest-for-position-absolute-block-that-.patch

try looks ok.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3aac76058678de66293d3710c6622da9d46dc04&selectedJob=120864513
Attachment #8892253 - Attachment is obsolete: true
Attachment #8893650 - Flags: review?(dbaron)
Comment on attachment 8893650 [details] [diff] [review]
Bug1379306-reftest-for-position-absolute-block-that-.patch

r=dbaron
Attachment #8893650 - Flags: review?(dbaron) → review+
The fix is probably worth uplifting to beta 56.
status-firefox56: wontfix → affected
status-firefox57: --- → affected
(Assignee)

Comment 26

3 months ago
Created attachment 8894730 [details] [diff] [review]
0001-Bug1379306-Fix-the-wrong-position-when-we-calculate-.patch
Attachment #8892252 - Attachment is obsolete: true
Attachment #8894730 - Flags: review+
(Assignee)

Comment 27

3 months ago
Created attachment 8894731 [details] [diff] [review]
0002-Bug1379306-Reftest-for-position-absolute-block-that-.patch
Attachment #8893650 - Attachment is obsolete: true
Attachment #8894731 - Flags: review+
(Assignee)

Comment 28

3 months ago
try: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=285f7fc5033081ede500f45ca2069fc501908339&selectedJob=121553062
(Assignee)

Updated

3 months ago
Keywords: checkin-needed

Comment 29

3 months ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed

Comment 30

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fe69c6a1b4a3
https://hg.mozilla.org/mozilla-central/rev/2cc10ab1ca28
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Keywords: dev-doc-needed
Please request Beta approval on this when you get a chance.
Flags: needinfo?(ywu)
Flags: in-testsuite+
(Assignee)

Comment 32

3 months ago
I don't think this worth uplifting. Let's ride the train as normal. Thx Ryan.
status-firefox56: affected → wontfix
Flags: needinfo?(ywu)
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.