The fill of a table row is written, in both Writers

[dkulyk]

Description

Row::getFill() reached the file by accident or not at all.

Row::nextCell() copied the fill of the row onto the cell it moved to, so a table walked with nextCell() came out banded, while the same table reached through getCell() came out blank. And because the copy happened at the moment of the walk, a fill set on the row after its cells were made was lost either way. Measured on one deck, three tables, same fill:

how the fill is asked for	in the file, before	after
$row->setFill(...), cells reached with getCell()	nothing	FFEEDD
$row->setFill(...), cells walked with nextCell()	FFEEDD	FFEEDD
the fill set on each cell by hand	FFEEDD	FFEEDD

Neither format has a fill on a row to write it to, so painting the cells is not a shortcut -- it is the only representation there is:

• OOXML: CT_TableRow is tc*, extLst and a required h. A row has a height and nothing else. What PowerPoint shows as a banded row comes from the table style, a:band1H and friends, which is a property of the table, not a fill on a row. Filling a row by hand in PowerPoint writes an a:solidFill into every one of its cells.
• ODF: style:table-row-properties does accept a flat fo:background-color, but no gradient, and LibreOffice does not draw it in a presentation. I wrote the attribute into the row style by hand and converted both files to PDF: 0 pixels of 389 520 differ. What LibreOffice writes instead is table:table-row/@table:default-cell-style-name, a cell style the row's cells inherit unless they carry one of their own -- again, the fill lands on cells.

So both Writers now paint a cell that has no fill of its own with the fill of its row. A cell that asks for a fill of its own keeps it -- the fill of the row is what a cell falls back to, not what it is overruled by. The ODPresentation Writer needs the gradient of a row defined as well, or the style of the cell would name a gradient_* that is not in the file.

A cell that was given no fill, and a cell that refused one

Following the review, Fill::FILL_NONE turned out to be doing two jobs: the state a cell starts in, and the state a cell is put in by asking for no fill. Reading the first as "take the row's" made the second impossible to express -- a transparent cell inside a painted row, which is exactly what both applications write when you ask for one: PowerPoint leaves an untouched cell's a:tcPr empty and writes a:noFill for a refusal, LibreOffice leaves an untouched cell without a table:style-name and gives a refusal a style with draw:fill="none".

So a table Cell and a table Row are now born at a new Fill::FILL_UNSET, and that is what the Writers resolve against the row. FILL_NONE means what it says everywhere else in the library: no fill, and none inherited either.

Two consequences, both in the changelog under BC Breaks:

• $cell->getFill()->getFillType() on an untouched cell returns 'unset' where it used to return 'none'.
• Row::nextCell() no longer copies the row's fill onto the cell it moves to. The copy was already destructive -- walking past a cell replaced a fill it had been given -- and under FILL_UNSET it would turn "not asked" into an explicit value at navigation time, undoing the distinction as you read the row. Nothing needs it now that the Writers resolve the row's fill when they write.

One thing this PR does not change: an unset cell is still written as a:noFill, as it always was, so no existing deck moves. Writing nothing instead, and letting the table style in tableStyles.xml paint the cell the way PowerPoint would, is a separate decision and a separate PR. The readers are a follow-up too: Reader\PowerPoint2007::loadStyleFill() has no a:noFill branch, so a refusal is dropped on read, and Reader\ODPresentation has no table handling at all.

Checklist:

• My CI is 🟢

  phpstan is clean and the suite passes: 823 tests, 7255 assertions.

• I have covered by unit tests my new code (check build/coverage for coverage report)

  For the row's fill: PptSlidesTest::testFillSolidTableRow and ContentTest::testTableRowFill cover both cells taking the fill of the row and a cell keeping its own, StylesTest::testGradientTableRow the gradient a row asks for. For a cell that has nothing: testFillUntouchedTableRow and testTableRowFillUntouched. For a cell that refuses: testFillNoneCellInFilledRow and testTableCellFillNoneInFilledRow. For the deletion: RowTest::testNextCellKeepsWhatTheCellWasGiven. Each one I checked by breaking the line it covers and watching it fail.

• I have updated the documentation to describe the changes

  docs/usage/shapes/table.md gained a "Define the fill" section: what a row fill does, that a cell fill wins, and how a cell asks to stay transparent inside a filled row.

• I have updated the changelog

  An entry under 1.3.0, and the two BC Breaks above.

[coveralls]

coverage: 92.441% (+0.005%) from 92.436% — sapientpro:fix/table-row-fill into PHPOffice:master

[Progi1984]

Small comment (which applies multiple times)

Need a rebase too

[dkulyk]

@Progi1984 Good question, and following it turned up something worth deciding together, so let me lay out what both formats actually do.

What PowerPoint writes

CT_TableRow has no fill: per ECMA-376 an <a:tr> carries h, its <a:tc> children and extLst, nothing else. "Fill a row" in the PowerPoint UI is therefore not stored on the row — PowerPoint writes the fill into every cell of it.

Here is a deck saved by PowerPoint 16 — one table, the third row filled red, and the fourth cell of that row set to "No fill":

<a:tr h="370840">
  <a:tc>…<a:tcPr><a:solidFill><a:srgbClr val="C00000"/></a:solidFill></a:tcPr></a:tc>
  <a:tc>…<a:tcPr><a:solidFill><a:srgbClr val="C00000"/></a:solidFill></a:tcPr></a:tc>
  <a:tc>…<a:tcPr><a:solidFill><a:srgbClr val="C00000"/></a:solidFill></a:tcPr></a:tc>
  <a:tc>…<a:tcPr><a:noFill/></a:tcPr></a:tc>
  …
</a:tr>

The two rows above it are untouched, and their cells are just <a:tcPr/> — empty. Their colours come from <a:tableStyleId>.

What LibreOffice writes

The same deck, converted with soffice --convert-to odp, comes out structured differently — and this is not style:table-row-properties:

<table:table-row table:style-name="ro1" table:default-cell-style-name="ce3">
  <table:table-cell/>
  <table:table-cell/>
  <table:table-cell/>
  <table:table-cell table:style-name="ce4"/>
  …
</table:table-row>

with ro1 holding only style:row-height, ce3 holding draw:fill="solid" draw:fill-color="#c00000", and ce4 holding draw:fill="none". So in ODF a row's fill lives in the default cell style the row points at, and a cell either says nothing and inherits it, or brings its own style and overrides it.

The point

Both formats distinguish "this cell was not given a fill" from "this cell was given no fill on purpose" — OOXML by an empty tcPr versus <a:noFill/>, ODF by an absent table:style-name versus a cell style with draw:fill="none". The only place that cannot tell them apart is this library: Fill::$fillType defaults to FILL_NONE (Style/Fill.php#L55), so an untouched cell and an explicitly transparent one are the same object state. As this PR stands it reads FILL_NONE as "has none of its own", which makes the fourth cell above inexpressible.

Options

1. A Fill::FILL_UNSET sentinel, as the default fill type of Cell and Row. UNSET means "not asked", FILL_NONE becomes explicitly transparent, and the writers resolve UNSET against the row. What it touches:

• Shape/Table/Cell.php, Shape/Table/Row.php — the fill each is born with. Row has to change too: nextCell() clones the row's fill onto the cell (Row.php#L125), so a row still defaulting to FILL_NONE would silently stamp every cell walked as explicitly transparent;
• Writer/PowerPoint2007/AbstractDecoratorWriter.php#L143 — writeFill() matches FILL_NONE, then solid, then gradient, then falls through to writePatternFill(). An UNSET fill arriving there would emit a:pattFill prst="unset", so it needs the same branch as FILL_NONE;
• the three table sites this PR already touches;
• Reader/PowerPoint2007.php#L1184, see the last note.

Everything else keeps the current FILL_NONE default — charts, shapes, backgrounds are untouched. It also composes with the idiom people write everywhere, $cell->getFill()->setFillType(…), which moves the cell out of UNSET by itself, with no new API to remember. The costs are a public constant that only means anything for table cells, and getFillType() on an untouched cell returning 'unset' instead of 'none'.

2. Cell::$fill nullable, null meaning "not asked". No sentinel in the public enum, but more machinery: a cell has no reference to its row today (Row::__construct() does a bare new Cell()), so one has to be added; and getFill(): Fill cannot hand back a copy of the row's fill, because $cell->getFill()->setFillType(…) would then mutate a throwaway. It would have to materialise — $this->fill = clone $row->getFill() on first read — which makes a cell "explicitly set" merely by being read, and the writers read it. Workable with a hasFill(): bool for the writers to ask instead, but that is three new members against one constant.

3. Row::setFill() copies the fill onto its cells there and then, extending what nextCell() already does. The writers then need nothing at all: this PR's three fallbacks disappear, FILL_NONE on a cell stays explicitly transparent, and a cell set after the row keeps its own colour. The catch is that $row->getFill()->setFillType(…), mutating the row's Fill in place, would not propagate — only setFill() would.

4. Leave it as this PR has it and document that a cell with no fill of its own takes its row's. Zero cost, and the limitation is narrow: a transparent cell inside a filled row, which was not expressible before this PR either, since a row's fill never reached the file at all.

I lean towards 1, with 3 the close second — it is the smallest diff of the four.

A second question, separable from the first

Whichever option you pick, there is a choice about what an unset cell should be written as. Today every untouched cell gets <a:noFill/>. But AbstractSlide.php#L427 already writes <a:tblPr firstRow="1" bandRow="1"/> with no a:tableStyleId, and PptTableProps.php#L39 ships a tableStyles.xml whose def is {5C22544A-7EE6-4342-B048-85BDC9FD1C3A} — the very GUID PowerPoint puts in the file above. So PowerPoint resolves our tables to that table style and would band them, first row and all; the a:noFill we write on every cell is what suppresses it.

So UNSET could be written either as a:noFill, exactly as today, with no visual change anywhere; or as nothing at all, letting the table style paint the cell, which is what both applications do natively (and in ODF, by not writing table:style-name on the cell). The second is more faithful but changes the look of every table already out there, so it is probably its own PR rather than this one. I mention it because it is the reason to keep UNSET and NONE apart in the first place.

Worth noting for the ODP side of any of this: the row's fill can go on table:table-row/@table:default-cell-style-name, the way LibreOffice writes it, instead of being copied into every cell's style. Fewer styles, and it is what the format expects. The comment currently in Content.php about a row not being able to carry a fill is only true of style:table-row-properties; I had missed this attribute.

The readers need the same decision

Whatever the writers end up meaning by "unset", the readers have to agree with them, and today neither does.

Reader\PowerPoint2007 reads the cells (#L1145) but loadStyleFill() (#L1184) knows only a:gradFill and a:solidFill and returns null for everything else, so <a:noFill/> is dropped: the fourth cell above comes back with nothing set. Under any option here that is the difference between "explicitly transparent" and "not asked", so it stops being cosmetic — it needs an a:noFill branch returning an explicit FILL_NONE. The empty <a:tcPr/> cells need no work: they simply leave the cell at its default, which is the right answer once the default carries the distinction.

Reader\ODPresentation has no table handling at all — no table:table, no rows, no cells — so an ODP this library writes cannot be read back as a table, and there is nothing there yet to teach about table:default-cell-style-name or draw:fill="none". Worth knowing before we decide how much of this belongs in one PR.

I would rather do the writers here and the readers in a follow-up, unless you want them together.

Meanwhile I have rebased onto master (the only conflict was the changelog, both entries kept) and added three tests for the case you were asking about: a cell whose row has no fill either, in both writers, plus the gradient path in ODPresentation\Styles, where a row with no fill writes no gradient at all.

[dkulyk]

@Progi1984 As for the other PRs, just let me know what needs to be updated or rebased, and I’ll take care of it.

[Progi1984]

I like the option 1.

[dkulyk]

Option 1 it is — I'll implement it here.

One thing that falls out of it and is worth flagging, because it is a deletion rather than an addition: Row::nextCell() should stop touching the cell's fill (Row.php#L125, $this->cells[$i]->setFill(clone $this->getFill())).

It is already destructive today — walking past a cell that was given its own fill overwrites it with the row's — and under UNSET it would be worse: it turns "not asked" into an explicit value at navigation time, silently destroying the very distinction we are adding. Nothing depends on it: RowTest::testNextCell() only asserts the return type, and Sample_04_Table.php sets a row fill and then walks the row, which keeps rendering identically once the writers do the inheritance. So the copy stops being needed exactly when UNSET starts working.

The rest, defaulted so you only need to answer if you disagree:

• Readers — a follow-up PR. Reader\PowerPoint2007::loadStyleFill() needs an a:noFill branch returning an explicit FILL_NONE, otherwise a deck round-trips as "not asked" and loses its transparent cells. Reader\ODPresentation is the bigger half: it has no table handling at all. Worth knowing that it is not starting from zero, though — it already keeps a style registry ($arrayStyles, filled by loadStyle() from office:automatic-styles and resolved by name for slide backgrounds, shape fills and paragraph spacing), so tables mean teaching it the table-cell and table-row families and walking the rows, not inventing a mechanism.

• What an unset cell is written as — unchanged here. Staying with a:noFill, so nothing moves visually. Writing nothing instead, and letting the tableStyles.xml def paint the cell as PowerPoint would, deserves its own PR and its own decision.

• table:default-cell-style-name — not in this PR, and not for the reason I first gave. It is valid ODF 1.2 and it is how LibreOffice writes a row's fill, but it would do nothing here yet: Content.php gives every cell its own table-cell style for the borders and paragraph properties, and a row default is only consulted for cells that carry no table:style-name of their own. Making that per-cell style conditional is a change of its own, so the row's fill stays copied into each cell style for now.

I'll push the UNSET implementation on top of the rebase and the tests already here.