Stop a table with no background leaving the next one painted twice - #43
Merged
jakejackson1 merged 1 commit intoSep 7, 2026
Conversation
Every table writes a ___TABLE___BACKGROUNDS placeholder into the page and, when it ends, splices its backgrounds in behind it. The splice only ran when the table actually had backgrounds, so a table with none left its placeholder sitting in the page - and because the splice is a plain preg_replace over the whole page, the next table's backgrounds went behind that stale placeholder as well as behind its own. Five plain tables in front of a coloured one had its background painted six times. That is invisible while the fill is opaque, but a translucent background comes out darker than it was asked to be, a background image is drawn and drawn again, and the page content stream carries a copy of the whole lot each time. Take the placeholder out at the end of every level 1 table, whether or not anything was put behind it. The two preg_replace calls per buffer collapse into one str_replace, which is also no longer at the mercy of a $ or a backslash turning up in the background operators. tests/data/snapshots/table.pdf is regenerated: its fixture has a plain table in front of a coloured one, so three pixels of antialiasing along the table's right edge shift now that the background underneath is painted once. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
jakejackson1
merged commit Sep 7, 2026
b085aef
into
mirror/604-avoid-table-background
27 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A follow-up to #38, and stacked on it — both change the same statement at the end of
_tableWrite(), so this targets that branch and will retarget itself togravitypdfwhen #38 merges. Not a mirror of anything upstream; this is the overdraw #38's test plan set aside as out of its scope.Every table writes a
___TABLE___BACKGROUNDSplaceholder into the page, and when it ends it splices its collected backgrounds in behind it. The splice only ran when the table actually had backgrounds:so a table with none left its placeholder sitting in the page.
preg_replacehas no limit, and the page is one long string, so the next table's backgrounds went in behind that stale placeholder as well as behind its own.Each background-free table in front of a coloured one adds another copy. Five of them and the background is painted six times. That is invisible while the fill is opaque, which is why it went unnoticed, but:
page-break-inside: avoidis not involved. The plain leading table in #38's reproducer is what put the stale placeholder there, which is also why that PR's tests had to count distinct rectangles.Try it
Before: six
re foperators for the one background, and a band that rendersrgb(255, 20, 20)— very nearly solid red.After: one, and
rgb(255, 166, 166), which is the 35% tint that was asked for.Test plan
tests/Mpdf/TableBackgroundTest.php—redRectangles()no longer runsarray_uniqueover its matches, so the four cases Put the page back after measuring a page-break-inside:avoid block (mpdf/mpdf#533 #570 #785 #1131 #1666 #1805 #2075 #322 #1801) #38 added now count every rectangle drawn rather than every distinct one.testATableBackgroundInsideAnAvoidBlockThatStaysPutIsStillDrawnbecomes a real assertion because of it: on Put the page back after measuring a page-break-inside:avoid block (mpdf/mpdf#533 #570 #785 #1131 #1666 #1805 #2075 #322 #1801) #38 it finds two rectangles, not one.testATableWithNoBackgroundDoesNotLeaveTheNextOnePaintedTwice— one plain table then a red one. Two rectangles on Put the page back after measuring a page-break-inside:avoid block (mpdf/mpdf#533 #570 #785 #1131 #1666 #1805 #2075 #322 #1801) #38.testEachTableWithNoBackgroundUsedToAddAnotherCopy— five plain tables then a red one. Six on Put the page back after measuring a page-break-inside:avoid block (mpdf/mpdf#533 #570 #785 #1131 #1666 #1805 #2075 #322 #1801) #38.testATranslucentBackgroundIsNotDarkenedByBeingPaintedTwice— the same translucent table rendered alone and behind a plain one must emit the same number of fills. Fails on Put the page back after measuring a page-break-inside:avoid block (mpdf/mpdf#533 #570 #785 #1131 #1666 #1805 #2075 #322 #1801) #38 (1 vs 2).testATableWithNoBackgroundAfterAColouredOneIsStillUnaffected— the other order has never been wrong, because the coloured table's splice takes every placeholder out before the plain one writes its own. Control, passes on both.tests/Snapshots/TableBackgroundSnapshotTest.php— translucent red, then a plain table, then translucent red again, then plain, then translucent blue, then plain, then a translucent cell. Everything after the first plain table should match the shade of the one before it. Rendered on Put the page back after measuring a page-break-inside:avoid block (mpdf/mpdf#533 #570 #785 #1131 #1666 #1805 #2075 #322 #1801) #38 the second red, the blue and the green all come out visibly darker; 90,011 pixels differ at 120 dpi. Stored astests/data/snapshots/table-background.pdf; a fresh render compares at zero differing pixels.tests/data/snapshots/table.pdfis regenerated. Its fixture has a plain table in front of a coloured one, so it was hitting this. The background underneath is now painted once and three pixels of antialiasing along the table's right edge shift with it — at 120 dpi, against the zero-tolerance limit the harness uses, that is a failure unless the document is refreshed. Nothing else about the page moves.PageWriterwas stripping it at output time, now it is gone earlier.composer test— 1130 tests, 2653 assertions, up from 1126/2649 on Put the page back after measuring a page-break-inside:avoid block (mpdf/mpdf#533 #570 #785 #1131 #1666 #1805 #2075 #322 #1801) #38.composer csclean.phpstanoutput identical to Put the page back after measuring a page-break-inside:avoid block (mpdf/mpdf#533 #570 #785 #1131 #1666 #1805 #2075 #322 #1801) #38's.More info — why
str_replace, and why the placeholder can go earlyThe two
preg_replacecalls per buffer collapse into onestr_replace. The pattern was a literal with no metacharacters and the capture group existed only so the first pass could put the placeholder back for the second pass to remove, so the two passes together were always "replace the placeholder with a space, the backgrounds, and a newline either side". Onestr_replacesays that directly, and it is no longer at the mercy of a$or a backslash turning up in the background operators —preg_replacewould have read those as backreferences in the replacement string.Taking the placeholder out early is safe because it was already being taken out later:
PageWriter::writePages()strips any that survive on the way to the page. The only thing that changes is that a spent one is no longer around to catch a splice meant for someone else.$this->tableBackgrounds = []moves out of the!$this->keep_block_togetherarm's sibling position and stays at the end of the$level == 1block, exactly where #38 left it — the measuring pass still discards what it collected.