Skip to content

A "Q" still leaves the setters' record of the page stale everywhere outside the image transforms #78

Description

@jakejackson1

#71 / #75 fixed this for transformed images. The same defect is still present at every other q/Q pair in the codebase, and one of them — PaintDivBB() — affects 18 of the 48 snapshot documents.

The mechanism

SetLineWidth(), SetDColor(), SetFColor(), SetLineCap(), SetLineJoin(), SetDash() and the font/text-rendering setters only write a PDF operator when its value differs from the one they last wrote, remembered in $pageoutput[$this->page]. q pushes the graphics state and Q pops it, undoing whatever the operators between them set — so after a raw Q that record no longer describes the page. Two things follow:

  • a setter asked for a value the record already claims is in force stays quiet, and the next thing drawn takes whatever the Q restored;
  • a setter asked for a value the Q has just put back writes it again, for nothing.

#75 added saveGraphicsState() / restoreGraphicsState(), which write the q/Q and push/pop the record with them, but only converted the two wrappers in printobjectbuffer() and StartTransform()/StopTransform().

Evidence

Converting only the four PaintDivBB() wrappers to the pair and rendering the snapshot corpus: 18 of 48 documents change.

tests/Snapshots: Tests: 48, Skipped: 18
background-and-borders  background-size  barcode  columns  dictionary-line-breaking
dot-tab  justify  line-height  list-style-type  page-break-avoid-layout
page-break-avoid-state  paging-css  positioned-html  rtl  shadow  table
toc-and-index  zero-font-size

Both directions show up. Operators that were wrongly skipped and are now written — 14 of them, in background-and-borders, line-height and positioned-html:

 Q
-0.000 G
+0.283 w
 2 j
-2 J
 q
 0.000 w

That +0.283 w is SetLineWidth(0.1) in the // Reset Corners and Dash off block at src/Mpdf.php:17130. It was skipped because the record still held the width set inside the wrapper the Q had just closed, so whatever was stroked next used the width the restore put back instead of the 0.1mm asked for.

And operators that were being written for nothing, across the corpus:

operator redundant writes
0.000 G 376
2 J 326
2 j 288
0.283 w 191

The sites

Verified at 97d3fe1. Each of these has setter calls between the q and the Q:

where q Q
PaintDivBB() — four border clips src/Mpdf.php:17048, :17145, :17209, :17270 :17127, :17191, :17255, :17316
_tableWrite()$tableClipPath, wraps a whole table body :22052, :22473 :22336, :23247
WriteFixedPosHTML()overflow: hidden clip :14838 :14873
Form.php — textarea clip around MultiCell() src/Form.php:358 src/Form.php:369
Rotate() — writes an unmatched Q, then a fresh q, straddling arbitrary content :10832 :10829

Rotate() is the case restoreGraphicsState()'s empty-stack fallback was written for: it restores a state nothing recorded, so clearing the record outright is the right answer there.

What exists only because of this

  • The four // Reset Corners and Dash off blocks after PaintDivBB()'s wrappers (src/Mpdf.php:17130, :17194, :17258, :17319) re-issue SetLineWidth/SetDColor/SetLineJoin/SetLineCap/SetDash by hand — and are themselves subject to the defect, which is where the 14 skipped widths come from.
  • Cell(), Text() and _printListBullet() (:7866) deliberately build their q … Q blocks out of raw operators (0 j 0 J [] 0 d, %.3F w) rather than calling the setters, specifically so the record is never touched. Correct as they stand — but only convert them together with the q/Q, never one without the other.
  • printkwtbuffer() clears the record outright (:25423) after placing a StopTransform(true) wrapper. The other four StartTransform(true)/StopTransform(true) sites — header/footer at :10071, column balancing at :24795 and :24914, rotated table at :25142 — do not. The returned form can't use the stack (its q is written later, out of call order), so those need the outright clear, or an explanation of why they don't.

Suggested fix

Route every one of them through saveGraphicsState() / restoreGraphicsState(), then delete the four reset blocks and the hand-written clear. For $tableClipPath and the Form clip, keep the re W n and pass it as the $operators argument rather than baking the q into the string.

Worth considering at the same time, since it is the reason the mechanism keeps getting missed: $pageoutput and the eight setters that consult it (58 call sites, keys Font 18, TextRendering 12, FillColor 8, LineWidth/LineJoin/LineCap/DrawColor/Dash 4 each) all live in Mpdf.php with no single owner. Moving them into one collaborator would put the "only write when changed" rule and its save/restore in one place. Note that getStateSnapshot() hand-lists the collaborators it captures (form, tableOfContents only), so anything holding the stack has to be added there or look-ahead unwinding strands entries.

This will move snapshot fixtures — unlike #75, which left every byte unchanged — so the PR should regenerate them and the pixel comparison on CI is what confirms nothing rendered differently.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions