Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Table Borders Fixes #2535

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Table Borders Fixes #2535

wants to merge 6 commits into from

Conversation

oleibman
Copy link
Contributor

@oleibman oleibman commented Dec 29, 2023

Fix #2402. Fix #2474. Both issues deal with borders around tables when they aren't wanted. There are 3 big issues in the code, and several minor ones.

First big issue - Word table styles can have both a styleId and a name, which are often different from each other, and each of which is used by various Word functions, and what documentation I can find is far from clear on the difference. I have added a tableStyle property (for styleId) to Style/Table, and the reader will now preserve both styleId and name. It will similarly preserve basedOn, which in now a private property in Style/Paragraph, but is changed to be a protected property in Style.

Second big issue - Word2007 Reader assumes that table style can be specified either by name or by inline declarations, but not both. Guess what? It is now changed to support both. This makes the delta for Reader/Word2007/AbstractPart appear to be much more complicated than it actually is. The change is almost entirely of the form:

if (condition) {short_code_block} else {long_code_block}

to

long_code_block

Third big issue. In html, td does not inherit border styles from table. In word, cell border styles are specified in table styles (as insideH/V), so they do, in effect, inherit. This is resolved, as best as I can, by having each td/th without its own style use the table border style. So adding an html border style should produce a consistent result in Html and Docx output.

Minor issues:

  • Html table (not css) attribute border=0 should set borderStyle none on all borders; any other value should set borderStyle single.
  • PhpWord accepts named colors from html styles. According to the documentation that I can find, Word does not recognize those, but, in practice, it often does. Nevertheless, I have added translation to hex (borrowed from PhpSpreadsheet). If nothing else, this will increase interoperability (e.g. RTF doesn't accept named colors, and html 3-hex-digit short forms are now permitted). If a color is not found in the translation table, it will be left unchanged, so there should be no impact.
  • Writer/Html/Style/Table now accepts colors as 6 hex digits, as well as strings.
  • The parsing of border css attributes is not accurate. It rejects legitimate values. One example is 2px solid red, since PhpWord, unlike html, insists on color before style. It rejects 2px #ff0000 solid because it doesn't accept colors as hex strings. It does not allow the omission of the size and color attributes, but css does. The parsing is rewritten to try to overcome these deficiencies. Note, BTW, that css border:0 is not acceptable css (size needs a unit and style is omitted); this was mentioned in one of the issues as not being handled correctly, but, since it is invalid, there should be no expectation of its being handed in any particular way.
  • Style/Border::hasBorder is expanded to test all of Size, Color, and Style, rather than limiting its test to Size.
  • Properties insideHStyle and insideVStyle are added to Style/Table. Their Color and Size equivalents already existed.
  • If border is not specified as an Html or css attribute on a table, it is not the same as specifying html border=0 or css border:none. The end result will be whatever the app that reads the result defaults to. The results may not be consistent between, say, Html and Docx. This is already addressed in part by setting default styling for table and td in the html head section to match the Word defaults. However, there may still be differences; the way to (mostly) avoid them is to specify a table style.
  • Add support for Html Font Style background-color.

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.

Fixes # (issue)

Checklist:

  • I have run composer run-script check --timeout=0 and no errors were reported
  • The new code is covered by unit tests (check build/coverage for coverage report)
  • I have updated the documentation to describe the changes

Fix PHPOffice#2402. Fix PHPOffice#2474. Both issues deal with borders around tables when they aren't wanted. There are 3 big issues in the code, and several minor ones.

First big issue - Word table styles can have both a `styleId` and a `name`, which are often different from each other, and each of which is used by various Word functions, and what documentation I can find is far from clear on the difference. I have added a `tableStyle` property (for styleId) to Style/Table, and the reader will now preserve both `styleId` and `name`. It will similarly preserve `basedOn`, which in now a private property in Style/Paragraph, but is changed to be a protected property in Style.

Second big issue - Word2007 Reader assumes that table style can be specified either by name or by inline declarations, but not both. Guess what? It is now changed to support both. This makes the delta for Reader/Word2007/AbstractPart appear to be much more complicated than it actually is. The change is almost entirely of the form:
```
if (condition) {short_code_block} else {long_code_block}
```
to
```
long_code_block
```

Third big issue. In html, td does not inherit border styles from table. In word, cell border styles are specified in table styles (as insideH/V), so they do, in effect, inherit. This is resolved, as best as I can, by having each td/th without its own style use the table border style. So adding an html border style should produce a consistent result in Html and Docx output.

Minor issues:
- Html table (not css) attribute border=0 should set borderStyle none on all borders; any other value should set borderStyle single.
- PhpWord accepts named colors from html styles. According to the documentation that I can find, Word does not recognize those, but, in practice, it often does. Nevertheless, I have added translation to hex (borrowed from PhpSpreadsheet). If nothing else, this will increase interoperability (e.g. RTF doesn't accept named colors, and html 3-hex-digit short forms are now permitted). If a color is not found in the translation table, it will be left unchanged, so there should be no impact.
- Writer/Html/Style/Table now accepts colors as 6 hex digits, as well as strings.
- The parsing of border css attributes is not accurate. It rejects legitimate values. One example is `2px solid red`, since PhpWord, unlike html, insists on color before style. It rejects `2px #ff0000 solid` because it doesn't accept colors as hex strings. It does not allow the omission of the size and color attributes, but css does. The parsing is rewritten to try to overcome these deficiencies. Note, BTW, that css `border:0` is not acceptable css (size needs a unit and style is omitted); this was mentioned in one of the issues as not being handled correctly, but, since it is invalid, there should be no expectation of its being handed in any particular way.
- Style/Border::hasBorder is expanded to test all of Size, Color, and Style, rather than limiting its test to Size.
- Properties insideHStyle and insideVStyle are added to Style/Table. Their Color and Size equivalents already existed.
- If border is not specified as an Html or css attribute on a table, it is not the same as specifying html border=0 or css border:none. The end result will be whatever the app that reads the result defaults to. The results may not be consistent between, say, Html and Docx. This is already addressed in part by setting default styling for table and td in the html head section to match the Word defaults. However, there may still be differences; the way to (mostly) avoid them is to specify a table style.
@coveralls
Copy link

coveralls commented Dec 29, 2023

Coverage Status

coverage: 97.021% (+0.01%) from 97.01%
when pulling 49a10ec on oleibman:styleid
into a7b9030 on PHPOffice:master.

Code to support it was missing, and is now added.
@Progi1984
Copy link
Member

@oleibman You have conflicting files. Could you rebase your PR ?

@oleibman
Copy link
Contributor Author

oleibman commented Jan 8, 2024

@Progi1984 I don't know how to rebase the PR. I do know how to resolve the trivial conflicts here on Github, but you didn't like the way it handled that with an earlier PR - I think it did a "merge commit" rather than your preferred rebase. Why wasn't that okay? And, assuming it still wouldn't be okay here, how do I go about rebasing?

@Progi1984
Copy link
Member

@oleibman Aouch! Hard to review. I will make it on computer.

Next Time : could you make a PR for each bug fix / improvement ?

@Progi1984 Progi1984 added this to the 1.3.0 milestone Aug 20, 2024
@Progi1984 Progi1984 modified the milestones: 1.3.0, 1.3.1 Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants