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

Add support for tables #1577

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

Add support for tables #1577

wants to merge 8 commits into from

Conversation

hollandjake
Copy link
Contributor

@hollandjake hollandjake commented Dec 24, 2024

What kind of change does this PR introduce?

  • Add doc.table() support
  • Tables can have each cell individually customised
  • Tables have accessibility support
  • See the guide for new examples

Checklist:

  • Unit Tests
  • Documentation
  • Update CHANGELOG.md
  • Ready to be merged

Resolves #29

Note this fixes some other bugs relating to guide.pdf generation as well as removing comments from build (to remove the jsdoc comments)

@hollandjake
Copy link
Contributor Author

I've just noticed @blikblum had added some visual test for pdfmake, was there any indication as to if we were wanting the table interface to be similar to pdfmakes?

@MatthijsReyers
Copy link

Great feature! Tables are really the one thing I was missing in pdfkit, I've pulled your branch to play around a bit and I was just wondering if there is any way to create tables with columns of different widths? That seems like it should be pretty core functionality, but I can't figure out how to do it, intuitively I would expect the API to use one of these properties, but it does not seem to work:

await doc.table({ 
        rows: 2, 
        height: 100,
        width: 10 + 20 + 30,
        cellWidth: [10, 20, 30],
    })
    .row([
        {
            value: 'A', 
            cellWidth: 10,
            width: 10,
        },
        {
            value: 'B', 
            cellWidth: 20,
            width: 20,
        },
        {
            value: 'C', 
            cellWidth: 30,
            width: 30,
        },
    ])
    .row([
        'D', 
        'E',
        'F'
    ])

@hollandjake
Copy link
Contributor Author

No way to do column width atm but there is colspan support

@hollandjake
Copy link
Contributor Author

@MatthijsReyers originally I implemented the table logic to follow along with how HTML tables are generated, but I'm thinking of rewriting the table generation logic to follow more along with the pdfmake approach, allowing us to define column widths instead of number of columns (with auto support of course).

@hollandjake hollandjake changed the title Add support for tables wip: Add support for tables Jan 2, 2025
@hollandjake hollandjake marked this pull request as draft January 2, 2025 10:56
@hollandjake hollandjake changed the title wip: Add support for tables Add support for tables Jan 8, 2025
@hollandjake
Copy link
Contributor Author

@MatthijsReyers check out the new and improved version with dynamic row heights and heaps of new features

@hollandjake hollandjake force-pushed the table branch 2 times, most recently from 2c749b7 to c3ab70f Compare January 9, 2025 17:28
@MatthijsReyers
Copy link

MatthijsReyers commented Jan 10, 2025

@MatthijsReyers check out the new and improved version with dynamic row heights and heaps of new features

Exciting! I will try to have a look at it this weekend

@hollandjake
Copy link
Contributor Author

@MatthijsReyers just rebased onto the dynamic sizing PR #1576, note that during my extended testing I found a bug in the LineWrapper where some multi-line text would be cut off, this is fixed in another PR #1583

@hollandjake hollandjake marked this pull request as ready for review January 11, 2025 11:14
@hollandjake hollandjake marked this pull request as draft January 11, 2025 11:17
@hollandjake hollandjake force-pushed the table branch 2 times, most recently from a77f1db to 7f41aa3 Compare January 13, 2025 12:09
@hollandjake hollandjake marked this pull request as ready for review January 13, 2025 12:11
@hollandjake
Copy link
Contributor Author

@liborm85 do you want me to generate the guide.pdf and commit it or do you do that as part of the release?

@liborm85
Copy link
Collaborator

@hollandjake Not necessary, it will be done as part of the release.

@hollandjake hollandjake force-pushed the table branch 2 times, most recently from 962186a to 8be401c Compare January 15, 2025 11:21
@hollandjake
Copy link
Contributor Author

Im moving this back into draft until the text rotation PR #1589 is merged as it makes sense to include that as a supported feature of tables (since positioning rotated text in a cell is non automatic due to alignments)

@hollandjake hollandjake marked this pull request as draft January 15, 2025 11:23
@hollandjake hollandjake marked this pull request as ready for review January 15, 2025 19:53
@hollandjake
Copy link
Contributor Author

Text rotation in a table has been added, see the test table-rotated-text for example with all the alignment permutations

@hollandjake hollandjake force-pushed the table branch 3 times, most recently from 46627e3 to f1bd0f0 Compare January 15, 2025 20:11
@ghost
Copy link

ghost commented Jan 16, 2025

Can you share an example of how to implement accessibility for a table?
image

@hollandjake
Copy link
Contributor Author

Can you share an example of how to implement accessibility for a table? image

Hi, we actually implemented accessibility support for Table, TR, (with TH and TD controlled with the cell style type ) Regarding supporting the other THead, TBody, TFoot. i'm sure it would be possible to achieve although figuring out a clean interface for this may be difficult (for now we treat all cells as being in the implicit body)

@hollandjake
Copy link
Contributor Author

Note: One existing bug which I am in the process of fixing relates to computing the height cutoff of overflowing text when rotated or not placed at the top left corner. A hopeful fix is coming ASAP

@hollandjake
Copy link
Contributor Author

A potential spinoff PR for this could be dynamic content to allow a user to for example put an image in a cell or maybe a mix of bold and non-bold text. But im locking the scope of this PR to exclude this

docs/table.md Show resolved Hide resolved
docs/table.md Outdated Show resolved Hide resolved
lib/table/index.js Outdated Show resolved Hide resolved
lib/table/render.js Show resolved Hide resolved
@blikblum
Copy link
Member

A potential spinoff PR for this could be dynamic content to allow a user to for example put an image in a cell or maybe a mix of bold and non-bold text. But im locking the scope of this PR to exclude this

I would keep as simple as possible

In fact, IMHO, such advanced layout (table etc) should be handled in a layer above because there are so many use cases / needs and many ways to accomplish. The risk of opening a can of worms of feature requests / bugs is big (see pdfmake table issues for an example)

An alternative is having an package like pdfkit-table that exports a function that receives a Document object and constructs the table

@hollandjake
Copy link
Contributor Author

A potential spinoff PR for this could be dynamic content to allow a user to for example put an image in a cell or maybe a mix of bold and non-bold text. But im locking the scope of this PR to exclude this

I would keep as simple as possible

In fact, IMHO, such advanced layout (table etc) should be handled in a layer above because there are so many use cases / needs and many ways to accomplish. The risk of opening a can of worms of feature requests / bugs is big (see pdfmake table issues for an example)

An alternative is having an package like pdfkit-table that exports a function that receives a Document object and constructs the table

@blikblum Would you prefer this PR be its own package?

@blikblum
Copy link
Member

@blikblum Would you prefer this PR be its own package?

I am in the fence.

Particularly i rarely use pdfkit directly. I prefer using pdfmake and have plans to eventually use react-pdf (without React). On the other side many users request such feature.

A focused with a narrow api and a small implementation would be doable to be added to pdfkit directly (your work fits this criteria except by the async stuff)

Lets see what other maintainers think

@liborm85
Copy link
Collaborator

Many people use plain pdfkit, and I see no reason why it couldn't support simple tables as well. Based on #29, this feature seems to be quite in demand.
I am in favor of including tables into pdfkit.

@ajaykuma1
Copy link

Can you share an example of how to implement accessibility for a table? image

Hi, we actually implemented accessibility support for Table, TR, (with TH and TD controlled with the cell style type ) Regarding supporting the other THead, TBody, TFoot. i'm sure it would be possible to achieve although figuring out a clean interface for this may be difficult (for now we treat all cells as being in the implicit body)

I have encountered an issue while testing a PDF table-accessible.pdf generated using the below code:

const fs = require('fs');
const PDFDocument = require('pdfkit');

// Create a new PDFDocument
var doc = new PDFDocument({
  pdfVersion: '1.5',
  lang: 'en-US',
  tagged: true,
  displayTitle: true
});

doc.pipe(fs.createWriteStream('table-accessible.pdf'));
doc.info['Title'] = 'Test Document';
doc.info['Author'] = 'Author Name';
var struct = doc.struct('Document');
doc.addStructure(struct);

struct.add(doc.struct('Table', () => {
  doc.table({
    data: [
      ['Column 1', 'Column 2', 'Column 3'],
      ['Cell 1', 'Cell 2', 'Cell 3'],
      ['Cell 4', 'Cell 5', 'Cell 6'],
      ['Cell 7', 'Cell 8', 'Cell 9']
    ]
  })
}));

doc.end();

When I test the generated PDF with PAC 2014 - PDF Accessibility Checker, it throws an error
image

@hollandjake
Copy link
Contributor Author

hollandjake commented Jan 30, 2025

@ajaykuma1 Nice catch, I've gone ahead and added full support for accessibility along with some nice to have accessible configurations too. the working example from your code is now the following:

const fs = require('fs');
const PDFDocument = require('pdfkit');

// Create a new PDFDocument
var doc = new PDFDocument({
  pdfVersion: '1.5',
  lang: 'en-US',
  tagged: true,
  displayTitle: true,
  compress: false,
});

doc.pipe(fs.createWriteStream('table-accessible.pdf'));

var struct = doc.struct('Document');
doc.addStructure(struct);
doc.table({
  rowStyles: [{type: "TH", scope: "Column"}], // Mark the first row as headers with the column scope
  data: [
    ['Column 1', 'Column 2', 'Column 3'],
    ['Cell 1', 'Cell 2', 'Cell 3'],
    ['Cell 4', 'Cell 5', 'Cell 6'],
    ['Cell 7', 'Cell 8', 'Cell 9'],
  ],
  structParent: struct,
})

doc.end();

NOTE: There is currently an issue on the accessibility front such that colors don't seem to be getting handled correctly according to the spec

Operator 'CS' not allowed in this current state.

I'm not 100% sure on why exactly this is an issue as every PDF parser ive seen is fine with it except PAC. but if I comment out the color space code in the package, PAC is fully happy. If this is still an issue for you, please raise it as another issue as I dont' believe this is related to the table changes

- Added page.contentWidth
- Added page.contentHeight
- Tables support cell customization (including colors)
- Tables also support rotatable text (with alignment support)
- code generation now respects the current document positioning to allow use of page dependent operations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tables
5 participants