Repository: Kozea/WeasyPrint (python)

Issue:
Running elements not working with tables
I tried the new running elements feature which seems to be breaking when the running element contains a table.

I used the example from @hbrunn in #882 and added another header:

```html
<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>
        <div class="header">
            <table><tr><td>Hello World</td></tr></table>
        </div>
        <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>
```
This will throw following 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
```


Pull request:
Fix some corner cases for running elements
Fix #967.

This pull request is not clean at all, but it’s much better than the current `master` branch. There’s a lot to say, but I don’t have the time right now (I hope I won’t forget…).

We need some tests before merging.

Patch:
diff --git a/weasyprint/formatting_structure/build.py b/weasyprint/formatting_structure/build.py
index f7153c2c..ce653157 100644
--- a/weasyprint/formatting_structure/build.py
+++ b/weasyprint/formatting_structure/build.py
@@ -410,14 +410,14 @@ def compute_content_list(content_list, parent_box, counter_values, css_token,
                 counters.format(counter_value, counter_style)
                 for counter_value in counter_values.get(counter_name, [0])))
         elif type_ == 'string()':
-            if in_page_context:
-                texts.append(context.get_string_set_for(page, *value))
-            else:
+            if not in_page_context:
                 # string() is currently only valid in @page context
                 # See https://github.com/Kozea/WeasyPrint/issues/723
                 LOGGER.warning(
                     '"string(%s)" is only allowed in page margins' %
                     (' '.join(value)))
+                continue
+            texts.append(context.get_string_set_for(page, *value) or '')
         elif type_ == 'target-counter()':
             anchor_token, counter_name, counter_style = value
             lookup_target = target_collector.lookup_target(
@@ -494,17 +494,15 @@ def compute_content_list(content_list, parent_box, counter_values, css_token,
             if is_open:
                 quote_depth[0] += 1
         elif type_ == 'element()':
-            name, position = value
-            if name not in context.running_elements:
-                # TODO: emit warning
+            if not in_page_context:
+                LOGGER.warning(
+                    '"element(%s)" is only allowed in page margins' %
+                    (' '.join(value)))
                 continue
-            new_box = None
-            for i in range(context.current_page - 1, -1, -1):
-                if i not in context.running_elements[name]:
-                    continue
-                running_box = context.running_elements[name][i]
-                new_box = running_box.deepcopy()
-                break
+            new_box = context.get_running_element_for(page, *value)
+            if new_box is None:
+                continue
+            new_box = new_box.deepcopy()
             new_box.style['position'] = 'static'
             for child in new_box.descendants():
                 if child.style['content'] in ('normal', 'none'):
@@ -741,7 +739,7 @@ def anonymous_table_boxes(box):
     See http://www.w3.org/TR/CSS21/tables.html#anonymous-boxes
 
     """
-    if not isinstance(box, boxes.ParentBox):
+    if not isinstance(box, boxes.ParentBox) or box.is_running():
         return box
 
     # Do recursion.
@@ -1134,7 +1132,7 @@ def flex_boxes(box):
     See http://www.w3.org/TR/css-flexbox-1/#flex-items
 
     """
-    if not isinstance(box, boxes.ParentBox):
+    if not isinstance(box, boxes.ParentBox) or box.is_running():
         return box
 
     # Do recursion.
@@ -1218,7 +1216,7 @@ def process_whitespace(box, following_collapsible_space=False):
         box.text = text
         return following_collapsible_space
 
-    if isinstance(box, boxes.ParentBox):
+    if isinstance(box, boxes.ParentBox) and not box.is_running():
         for child in box.children:
             if isinstance(child, (boxes.TextBox, boxes.InlineBox)):
                 following_collapsible_space = process_whitespace(
diff --git a/weasyprint/layout/__init__.py b/weasyprint/layout/__init__.py
index 24078bfb..9aab5b89 100644
--- a/weasyprint/layout/__init__.py
+++ b/weasyprint/layout/__init__.py
@@ -196,7 +196,8 @@ class LayoutContext(object):
         self._excluded_shapes_lists = []
         self.excluded_shapes = None  # Not initialized yet
         self.string_set = defaultdict(lambda: defaultdict(lambda: list()))
-        self.running_elements = {}
+        self.running_elements = defaultdict(
+            lambda: defaultdict(lambda: list()))
         self.current_page = None
         self.forced_break = False
 
@@ -225,7 +226,17 @@ class LayoutContext(object):
             self.excluded_shapes = None
 
     def get_string_set_for(self, page, name, keyword='first'):
-        """Resolve value of string function (as set by string set).
+        """Resolve value of string function."""
+        return self.get_string_or_element_for(
+            self.string_set, page, name, keyword)
+
+    def get_running_element_for(self, page, name, keyword='first'):
+        """Resolve value of element function."""
+        return self.get_string_or_element_for(
+            self.running_elements, page, name, keyword)
+
+    def get_string_or_element_for(self, store, page, name, keyword):
+        """Resolve value of string or element function.
 
         We'll have something like this that represents all assignments on a
         given page:
@@ -236,17 +247,19 @@ class LayoutContext(object):
         Value depends on current page.
         http://dev.w3.org/csswg/css-gcpm/#funcdef-string
 
-        :param name: the name of the named string.
-        :param keyword: indicates which value of the named string to use.
-                        Default is the first assignment on the current page
-                        else the most recent assignment (entry value)
-        :returns: text
+        :param store: dictionary where the resolved value is stored.
+        :param page: current page.
+        :param name: name of the named string or running element.
+        :param keyword: indicates which value of the named string or running
+                        element to use. Default is the first assignment on the
+                        current page else the most recent assignment.
+        :returns: text for string set, box for running element
 
         """
-        if self.current_page in self.string_set[name]:
+        if self.current_page in store[name]:
             # A value was assigned on this page
-            first_string = self.string_set[name][self.current_page][0]
-            last_string = self.string_set[name][self.current_page][-1]
+            first_string = store[name][self.current_page][0]
+            last_string = store[name][self.current_page][-1]
             if keyword == 'first':
                 return first_string
             elif keyword == 'start':
@@ -263,8 +276,9 @@ class LayoutContext(object):
                     break
             elif keyword == 'last':
                 return last_string
+            elif keyword == 'first-except':
+                return
         # Search backwards through previous pages
         for previous_page in range(self.current_page - 1, 0, -1):
-            if previous_page in self.string_set[name]:
-                return self.string_set[name][previous_page][-1]
-        return ''
+            if previous_page in store[name]:
+                return store[name][previous_page][-1]
diff --git a/weasyprint/layout/blocks.py b/weasyprint/layout/blocks.py
index b6b350de..e6a4bd56 100644
--- a/weasyprint/layout/blocks.py
+++ b/weasyprint/layout/blocks.py
@@ -353,9 +353,9 @@ def block_container_layout(context, box, max_position_y, skip_stack,
                     resume_at = (index, None)
                     break
             elif child.is_running():
-                context.running_elements.setdefault(
-                    child.style['position'][1], {}
-                )[context.current_page - 1] = child
+                running_name = child.style['position'][1]
+                page = context.current_page
+                context.running_elements[running_name][page].append(child)
             continue
 
         if isinstance(child, boxes.LineBox):
diff --git a/weasyprint/layout/inlines.py b/weasyprint/layout/inlines.py
index c0ebbb55..77616a20 100644
--- a/weasyprint/layout/inlines.py
+++ b/weasyprint/layout/inlines.py
@@ -738,6 +738,11 @@ def split_inline_box(context, box, position_x, max_x, skip_stack,
                         old_child.translate(dx=dx)
             float_resume_at = index + 1
             continue
+        elif child.is_running():
+            running_name = child.style['position'][1]
+            page = context.current_page
+            context.running_elements[running_name][page].append(child)
+            continue
 
         last_child = (index == len(box.children) - 1)
         available_width = max_x


Test patch:
diff --git a/weasyprint/tests/test_layout/test_page.py b/weasyprint/tests/test_layout/test_page.py
index 249ebe7c..a57dfb16 100644
--- a/weasyprint/tests/test_layout/test_page.py
+++ b/weasyprint/tests/test_layout/test_page.py
@@ -1219,57 +1219,130 @@ def test_margin_boxes_vertical_align():
 @assert_no_logs
 def test_margin_boxes_element():
     pages = render_pages('''
-        <html>
-            <head>
-                <style type="text/css">
-                    .footer {
-                        position: running(footer);
-                    }
-                    @page {
-                        @bottom-center {
-                            content: element(footer);
-                        }
-                    }
-                    h1 {
-                        margin-bottom: 15cm;
-                    }
-                    .page:before {
-                        content: counter(page);
-                    }
-                    .pages:after {
-                        content: counter(pages);
-                    }
-                </style>
-            </head>
-            <body>
-                <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>
-                <div class="footer">
-                    Last page will be a static footer
-                </div>
-            </body>
-        </html>
+      <style>
+        footer {
+          position: running(footer);
+        }
+        @page {
+          margin: 50px;
+          size: 200px;
+          @bottom-center {
+            content: element(footer);
+          }
+        }
+        h1 {
+          height: 40px;
+        }
+        .pages:before {
+          content: counter(page);
+        }
+        .pages:after {
+          content: counter(pages);
+        }
+      </style>
+      <footer class="pages"> of </footer>
+      <h1>test1</h1>
+      <h1>test2</h1>
+      <h1>test3</h1>
+      <h1>test4</h1>
+      <h1>test5</h1>
+      <h1>test6</h1>
+      <footer>Static</footer>
     ''')
-    # first footer
     footer1_text = ''.join(
         getattr(node, 'text', '')
         for node in pages[0].children[1].descendants())
     assert footer1_text == '1 of 3'
 
-    # second footer
     footer2_text = ''.join(
         getattr(node, 'text', '')
         for node in pages[1].children[1].descendants())
     assert footer2_text == '2 of 3'
-    # last footer
+
     footer3_text = ''.join(
         getattr(node, 'text', '')
         for node in pages[2].children[1].descendants())
-    assert footer3_text == 'Last page will be a static footer'
+    assert footer3_text == 'Static'
+
+
+@assert_no_logs
+@pytest.mark.parametrize('argument, texts', (
+    # TODO: start doesn’t work because running elements are removed from the
+    # original tree, and the current implentation in
+    # layout.get_running_element_for uses the tree to know if it’s at the
+    # beginning of the page
+
+    # ('start', ('', '2-first', '2-last', '3-last', '5')),
+
+    ('first', ('', '2-first', '3-first', '3-last', '5')),
+    ('last', ('', '2-last', '3-last', '3-last', '5')),
+    ('first-except', ('', '', '', '3-last', '')),
+))
+def test_running_elements(argument, texts):
+    pages = render_pages('''
+      <style>
+        @page {
+          margin: 50px;
+          size: 200px;
+          @bottom-center { content: element(title %s) }
+        }
+        article { break-after: page }
+        h1 { position: running(title) }
+      </style>
+      <article>
+        <div>1</div>
+      </article>
+      <article>
+        <h1>2-first</h1>
+        <h1>2-last</h1>
+      </article>
+      <article>
+        <p>3</p>
+        <h1>3-first</h1>
+        <h1>3-last</h1>
+      </article>
+      <article>
+      </article>
+      <article>
+        <h1>5</h1>
+      </article>
+    ''' % argument)
+    assert len(pages) == 5
+    for page, text in zip(pages, texts):
+        html, margin = page.children
+        if margin.children:
+            h1, = margin.children
+            line, = h1.children
+            textbox, = line.children
+            assert textbox.text == text
+        else:
+            assert not text
+
+
+@assert_no_logs
+def test_running_elements_display():
+    page, = render_pages('''
+      <style>
+        @page {
+          margin: 50px;
+          size: 200px;
+          @bottom-left { content: element(inline) }
+          @bottom-center { content: element(block) }
+          @bottom-right { content: element(table) }
+        }
+        table { position: running(table) }
+        div { position: running(block) }
+        span { position: running(inline) }
+      </style>
+      text
+      <table><tr><td>table</td></tr></table>
+      <div>block</div>
+      <span>inline</span>
+    ''')
+    html, left, center, right = page.children
+    assert ''.join(
+        getattr(node, 'text', '') for node in left.descendants()) == 'inline'
+    assert ''.join(
+        getattr(node, 'text', '') for node in center.descendants()) == 'block'
+    assert ''.join(
+        getattr(node, 'text', '') for node in right.descendants()) == 'table'