Closed Bug 1626129 Opened 4 years ago Closed 4 years ago

Height computation for (nested?) box is somewhat different in printing

Categories

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

Product:
Core
Component:
Layout: Tables
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla78
Tracking Flags:
Tracking Status
firefox78 --- fixed

People

(Reporter: hiro, Assigned: TYLin)

References

Details

(Whiteboard: [layout:backlog:78])

Attachments

(7 files)

compasison.png
39.12 KB, image/png
Details
A frame tree dump of 422678-1.html
18.40 KB, text/plain
Details
A frame tree dump of 422678-1-ref.html
20.58 KB, text/plain
Details
1626129-1.html
346 bytes, text/html
Details
Screenshot of 1626129-1.html in paged mode.png
57.48 KB, image/png
Details
Bug 1626129 - Apply skip sides for nsTableCellFrame's border and padding, and use it to calculate available space to reflow children.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1626129 - Add more reftests for breaking a table cell.
47 bytes, text/x-phabricator-request
Details | Review

Open below file on LayoutDebugger on "PagedMode" and scroll down to the end of the contents and see the blue box height on the last page. You can see the difference.

https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/reftests/pagination/820496-1.html
https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/reftests/pagination/820496-1-ref.html

I am not sure this is a bug which has been already filed or not (I couldn't find any of relevant bugs though). This is one of reasons that some of reftests in layout/reftest/pagination fail with jgraham's wpt printing machinery.

Attached image compasison.png —

820496-1.html might not be a good example to represent this issue, since the blue background color in the test html is on <body> element, so I started thinking the blue background should be rendered on the whole area of the last page, whereas the blue background color in the ref html is on a <div>.

https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/reftests/bugs/422678-1.html might be a better example to show this issue. I am attaching two screenshots to compare with the test and the ref images.

Depends on: 1626165
No longer depends on: 1626165

I noticed that there is 4px difference between the vis-overflow of the second PageContent in test html and reference html. vis-overflow=(0,0,23040,37980) in the reference and vis-overflow=(0,0,23040,37740) in the test. I have no idea where this 4px difference comes from, maybe we accidentally factor a margin/padding for an nsIFrame when we do paginate?

Flags: needinfo?(aethanyc)
Attachment #9137320 - Attachment mime type: application/octet-stream → text/plain
Flags: needinfo?(aethanyc)
Attachment #9137322 - Attachment mime type: application/octet-stream → text/plain

This is a table layout bug. The table cell frame doesn't handle border and padding correctly in a paginated context.

For example, it didn't apply skip sides (by using ApplySkipSides) for the borderPadding. Also, it unconditionally subtracts block-axis border/padding from available block-size for its children here, and add it back after the cell size is known. This can go wrong even if the skip sides are applied. For example, if the table cell needs a second page, its bottom border/padding needs to be skipped on the first page.

Hiro, did you see other reftest failures other than table layout under jgraham's wpt printing machinery? If yes, I think it's worth filing such bugs.

(NI myself to revisit this later.)

Component: Printing: Output → Layout: Tables
Flags: needinfo?(aethanyc)
See Also: → 1627571

(In reply to Ting-Yu Lin [:TYLin] (UTC-8) from comment #4)

Hiro, did you see other reftest failures other than table layout under jgraham's wpt printing machinery? If yes, I think it's worth filing such bugs.

I filed bug 1627571 for a reftest failure which is not including any table related elements.

Though I haven't audited all failure cases, it looks like most of them are related to table frames, e.g. table-page-break-before-auto-2.html is one of them.

Whiteboard: [layout:backlog:77]

FYI, I saw "Set table cell incomplete 0x7fc1f2f27c10" when I run layout/reftests/pagination/1406291-1.html with the wpt printing stuff. It may or may not be related to this bug.

Attached file 1626129-1.html —

A test case adapted from 422678-1.html. The table cell's border and padding look pretty wrong in paged mode.

The <td>'s bottom border/padding should be skipped in the first page, and its top border/padding should be skipped in the second page as well.

Before this patch, nsTableCellFrame never skips border and padding. If
it needs to be broken into multiple parts, the cell's border and padding
didn't render correctly, and its children didn't have the correct
available space to reflow. (A side effect of this is that it renders
correctly if it has box-decoration-break:clone.)

table-nested-1308876-1-ref.html is modified to reflect the rendering
after applying this patch, which isn't necessarily correct (based on the
file's comment).

Assignee: nobody → aethanyc
Status: NEW → ASSIGNED
Flags: needinfo?(aethanyc)

  • 2a and 2b are copied from 1a and 1b with "box-decoration-break: clone"
    added.

  • 3a and 3b are copied from 1a and 1b. They test that the content fit
    but the table cell's bottom border and padding cannot fit. As of this
    bug, their rendering matches the block frame's, but 3a can now trigger a
    "data loss" warning and 3b an assertion in
    nsTableRowGroupFrame::SplitRowGroup. That might need a deeper look
    into table's pagination to have a proper fix.

Depends on D71096

Attachment #9142546 - Attachment is obsolete: true
Attachment #9142546 - Attachment is obsolete: false

Because this bug's Severity has not been changed from the default since it was filed, and it's Priority is P3 (Backlog,) indicating it has been triaged, the bug's Severity is being updated to S3 (normal.)

Severity: normal → S3
Pushed by aethanyc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3fdea5ae7d2a
Apply skip sides for nsTableCellFrame's border and padding, and use it to calculate available space to reflow children. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/f0e5f7503048
Add more reftests for breaking a table cell. r=dbaron

By looking at the log, table-nested-1308876-1.xhtml has a fuzzy pixel on macOS.

A try run with fuzzy annotation added https://treeherder.mozilla.org/#/jobs?repo=try&collapsedPushes=652615&revision=3e684edeb77c93c4021a3aa694170e9298b7fa30

Flags: needinfo?(aethanyc)
Pushed by aethanyc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1cdfd91b0a4f
Apply skip sides for nsTableCellFrame's border and padding, and use it to calculate available space to reflow children. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/751209e30850
Add more reftests for breaking a table cell. r=dbaron
Whiteboard: [layout:backlog:77] → [layout:backlog:78]

https://hg.mozilla.org/mozilla-central/rev/1cdfd91b0a4f
https://hg.mozilla.org/mozilla-central/rev/751209e30850

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox78: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla78
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: