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] position: running(); content: element(); #882

Merged
merged 2 commits into from Sep 27, 2019
Merged

Conversation

@hbrunn
Copy link
Contributor

hbrunn commented Jun 17, 2019

@liZe this is a result of our discussion in OCA/reporting-engine#254

This doesn't work yet, but I've a few questions so I figure it's a good idea to open this draft PR in order to talk about code

weasyprint/css/utils.py Outdated Show resolved Hide resolved
@@ -209,6 +209,9 @@ def __init__(self, enable_hinting, style_for, get_image_from_uri,
self.tables = {}
self.dictionaries = {}

# TODO: this is probably the wrong place to put this
self.running_elements = {}

This comment has been minimized.

Copy link
@hbrunn

hbrunn Jun 17, 2019

Author Contributor

we'll have to carry around the running elements somewhere. Do you think this belongs here or should I extend the state tuple?

This comment has been minimized.

Copy link
@liZe

liZe Jul 4, 2019

Member

I think that the specification isn't clear enough. It's hard to know which running element you'll have in the page margin when multiple running elements are in the page. The string() function at least has a second argument for that.

Even a sigle value, in example 5 for example, is not clear to me: what's on the second page?

  • We could put '2 / Miranda v. Arizona in Context'.
  • We could put '1 / Miranda v. Arizona in Context', as the running element is defined on page 1.
  • We could put nothing, as no running element is defined on page 2.
    (I think that we want the first answer.)

The place where to put running elements depends on the answer to this question. But if we think that running elements somehow behave like named strings, then it's the right place to set

This comment has been minimized.

Copy link
@hbrunn

hbrunn Jul 13, 2019

Author Contributor

If it's not the first answer I wouldn't want to implement it because then it wouldn't be useful for me :-)
I define a box now that does nothing, save for setting the running element for the page. This way, the last running() on the page wins, that's how I interpret the spec.

The not-doing-anything-box seems clumsy to me, can this be more elegant?

This comment has been minimized.

Copy link
@liZe

liZe Sep 27, 2019

Member

This way, the last running() on the page wins, that's how I interpret the spec.

There's a second optional parameter for element() to define this. We have to use the same behavior as string().

@hbrunn

This comment has been minimized.

Copy link
Contributor Author

hbrunn commented Jun 17, 2019

generally, my problem is that in https://github.com/Kozea/WeasyPrint/blob/master/weasyprint/layout/pages.py#L348-L349, we assume inline boxes, which crashes in https://github.com/Kozea/WeasyPrint/blob/master/weasyprint/formatting_structure/build.py#L1247. What would be the smartest way to change this? (I just started studying this library, so I'm not yet well versed with available helper functions)

@liZe

This comment has been minimized.

Copy link
Member

liZe commented Jul 4, 2019

generally, my problem is that in https://github.com/Kozea/WeasyPrint/blob/master/weasyprint/layout/pages.py#L348-L349, we assume inline boxes, which crashes in https://github.com/Kozea/WeasyPrint/blob/master/weasyprint/formatting_structure/build.py#L1247. What would be the smartest way to change this? (I just started studying this library, so I'm not yet well versed with available helper functions)

For now, you can call the functions that are in build_formatting_structure, as we now may have blocks:

# If this is changed, maybe update weasy.layout.pages.make_margin_boxes()
process_whitespace(box)
box = anonymous_table_boxes(box)
box = flex_boxes(box)
box = inline_in_block(box)
box = block_in_inline(box)
box = set_viewport_overflow(box)

We may want to create a function for this later.

@hbrunn

This comment has been minimized.

Copy link
Contributor Author

hbrunn commented Jul 13, 2019

@liZe thanks for the pointers. now this PR actually works, and I just need to wrap it up. last question: I need to have get_content_list called on the running box for every time it's rendered, how do you propose to do that?

@hbrunn hbrunn force-pushed the hbrunn:master-running branch from 4e1b035 to af6042d Sep 22, 2019
@hbrunn hbrunn marked this pull request as ready for review Sep 22, 2019
@hbrunn hbrunn force-pushed the hbrunn:master-running branch from af6042d to 0acdbaa Sep 22, 2019
@hbrunn

This comment has been minimized.

Copy link
Contributor Author

hbrunn commented Sep 22, 2019

@liZe this is ready for review now

@hbrunn hbrunn force-pushed the hbrunn:master-running branch from 0acdbaa to b30ffb1 Sep 22, 2019
@liZe

This comment has been minimized.

Copy link
Member

liZe commented Sep 23, 2019

@liZe this is ready for review now

OK, thanks! I'll look at it.

@hbrunn

This comment has been minimized.

Copy link
Contributor Author

hbrunn commented Sep 23, 2019

great. I just noticed I failed to remove the note about running elements in the docs, just pushed a fixup commit for that.
And I noticed late yesterday that content: element(...) elements aren't always laid out the way I'd expect it (not take the full width of the page), but that might also be a wrong expectation. We'll see about that during review I think.

Here the file I mostly used for testing:

<html>
    <head>
        <style type="text/css">
            .header {
                position: running(header);
            }
            .footer {
                position: running(footer);
                text-align: center;
            }
            @page {
                @top-center {
                    content: element(header);
                }
                @top-right {
                    content: string(header_string);
                }
                @bottom-center {
                    content: element(footer);
                    /* without this, there's a line break before the pages
                       span
                    */
                    width: 100%;
                }
                @bottom-right {
                    content: counter(page);
                }
            }
            h1 {
                margin-bottom: 15cm;
                string-set: header_string content();
            }
            .page:before {
                content: counter(page);
            }
            .pages:before {
                content: counter(pages);
            }
            .header_string:before {
                content: string(header_string);
            }
        </style>
    </head>
    <body>
        <div class="header">
            Hello world file: <span class="header_string" />
        </div>
        <div class="footer">
            <span class="page" /> of <span class="pages" />
        </div>
        <h1>test1</h1>
        <h1>test2</h1>
        <h1>test3</h1>
        <h1>test4</h1>
        <h1>test5</h1>
        <h1>test6</h1>
        <h1>test7</h1>
        <h1>test8</h1>
        <h1>test9</h1>
        <h1>test10</h1>
        <h1>test11</h1>
        <h1>test12</h1>
        <div class="header">
            Hello world2
        </div>
        <h1>test13</h1>
    </body>
</html>

which then yields test.pdf

@liZe

This comment has been minimized.

Copy link
Member

liZe commented Sep 27, 2019

Thanks a lot 👏. The global way it works seems to be OK for me.

I can merge this PR, but I'll change some little things:

  • We should use ('running()', name) instead of a dedicated class, because that's how it's done for other functions. One day we'll use real objects for everything I suppose…
  • __deepcopy__ should be a normal method.
  • The inline_in_block and block_in_inline should work out of the box, there's probably something wrong elsewhere if we have to add or box.is_running().
  • The RunningPlaceholder probably has to be removed. Having a placeholder inheriting from BlockBox with random used values set is a huge lie that will bring much more harm in the future than the problems it solves right now 😄.

I'd expect it (not take the full width of the page), but that might also be a wrong expectation. We'll see about that during review I think.

I have to dive into the code by fixing the points listed above, I'll check this problem afterwards.

@liZe liZe merged commit fd70058 into Kozea:master Sep 27, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@liZe liZe added this to the 51 milestone Sep 27, 2019
@hbrunn

This comment has been minimized.

Copy link
Contributor Author

hbrunn commented Sep 27, 2019

thanks! If you give me a few pointers I'll be happy to investigate myself, integrating weasyprint into odoo is my current pet project :-)

To my understanding we will need some way to know where some box would be in the flow in order to use the correct element in the page margins, that's why I came up with the placeholder. Is your critique specifically that I made this a BlockBox and I should be using a more generic class higher up in the inheritance chain as base, or do you disagree with the placeholder altogether?

If you can't answer without doing the code dive yourself, I'll wait and check out the diff.

@liZe

This comment has been minimized.

Copy link
Member

liZe commented Sep 27, 2019

integrating weasyprint into odoo is my current pet project :-)

❤️

thanks! If you give me a few pointers I'll be happy to investigate myself

I've made some commits already, but I'll let you some work if you prefer, no problem 😉.

To my understanding we will need some way to know where some box would be in the flow in order to use the correct element in the page margins, that's why I came up with the placeholder. Is your critique specifically that I made this a BlockBox and I should be using a more generic class higher up in the inheritance chain as base, or do you disagree with the placeholder altogether?

The problem is that the box is removed from the page, we don't need to keep a placeholder to render it at the place it should have been, as we do for absolute boxes for example. When rendered in the margins, it's totally unrelated to its place in the page content (I hope!), unlike absolute boxes. You can see in 0a403d9 that everything works well without it.

@JanPretzel

This comment has been minimized.

Copy link

JanPretzel commented Oct 11, 2019

I just tried this feature and it is awesome! But I encountered a problem, when I add a table in the header of the example from @hbrunn I get an error.

Sample header with table:

<div class="header">
        <table><tr><td>Hello World</td></tr></table>
</div>

Error:

File "/usr/lib/python3.7/site-packages/weasyprint/__init__.py", line 168, in render
    font_config)
  File "/usr/lib/python3.7/site-packages/weasyprint/document.py", line 393, in _render
    [Page(page_box, enable_hinting) for page_box in page_boxes],
  File "/usr/lib/python3.7/site-packages/weasyprint/document.py", line 393, in <listcomp>
    [Page(page_box, enable_hinting) for page_box in page_boxes],
  File "/usr/lib/python3.7/site-packages/weasyprint/layout/__init__.py", line 183, in layout_document
    make_margin_boxes(context, page, state))
  File "/usr/lib/python3.7/site-packages/weasyprint/layout/pages.py", line 435, in make_margin_boxes
    yield margin_box_content_layout(context, page, box)
  File "/usr/lib/python3.7/site-packages/weasyprint/layout/pages.py", line 443, in margin_box_content_layout
    page_is_empty=True, absolute_boxes=[], fixed_boxes=[])
  File "/usr/lib/python3.7/site-packages/weasyprint/layout/blocks.py", line 507, in block_container_layout
    absolute_boxes, fixed_boxes, adjoining_margins)
  File "/usr/lib/python3.7/site-packages/weasyprint/layout/blocks.py", line 63, in block_level_layout
    page_is_empty, absolute_boxes, fixed_boxes, adjoining_margins)
  File "/usr/lib/python3.7/site-packages/weasyprint/layout/blocks.py", line 77, in block_level_layout_switch
    page_is_empty, absolute_boxes, fixed_boxes, adjoining_margins)
  File "/usr/lib/python3.7/site-packages/weasyprint/layout/blocks.py", line 130, in block_box_layout
    absolute_boxes, fixed_boxes, adjoining_margins)
  File "/usr/lib/python3.7/site-packages/weasyprint/layout/blocks.py", line 507, in block_container_layout
    absolute_boxes, fixed_boxes, adjoining_margins)
  File "/usr/lib/python3.7/site-packages/weasyprint/layout/blocks.py", line 63, in block_level_layout
    page_is_empty, absolute_boxes, fixed_boxes, adjoining_margins)
  File "/usr/lib/python3.7/site-packages/weasyprint/layout/blocks.py", line 77, in block_level_layout_switch
    page_is_empty, absolute_boxes, fixed_boxes, adjoining_margins)
  File "/usr/lib/python3.7/site-packages/weasyprint/layout/blocks.py", line 124, in block_box_layout
    context, box, (containing_block.width, containing_block.height))
  File "/usr/lib/python3.7/site-packages/weasyprint/layout/tables.py", line 704, in table_wrapper_width
    table = wrapper.get_wrapped_table()
  File "/usr/lib/python3.7/site-packages/weasyprint/formatting_structure/boxes.py", line 368, in get_wrapped_table
    raise ValueError('Table wrapper without a table')
ValueError: Table wrapper without a table

Any idea why this is happening? I am not quite sure if this is a bug or if it is expected behavior (I am quite new to this topic).

@liZe

This comment has been minimized.

Copy link
Member

liZe commented Oct 11, 2019

Hello @JanPretzel

I just tried this feature and it is awesome! But I encountered a problem, when I add a table in the header of the example from @hbrunn I get an error.

Could you please open an issue about that? Thank you!

@rjoly

This comment has been minimized.

Copy link

rjoly commented Nov 14, 2019

Nice job!

It would be awesome if we could use this feature in a near-future deployed application. Do you think that you could give a release date for v51?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.