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 page numbering using the paged media polyfill library pagedjs #11

Merged
merged 2 commits into from
Jan 24, 2019

Conversation

ggrossetie
Copy link
Owner

@ggrossetie ggrossetie commented Nov 18, 2018

TODO

  • Fix the issues with multi-pages table (ie. a table spread across multiple pages).
  • It's currently mandatory to serve the HTML page through an HTTP serve (in this prototype branch, we are using serve with the default port 5000). We need to find a workaround or to automate the process so the user won't have to manually run a command.
  • Repeat the table header on each page (upstream issue: https://gitlab.pagedmedia.org/tools/pagedjs/issues/84)

Paged media spec: https://www.w3.org/TR/CSS2/page.html

@ggrossetie
Copy link
Owner Author

I'm using a workaround suggested by @mojavelinux:

tr {
  page-break-inside: avoid;
}

@mojavelinux
Copy link

This rule seem pretty safe to me. If you have a row that's taller than a page, then you have a content problem ;)

@mojavelinux
Copy link

We could consider making the rule more precise so it only affects tables generated by Asciidoctor. But that's up to you.

@ggrossetie
Copy link
Owner Author

I think it's fine, it's a good default to avoid a break inside a table row 😄

@ggrossetie
Copy link
Owner Author

Even without the table header on each, the result is quite good. What do you think @mojavelinux ? Do you see something wrong ? @oncletom and I tried to get as close as possible to the default theme of Asciidoctor PDF.

@mojavelinux
Copy link

Even without the table header on each

That is certainly an important feature, but one that took me a second look to even notice. I guess that's the one other part of the paged media spec that the browsers actually support by default.

@mojavelinux
Copy link

I tried to get as close as possible to the default theme of Asciidoctor PDF.

While emulating Asciidoctor PDF is a nice goal, we shouldn't get too fixated on that theme. It has good parts and bad parts. Perhaps we can have at least one example named "asciidoctor-pdf-default" to emulates it, but the other examples should focus on showing what else is possible.

@mojavelinux
Copy link

It's currently mandatory to serve the HTML page through an HTTP serve

What's the reason for this? When I tried it, I was able to use a file:// URI. Does doing it that way leave something disabled?

@ggrossetie
Copy link
Owner Author

What's the reason for this? When I tried it, I was able to use a file:// URI. Does doing it that way leave something disabled?

I don't remember exactly but I think it was a cross-domain issue with file:// in Headless Chrome. We tried to run Chrome with --allow-file-access-from-files --disable-web-security but without success.
I need to give it a try one more time maybe we missed something.

@thom4parisot
Copy link
Collaborator

thom4parisot commented Nov 27, 2018

It's because paged.js uses fetch() to retrieve the stylesheets content, to parse it and to produce a paginated version of it. By default Chrome prevents fetching local files — it would make it easy for a remote script to pump local files.

@ggrossetie
Copy link
Owner Author

ggrossetie commented Nov 27, 2018

The main issue is that fetch implementation in Chrome does not support fetching local files: JakeChampion/fetch#92 (comment)

So even if we are explicitly disabling CORS, we still get an error:

fetch('file:///path/to/asciidoctor.css', {mode: 'no-cors'})
Fetch API cannot load file:///path/to/asciidoctor.css. URL scheme "file" is not supported.

If I replace fetch with XMLHttpRequest then it's working:

async fetchLocal(url) {
  return new Promise(function(resolve, reject) {
    var xhr = new XMLHttpRequest
    xhr.onload = function() {
      resolve(new Response(xhr.responseText, {status: xhr.status}))
    }
    xhr.onerror = function() {
      reject(new TypeError('Local request failed'))
    }
    xhr.open('GET', url)
    xhr.send(null)
  })
}

I don't know if Paged.js will consider to replace fetch by XMLHttpRequest ?
But I'm wondering how is it working on @mojavelinux browser ?

@mojavelinux
Copy link

It's because paged.js uses fetch() to retrieve the stylesheets content, to parse it and to produce a paginated version of it.

Got it. That makes sense.

But I'm wondering how is it working on @mojavelinux browser ?

It could be that I was seeing different results than you, so functionality was simply being disabled without me realizing it.

@mojavelinux
Copy link

Is it possible to hot patch paged.js so that we can use the distribution, but still replace it with our own fetchLocal? Perhaps paged.js is willing to make this an extension point (if that's not already possible).

@ggrossetie
Copy link
Owner Author

Is it possible to hot patch paged.js so that we can use the distribution, but still replace it with our own fetchLocal? Perhaps paged.js is willing to make this an extension point (if that's not already possible).

It's now fixed! https://gitlab.pagedmedia.org/tools/pagedjs/issues/86

@ggrossetie
Copy link
Owner Author

Synk is unhappy 😞
I've opened a few pull requests to address the remaining issues:

https://gitlab.pagedmedia.org/tools/pagedjs/merge_requests/44
https://gitlab.pagedmedia.org/tools/pagedjs/merge_requests/43

Paged.js will transform an HTML5 page into a set of printable pages.
Margin, footer and header can be configured on each pages.
We do not use npm to install pagedjs because currently it contains several critical vulnerabilities.
See: https://gitlab.pagedmedia.org/tools/pagedjs/merge_requests/44
@ggrossetie
Copy link
Owner Author

Repeat the table header on each page (upstream issue: https://gitlab.pagedmedia.org/tools/pagedjs/issues/84)

The above task is not critical.
I will move forward with this change as it improves the final output.

@ggrossetie ggrossetie merged commit 40cd54d into master Jan 24, 2019
ggrossetie pushed a commit that referenced this pull request Oct 28, 2019
@ggrossetie ggrossetie deleted the feat/pagedjs branch November 3, 2019 19:05
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.

3 participants