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

Bad page rendering: margins are off the page #1170

Closed
habamax opened this issue Jul 22, 2019 · 40 comments
Closed

Bad page rendering: margins are off the page #1170

habamax opened this issue Jul 22, 2019 · 40 comments
Assignees
Labels
Milestone

Comments

@habamax
Copy link
Contributor

habamax commented Jul 22, 2019

asciidoctor-pdf version: current master

image

image

test theme and test doc is in archive
testdoc.zip

result pdf with mytest theme
testpage.pdf

With the default built in theme everything is OK though.

If I do minor changes to the test document -- margins are either ok or slighly wrong -- I couldn't figure out exactly what is the root cause of the issue.

@habamax
Copy link
Contributor Author

habamax commented Jul 22, 2019

I have just checked with 4642974 -- it has no such a problem:

image

@habamax
Copy link
Contributor Author

habamax commented Jul 22, 2019

And 1390e35 which was introduce by me :) causes the issue.

But the changes there was only about default table alignment...

@habamax
Copy link
Contributor Author

habamax commented Jul 22, 2019

So,

table:
  align: center

makes the test document crazy (as well as aligh: right)

@mojavelinux
Copy link
Member

I tried the files you sent me and I don't get the result that you get. Are you sure you are synced with master?

@mojavelinux
Copy link
Member

makes the test document crazy (as well as aligh: right)

That tells me something is wrong with your master branch, because that is extremely unlikely.

@habamax
Copy link
Contributor Author

habamax commented Jul 22, 2019

Yes, I have just double checked -- git pull upstream master

With the mytheme I got this exact result when table_align: center

@habamax
Copy link
Contributor Author

habamax commented Jul 22, 2019

So I change the table_align from center to left and document renders OK.

@mojavelinux
Copy link
Member

mojavelinux commented Jul 22, 2019 via email

@habamax
Copy link
Contributor Author

habamax commented Jul 22, 2019

It looks like the issue somehow connected to the fonts...

font:
  catalog:
    Main:
      normal: NotoSerif-Regular.ttf
      bold: NotoSerif-Bold.ttf
      italic: NotoSerif-RegularItalic.ttf
      bold_italic: NotoSerif-BoldItalic.ttf
    Mono:
      normal: mplus-1mn-regular.ttf
      bold: mplus-1mn-bold.ttf
      italic: mplus-1mn-regular.ttf
      bold_italic: mplus-1mn-regular.ttf
    Heading:
      normal: OpenSans-Regular.ttf
      bold: OpenSans-Bold.ttf
      italic: OpenSans-Italic.ttf
      bold_italic: OpenSans-BoldItalic.ttf
    Literal:
      normal: mplus-1mn-regular.ttf
      bold: mplus-1mn-bold.ttf
      italic: mplus-1mn-regular.ttf
      bold_italic: mplus-1mn-regular.ttf
    Caption:
      normal: OpenSans-Regular.ttf
      bold: OpenSans-Bold.ttf
      italic: OpenSans-Italic.ttf
      bold_italic: OpenSans-BoldItalic.ttf

If I change noto serifs to open sans for Main section then rendering is OK.

@habamax
Copy link
Contributor Author

habamax commented Jul 22, 2019

I would clone the repo again and start with a clean workspace. Something got misconfigured.

Just did it -- the same behaviour.

@habamax
Copy link
Contributor Author

habamax commented Jul 22, 2019

And I have just tried bundled notoserif...subset font -- the same issue

@habamax habamax closed this as completed Jul 22, 2019
@habamax
Copy link
Contributor Author

habamax commented Jul 22, 2019

accidently closed, reopening

@habamax habamax reopened this Jul 22, 2019
@habamax
Copy link
Contributor Author

habamax commented Jul 22, 2019

And how do you make a document? Because without -r asciidoctor-diagram it works ok no matter of table alignment.

@habamax
Copy link
Contributor Author

habamax commented Jul 22, 2019

Another bundle with all fonts included:
asciidoctor-test.zip

I have just tried 2 commands:

  1. ruby C:/Users/maksim.kim/projects/asciidoctor-pdf/bin/asciidoctor-pdf -a pdf-stylesdir="C:/Users/maksim.kim/projects/asciidoctor-test/" -a pdf-fontsdir="C:/Users/maksim.kim/projects/asciidoctor-test/fonts/" testpage.adoc -- no issues

  2. ruby C:/Users/maksim.kim/projects/asciidoctor-pdf/bin/asciidoctor-pdf -r asciidoctor-diagram -a pdf-stylesdir="C:/Users/maksim.kim/projects/asciidoctor-test/" -a pdf-fontsdir="C:/Users/maksim.kim/projects/asciidoctor-test/fonts/" testpage.adoc -- with issues

@habamax
Copy link
Contributor Author

habamax commented Jul 22, 2019

This weirdly strange combination of table alignment, diagrams and main font -- if I either change font to smth other than NotoSerif, or remove some tables from the document or will not include diagram rendering -- then document renders OK.

@habamax
Copy link
Contributor Author

habamax commented Jul 22, 2019

BTW, if you open attached pdf from the original post and scroll it down from page 1 to the latest one you will see how left page margin changes.

@habamax
Copy link
Contributor Author

habamax commented Jul 22, 2019

So I am pretty much sure this is reproducible:
asciidoctor-test.zip

Inside the zip there is run.bat file where the path to asciidoctor should be changed. It should generate testpage.pdf with the issue.

@mojavelinux
Copy link
Member

I was going to ask for a reproducible project. You beat me too it. In the meantime, if you could manage to reproduce it in a test case, that would be the quickest way to a solution.

This strikes me as a bad interaction between the font library (ttfunk) and prawn-table. I can't really think off the top of my head how setting an alignment would mess up the margins otherwise. That's very bizarre.

@mojavelinux
Copy link
Member

I've been able to reproduce the problem from your sample project. I'll see if I can determine the cause.

@mojavelinux
Copy link
Member

Remember that the table of contents is rendered last. So what affects the last page also affects the table of contents. That's why the sections in between look fine.

@mojavelinux
Copy link
Member

This is an extremely difficult behavior to track down. Almost any change to the document fixes it, so I can't manage to isolate it. It could be prawn-table. It could be prawn-svg. Or it could be something else.

It doesn't have to do with the table alignment in the theme. If you set align=table on all tables (in fact, just a few), you can observe the same outcome as setting table_align in the theme. That's what leads me to believe it's prawn-table.

@habamax
Copy link
Contributor Author

habamax commented Jul 24, 2019

This is an extremely difficult behavior to track down. Almost any change to the document fixes it, so I can't manage to isolate it. It could be prawn-table. It could be prawn-svg. Or it could be something else.

I've spent 3+ hours trying to narrow it down to the smallest document that ends up with this issue. Indeed, even the minor change to the simple text could make document OK again.

It doesn't have to do with the table alignment in the theme. If you set align=table on all tables (in fact, just a few), you can observe the same outcome as setting table_align in the theme. That's what leads me to believe it's prawn-table.

But if you will remove one of the diagrams -- document is OK again! I have also tried to insert another table instead of that removed diagram in case it is somehow connected to the length of the doc -- issue disappears then.

Anyway, good things:

  1. you were able to reproduce it :)
  2. with table_align: left there is no issue (I can continue with my document)

Bad things:

  1. I can't make simpler document (less than I have already provided)
  2. Still not sure how to identify the root cause

@mojavelinux
Copy link
Member

But if you will remove one of the diagrams -- document is OK again!

Agreed. Though I think that may have more to do with the y-offset changing than the diagram itself. I've been able to reproduce with no diagrams in the document.

@mojavelinux
Copy link
Member

with table_align: left there is no issue (I can continue with my document)

In fact, I can reproduce this with beta.1 by setting align=center on every table. So this issue is not new. It's just easier to trigger now.

@mojavelinux
Copy link
Member

Btw, I can tell you that when it happens, the "x" on the bounding box goes negative. So we know why the text starts getting written off the page. We just don't know why the bounding box is being changed, or where.

@habamax
Copy link
Contributor Author

habamax commented Jul 24, 2019

with table_align: left there is no issue (I can continue with my document)

In fact, I can reproduce this with beta.1 by setting align=center on every table. So this issue is not new. It's just easier to trigger now.

"there is no issue" was about "me have workaround" not that there is no issue at all :)

@habamax
Copy link
Contributor Author

habamax commented Jul 24, 2019

Btw, I can tell you that when it happens, the "x" on the bounding box goes negative. So we know why the text starts getting written off the page. We just don't know why the bounding box is being changed, or where.

And this is visible if you scroll the document downwards -- you will see that the left margin becomes less and less

@mojavelinux
Copy link
Member

So I think this happens (but only in certain cases) when a table with center or right alignment gets split across pages. There must be a bug in prawn-table in how it handles the bounds that affects the page.

A simple solution to this problem is to record the bounding box before the table and restore it afterwards. That way, whatever crazy thing prawn-table does, we just undo it ;)

@mojavelinux
Copy link
Member

mojavelinux commented Jul 24, 2019

@habamax Yep, I know what you meant. I'm just very amazed that this also existed in beta.1 and we just didn't see it ;) But clearly it requires just the right document to reproduce.

I've gotten the sample document down to about 15 pages.

@mojavelinux
Copy link
Member

Good news! I can isolate at least one page in the 15 page document where I can turn the problem on and off. That should be enough for me to see what is different about Prawn's internal state at that point. All we need to do is figure out what state to reset and then we can at least fix the problem on our end.

@mojavelinux
Copy link
Member

mojavelinux commented Jul 24, 2019

I finally found a workaround.

The problem is caused by a bad interaction between a bounding box (for drawing) and the new page creation logic. Frankly, it's a bit of a mess in Prawn and not easily fixable. Suffice to say, it's not good to use a bounding box to restrict the width. It's better to use margins (i.e., indent), which sidesteps the issue entirely. I had found this out when doing the TOC, but forgot about that rule.

Therefore, I will apply a patch to Prawn table that will use margin indenting instead of the bounding box to position the table.

@mojavelinux
Copy link
Member

It's also unnecessary to use the bounding box (or margin) when the table has a width that matches the width of the page. So this issue shouldn't even come up unless you use autowidth (and the natural width is less than 100%) or an explicit width less than 100%.

@mojavelinux
Copy link
Member

A fix is on the way.

@mojavelinux
Copy link
Member

mojavelinux commented Jul 24, 2019

So I was all boastful that we had plenty of tests that this couldn't be a bug. Well, as it turns out, it was a bug that the tests missed. 🤯 But we've got a new test ready that will surely keep this bug out of our 🏠 now.

@habamax
Copy link
Contributor Author

habamax commented Jul 24, 2019

So I was all boastful that we had plenty of tests that this couldn't be a bug. Well, as it turns out, it was a bug that the tests missed. 🤯 But we've got a new test ready that will surely keep this bug out of our 🏠 now.

Now I know a new word "boastful" :)

I am happy you found out the root cause and come up with solution, can't wait to test it.

PS
I tried really hard to track down the issue after your words for "bounding_box" and "x" but the ruby codebase is still too alien for me. Hope next time I will be more lucky :)

mojavelinux added a commit to mojavelinux/asciidoctor-pdf that referenced this issue Jul 25, 2019
@mojavelinux mojavelinux self-assigned this Jul 25, 2019
@mojavelinux mojavelinux added this to the v1.5.0.beta.2 milestone Jul 25, 2019
@mojavelinux
Copy link
Member

I am happy you found out the root cause and come up with solution, can't wait to test it.

PR sent. See #1172.

I tried really hard to track down the issue after your words for "bounding_box" and "x" but the ruby codebase is still too alien for me. Hope next time I will be more lucky

Don't worry. The behavior of bounding_box at page boundaries still even confuses me. I honestly don't understand why Prawn can't handle that case correctly. But at least we know a perfectly good alternative that isn't affected by that problem.

habamax pushed a commit to habamax/asciidoctor-pdf that referenced this issue Jul 25, 2019
@habamax
Copy link
Contributor Author

habamax commented Jul 25, 2019

Just tested it out -- looks good for me

@mojavelinux
Copy link
Member

I now understand the root cause. We've already worked around it, but I want to document it here so the information isn't lost.

If a bounding box is used around content that gets split across pages (which is what prawn-table does when position is set to center or right), and indent is used around subsequent content that also gets split across page, Prawn subtracts the indentation twice coming out of that function. That's why the margin kept getting smaller and smaller. So it's some sort of bad interaction between the bounding_box and indent functions.

@mojavelinux
Copy link
Member

Yep, just confirmed it. bounding_box and indent just don't play together. There seems to be a flaw in Prawn's design here. It restores the padding to the wrong box, so when it removes the padding it goes negative.

@mojavelinux
Copy link
Member

Here's the upstream issue. It doesn't affect us since we took another path, but I figured it was worth submitting the fix while it was fresh. prawnpdf/prawn-table#114

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants