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.

Sign up for GitHub

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

Jump to bottom

Added test & patch for floating images page break #183

Merged
merged 4 commits into from
Apr 27, 2014
Merged

Added test & patch for floating images page break #183

merged 4 commits into from
Apr 27, 2014

Conversation

elpaso
Copy link
Contributor

@elpaso elpaso commented Apr 17, 2014

This patch fixes the floating image problem when they overflow at the bottom of the page.
It doesn't seem to break any other test.

@SimonSapin
Copy link
Member

Hi, thanks for contributing! This seems like a good idea, I hadn’t thought about it. Before this merged, please fix a few issues:

  • This code goes into an infinite loop if a float is bigger than the page, e.g. <img src=pattern.png style="height: 81px"> in your test. The code keeps making new pages, none of which is high enough. In other parts of the code, we avoid this issue by making sure that we make at least some progress in the document for each page, letting things overflow the page in the worst case. This is tracked with the page_is_empty variable. Please add a test case that reproduces this and fix the issue.
  • What does "ABP" mean in your code comment? Please expand the comment.
  • You create a new test_layout_images.py file, but this change is really about floats more than images: it applies just as well to a floated <div>. Please add the test in the existing test_layout.py file. Or, if you find it really too big (it is big, but that hasn’t really been a problem so far), start with a separate commit to move existing tests and split the file, preferably keeping kinda related tests together, and update each module’s docstring as appropriate.
  • Please make sure that no line (including empty lines) has trailing whitespace, that you have two blank lines between imports and a function or between two functions.
  • Add your name to the AUTHORS file :)

@elpaso
Copy link
Contributor Author

elpaso commented Apr 27, 2014

Hi,

I've added a test case for floats taller than the page and a check for page_is_empty / new_children

@SimonSapin
Copy link
Member

Looks good, thanks!

(There is still some trailing whitespace, but I’ll fix that.)

SimonSapin added a commit that referenced this pull request Apr 27, 2014
@SimonSapin
Merge pull request #183 from elpaso/master
45a648e 
Added test & patch for floating images page break
@SimonSapin SimonSapin merged commit 45a648e into Kozea:master Apr 27, 2014
@jjsarton
Copy link

I have a test page which contain a parapgraph with a floating image. With weasyprint v 0.23 the picture is printed partially outside of the printable area. The page-break what not OK
The html code for testing is here:
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8"/>
<title>Float Test</title>
<style type="text/css">
document { font-size: 16px; font-family: serif; line-height: 24px;}
h1 p { margin-top: 0; margin-bottom: 24px; }
h1, h2 { font-size: 1em; font-weight: bold;}
img { float: right; width: 15em; height: 15em;}
.dummyp { height: 45em;}
h2 { page-break-after: avoid;}
img { page-break-inside: avoid; border: solid 1px #00f;
vertical-align: top; }
.clear { clear: both;}
@media screen {
body {
width: 17cm;
margin: 0 auto;
}
}
@media print {
@page {
size: A4 portrait;
margin: 20mm;
padding-top: 1em;
padding-bottom: 1em;
border-top:solid .3pt black;
border-bottom:solid .3pt black;
@bottom-left {
content: 'Testcase float';
font-size: 12pt;
vertical-align: top;
}
@bottom-right {
content: "Page " counter(page) " / " counter(pages);
font-size: 12pt;
vertical-align: top;
}
}
}
</style>
</head>
<body>
<h1>Test for floating</h1>
<p class="dummyp">Paragraph...</p>
<h2>Floating test follow</h2>
<p>Paragraph wit floating img<img srx="grey.jpg" alt="picture" />
<p>An other paragraph</p>
<div class="clear"></div>
</body>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@elpaso @SimonSapin @jjsarton