Propose fix for issue #1278

[kygoh]

Before these properties are overwritten in the set_transparent_border function:

• border_{side}_style
• border_{side}_width
• border_{side}_color

a copy of the original value is stored in the style dictionary using property name prefixed with double underscore __:

• __border_{side}_style
• __border_{side}_width
• __border_{side}_color

When collapse_table_border is called each time the boxes of the table are built, the original property values are restored first, if exists.

[liZe]

Hi!

Thanks a lot for your proposal. 💜

a copy of the original value is stored in the style dictionary using property name prefixed with double underscore __

That’s something I would really like to avoid 😢. Adding "ghost" style properties is a workaround that often leads to other problems in the future, and even whet it seems harmless it’s often a trap set for future developers.

As explained in this comment and the next one, the solution is to keep the real style and to change both the layout.table module to handle correct border widths and the draw module to draw correct colors. Even if it may be really appealing, any other solution is a workaround on top of a buggy workaround, and that’s the kind of things that often give maintenance nightmares.

Would you be interested in trying to find such a solution?

[kygoh]

Adding "ghost" style properties is a workaround that often leads to other problems in the future

I am aware of the technical debt and the reason I only propose this PR rather than submit it.

Since I've created the test case to simulate the issue, the "ghost" style properties workaround was used to pass the test until a proper solution can be implemented.

Would you be interested in trying to find such a solution?

I would be interested to try but this library is still new to me and I am still unclear of its inner workings.

[kygoh]

layout.table module to handle correct border widths

Do you mean that these lines should be removed:

WeasyPrint/weasyprint/formatting_structure/build.py

Lines 979 to 981 in 0381529

	if table.style['border_collapse'] == 'collapse':
	table.collapsed_border_grid = collapse_table_borders(
	table, grid_width, grid_height)

and the collapse_table_borders border conflicts resolution logic be handled in layout.table module?

Then, grid_width and grid_height values need to be passed to layout.table?

I think the table.collapsed_border_grid tuple is only used in 2 places:

WeasyPrint/weasyprint/draw.py

Line 816 in 0381529

vertical_borders, horizontal_borders = table.collapsed_border_grid

WeasyPrint/weasyprint/layout/table.py

Line 67 in 0381529

_, horizontal_borders = table.collapsed_border_grid

and for test cases:

WeasyPrint/tests/test_boxes.py

Line 22 in 0381529

for grid in table.collapsed_border_grid)

draw module to draw correct colors

If the structure of (vertical_borders, horizontal_borders) can be preserved, then the following won't need to change?

WeasyPrint/weasyprint/draw.py

Line 862 in 0381529

score, (style, width, color) = vertical_borders[yy][x]

WeasyPrint/weasyprint/draw.py

Line 884 in 0381529

score, (style, width, color) = horizontal_borders[yy][x]

[liZe]

Do you mean that these lines should be removed and the collapse_table_borders border conflicts resolution logic be handled in layout.table module?

Yes, exactly.

Then, grid_width and grid_height values need to be passed to layout.table?

Yes.

If the structure of (vertical_borders, horizontal_borders) can be preserved, then the following won't need to change?

That would be great!

More changes are probably required for the layout than for the drawing, because the drawing part is already different for collapsed and separate cells.

[kygoh]

Please take a look at the latest changes that omitted border_{side}_style and border_{side}_color properties from set_transparent_border yet passed all test cases.

At this point is it safe to assume only the change of border_{side}_width in the box style dictionary causes issue #1278?

From your #1278 (comment):

we also probably need to change the layout.table module to use the right value of the border width

I think the right value will be:

WeasyPrint/weasyprint/formatting_structure/build.py

Line 1102 in 0381529

box.style[f'border_{side}_width'] = twice_width / 2

If the border_{side}_width is omitted from set_transparent_border, among the tests that failed are:

FAILED tests/layout/test_table.py::test_layout_table_fixed_5 - assert 10.0 == 5
FAILED tests/layout/test_table.py::test_layout_table_auto_17 - assert 10.0 == 5

due to:

>       assert table.border_left_width == 5  # half of the collapsed 10px border
E       assert 10.0 == 5
E        +  where 10.0 = <TableBox table>.border_left_width

Can you point out where table.border_left_width value is set?

[liZe]

Thanks a lot for your work on this topic.

At this point is it safe to assume only the change of border_{side}_width in the box style dictionary causes issue #1278?

Damn, you’re right! Border style and color are not useful for the layout, and they’re never used during the drawing (as we draw collapsed borders instead). There’s probably no need at all to change them in the style, and set_transparent_border is then not the best name to describe what we do (that is changing each border’s width to make the layout respect the changes introduced by collapsing).

I think the right value will be: twice_width / 2

Yes, it is.

Can you point out where table.border_left_width value is set?

These attributes are used values. For border width, it’s set by resolve_percentages.

The specification explains that the collapsed border width values should be set as used values. So, the solution is to change these attributes instead of the style dictionary (that are computed values), and check that everything else works well (ie. that the whole table layout code uses the used values, and not the computed values for example).

[kygoh]

These attributes are used values:

The used value is the result of taking the computed value and completing any remaining calculations to make it the absolute theoretical value used in the formatting of the document.

WeasyPrint/docs/going_further.rst

Lines 225 to 226 in 0381529

	While ``box.style`` contains computed values, the `used values`_ are set as
	attributes of the ``Box`` object itself during the layout. This include

Based on the above, is the following correct?

• table.style['border-left-width'] contains computed value
• used value table.border_left_width can be "set" from computed value table.style['border-left-width']

For border width, it’s set by resolve_percentages:

At the following point?

WeasyPrint/weasyprint/layout/table.py

Line 807 in 0381529

resolve_percentages(table, containing_block)

the solution is to change these attributes instead of the style dictionary (that are computed values)

meaning:

WeasyPrint/weasyprint/formatting_structure/build.py

Line 1102 in 0381529

box.style[f'border_{side}_width'] = twice_width / 2

should instead be:

    setattr(box, f'border_{side}_width', twice_width / 2)

[liZe]

Based on the above, is the following correct?

Yes, it is.

At the following point?

resolve_percentage is called for each box (table, row, cell…), in different parts of layout.table. Here, it’s just for the table, but it’s also done for rows, cells, etc.

meaning box.style[f'border_{side}_width'] = twice_width / 2 should instead be setattr(box, f'border_{side}_width', twice_width / 2)

That’s the idea, but it may not be that simple. The current bad solution replaces the style during the build step (that builds the boxes), so that the layout step (that sets the position and the size of each box) is sure to use the values that result from the collapsing algorithm. Now, we have to set the used values during the layout step.

We have to set the collapsed values after almost each resolve_percentages call in layout.table. It means that this code can be removed, and replaced by equivalent calls after almost each call to resolve_percentages in layout.table.

Here’s what you can try:

1. Remove this code.
2. Write equivalent functions (remove_borders, and set_collapsed_border instead of set_transparent_border as we only need change the width) in layout.table, that will set used values (setattr(box, f'border_{side}_width', twice_width / 2)) instead of computed values.
3. Find the resolve_percentages calls in layout.table that are called on the same boxes as the previous remove_borders or set_transparent_border. Call remove_borders or set_collapsed_border after the corresponding resolve_percentages call (only when border-collapse is set to collapse, of course), so that we have the right used value as soon as possible.

If some tests don’t pass after this, it probably means that we use computed values instead of used values somewhere during the layout. But maybe you’ll be lucky and everything will work as expected!

[kygoh]

The problem encountered with "experiment" using used values over computed values for border-{...}-width:

================================================================ short test summary info ================================================================
FAILED tests/layout/test_table.py::test_layout_table_fixed_5 - assert 32.0 == 34
=========================================================== 1 failed, 112 deselected in 2.01s ===========================================================

at

WeasyPrint/tests/layout/test_table.py

Line 239 in 21810ba

assert td_2.width == 34

for HTML:

WeasyPrint/tests/layout/test_table.py

Lines 206 to 223 in 21810ba

	<style>
	/* Do not apply: */
	colgroup, col, tbody, tr, td { margin: 1000px }
	</style>
	<table style="table-layout: fixed;
	border-collapse: collapse; border: 10px solid;
	/* ignored with collapsed borders: */
	border-spacing: 10000px; padding: 1000px">
	<colgroup>
	<col style="width: 30px" />
	</colgroup>
	<tbody>
	<tr>
	<td style="padding: 2px"></td>
	<td style="width: 34px; padding: 10px; border: 2px solid"></td>
	</tr>
	</tbody>
	</table>

Tracing the source code for the failed test, I think:

1. When table_wrapper_width is called, the border-{...}-width used values for cells have not been harmonized nor set.
2. The table width is derived from table_min_content_width value at:
   

   WeasyPrint/weasyprint/layout/table.py

   Lines 708 to 712 in 21810ba

   	(table_min_content_width, table_max_content_width,
   	column_min_content_widths, column_max_content_widths,
   	column_intrinsic_percentages, constrainedness,
   	total_horizontal_border_spacing, grid) = \
   	table_and_columns_preferred_widths(context, box, outer=False)

3. Stepping through the code for how table_min_content_width and table_max_content_width are derived will eventually lead to the margin_width function which is referencing computed values:
   

   WeasyPrint/weasyprint/layout/preferred.py

   Lines 150 to 153 in 21810ba

   	if left:
   	width += box.style['border_left_width']
   	if right:
   	width += box.style['border_right_width']

4. Putting breaks in the source shows that at
   

   WeasyPrint/weasyprint/layout/table.py

   Line 777 in 21810ba

   table.column_widths = upper_guess

5. table.column_widths = [ 30, 60 ] for used values vs table.column_widths = [ 30, 58 ] for computed values due to:

   54 (width) + 1 (border-left-width) + 5 (border-right-width) = 60
   54 (width) + 2 (border-left-width) + 2 (border-right-width) = 58
   

Any pointers?

[kygoh]

1. Remove this code.

This code is not so straightforward to remove as the border_{left,right}_width computed values are being used to determine preferred margin width:

WeasyPrint/weasyprint/layout/preferred.py

Lines 150 to 153 in 21810ba

	if left:
	width += box.style['border_left_width']
	if right:
	width += box.style['border_right_width']

Thus, I've made "smaller" modifications first until I can better understand the logic:

1. Instead of using __border_{...}_width to "backup" the original values, new computed values collapse_border_{...}_width is used to store the harmonized collapsed border widths (commit 9e59b20) (Note: property prefixed with __ may have special behavior - refer here)
2. These new computed values collapse_border_{...}_width are set as used values border_{...}_width in resolve_percentages (commit 800a683)
3. These new computed values collapse_border_{...}_width are also used in preferred margin width calculation when applicable (commit d87034f d4a79bc)

I believe used values will not be available yet when margin_width function is called as described in earlier #1884 (comment)

[kygoh]

3. [...] so that we have the right used value as soon as possible

Continuing from the latest commit, which have passed all tests, if I naively change margin_width to used values:

    if left:
        width += box.border_left_width
    if right:
        width += box.border_right_width

the result for tests/layout/test_table.py::test_layout_table_fixed_5 :

================================================================ short test summary info ================================================================
FAILED tests/layout/test_table.py::test_layout_table_fixed_5 - AttributeError: 'TableColumnBox' object has no attribute 'border_left_width'. Did you mean: 'border_width'?

with the relevant stack trace:

...
weasyprint/layout/table.py:712: in auto_table_layout
    table_and_columns_preferred_widths(context, box, outer=False)
weasyprint/layout/preferred.py:448: in table_and_columns_preferred_widths
    min_content_width(context, groups[i]))
weasyprint/layout/preferred.py:47: in min_content_width
    return block_min_content_width(context, box, outer)
weasyprint/layout/preferred.py:180: in block_min_content_width
    return _block_content_width(
weasyprint/layout/preferred.py:104: in _block_content_width
    return adjust(box, outer, width)
weasyprint/layout/preferred.py:173: in adjust
    return margin_width(box, fixed, left, right)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

box = <TableColumnBox col>, width = 30.0, left = True, right = True

    def margin_width(box, width, left=True, right=True):
        """Add box paddings, borders and margins to ``width``."""
        percentages = 0
    
        for value in (
            (['margin_left', 'padding_left'] if left else []) +
            (['margin_right', 'padding_right'] if right else [])
        ):
            style_value = box.style[value]
            if style_value != 'auto':
                if style_value.unit == 'px':
                    width += style_value.value
                else:
                    assert style_value.unit == '%'
                    percentages += style_value.value
    
        collapse = box.style['border_collapse'] == 'collapse'
        if left:
>           width += box.border_left_width
E           AttributeError: 'TableColumnBox' object has no attribute 'border_left_width'. Did you mean: 'border_width'?

From the stack trace, it's obvious that resolve_percentages has not been called to set the used values for TableColumnBox.

If we were to stick to just used values for border-{...}-width, could this lead to a cyclic problem ie table width requires columns and cells widths (which haven't been resolved) but columns and cells widths need table width (which can't be resolved)?

[kygoh]

Please review the latest commit:

1. collapse_table_border has been moved to layout.table to better reflect its functionality
2. instead of overwriting border-{...}-width computed values during collapsed-borders conflict resolution, they are set as used values
3. modification appears to pass all tests.

While I think the code can be refactored further, I will appreciate if you can merge this fix for the next release.

[kygoh]

@liZe please review the latest commit:

1. The computed border styles are now left untouched so restoring original values with "ghost" style properties are not required anymore (commit 9e59b20)
2. With the used border widths being set, these are also being used to compute cell and table intrinsic offsets as stated in 3.8.2. Computing Cell Measures (commit bb382b5)
3. Codes and test cases related to border-collapse have been moved to layout.table
4. However, the draw module is still using the winning border properties table.collapsed_border_grid rather than used values - probably to go back after gaining more familiarity.

I hope this fix can be merged while we continue improving the maintainability of border collapse codes.

[liZe]

Sorry for the delay, we’re currently quite busy with other projects! We’ll review and hopefully merge this PR as soon as possible.

[kygoh]

1. The computed border styles are now left untouched so restoring original values with "ghost" style properties are not required anymore (commit 9e59b20)

To summarize the key areas of the fix:

The original set_transparent_border causes border styles to be modified:

WeasyPrint/weasyprint/formatting_structure/build.py

Lines 1100 to 1103 in 4e9ad95

	def set_transparent_border(box, side, twice_width):
	box.style[f'border_{side}_style'] = 'solid'
	box.style[f'border_{side}_width'] = twice_width / 2
	box.style[f'border_{side}_color'] = TRANSPARENT

so that margin_width uses the new collapsed width:

WeasyPrint/weasyprint/layout/preferred.py

Lines 150 to 153 in 4e9ad95

	if left:
	width += box.style['border_left_width']
	if right:
	width += box.style['border_right_width']

and later store as used values in resolve_percentages:

WeasyPrint/weasyprint/layout/percent.py

Lines 95 to 98 in 4e9ad95

	# Used value == computed value
	for side in ('top', 'right', 'bottom', 'left'):
	prop = f'border_{side}_width'
	setattr(box, prop, box.style[prop])

This fix stores the collapsed border width in used values:

WeasyPrint/weasyprint/layout/table.py

Lines 1082 to 1084 in af141ba

	def set_border_used_width(box, side, twice_width):
	prop = f'border_{side}_width'
	setattr(box, prop, twice_width / 2)

and modifies margin_width to get the collapsed width from used values, if set:

WeasyPrint/weasyprint/layout/preferred.py

Lines 154 to 171 in af141ba

	collapse = box.style['border_collapse'] == 'collapse'
	if left:
	if collapse and hasattr(box, 'border_left_width'):
	# In collapsed-borders mode: the computed horizontal padding of the
	# cell and, for border values, the used border-width values of the
	# cell (half the winning border-width)
	width += box.border_left_width
	else:
	# In separated-borders mode: the computed horizontal padding and
	# border of the table-cell
	width += box.style['border_left_width']
	if right:
	if collapse and hasattr(box, 'border_right_width'):
	# [...] the used border-width values of the cell
	width += box.border_right_width
	else:
	# [...] the computed border of the table-cell
	width += box.style['border_right_width']

and skip storing as used values again in resolve_percentages:

WeasyPrint/weasyprint/layout/percent.py

Lines 95 to 102 in af141ba

	collapse = box.style['border_collapse'] == 'collapse'
	# Used value == computed value
	for side in ('top', 'right', 'bottom', 'left'):
	prop = f'border_{side}_width'
	# border-{side}-width would have been resolved
	# during border conflict resolution for collapsed-borders
	if not (collapse and hasattr(box, prop)):
	setattr(box, prop, box.style[prop])

[liZe]

Hi!

Thanks for your work on this issue 💜.

I’ve changed the tests to use drawing, as it’s easier to debug, and to be sure that the rendering is what we actually expect from the layout.

(Images are generated in tests/draw/results/* to check what’s going on.)

It looks like there are differences between what we have and what we expect:

• For test_running_elements_table_border_collapse and test_running_elements_table_border_collapse_span, there are spaces after the text. We don’t have that with the previous version, there must be something wrong.
• test_running_elements_table_border_collapse_border_style seems to be buggy. As it was also buggy with previous version, I’ve marked it as xfail.
• test_running_elements_table_border_collapse_margin shows that the margin doesn’t work when repeated. It was buggy in the previous version, it’s also marked as xfail.

So, what’s important is to find where this space comes from, other bugs can be fixed later. I’ll check the PR in details when these two bugs pass.

[kygoh]

For test_running_elements_table_border_collapse and test_running_elements_table_border_collapse_span, there are spaces after the text. We don’t have that with the previous version, there must be something wrong.

The space arises from moving the collapsed-borders conflict resolution from formatting_structure.build.wrap_table (called before make_margin_boxes):

WeasyPrint/weasyprint/formatting_structure/build.py

Lines 979 to 981 in 9e59b20

	if table.style['border_collapse'] == 'collapse':
	table.collapsed_border_grid = collapse_table_borders(
	table, grid_width, grid_height)

to layout.table.table_wrapper_width (called after make_margin_boxes):

WeasyPrint/weasyprint/layout/table.py

Lines 812 to 817 in 2a3747a

	def table_wrapper_width(context, wrapper, containing_block):
	"""Find the width of each column and derive the wrapper width."""
	table = wrapper.get_wrapped_table()
	collapse = table.style['border_collapse'] == 'collapse'
	if collapse:
	table.collapsed_border_grid = collapse_table_borders(table)

The used border-width values of the cell (half the winning border-width) is used by make_margin_boxes to determine the cells and table width:

WeasyPrint/weasyprint/layout/page.py

Lines 445 to 447 in ce46dbe

	# We need the three boxes together for the variable dimension:
	compute_variable_dimension(
	context, side_boxes, vertical, variable_outer)

through margin_width:

WeasyPrint/weasyprint/layout/preferred.py

Lines 154 to 171 in ce46dbe

	collapse = box.style['border_collapse'] == 'collapse'
	if left:
	if collapse and hasattr(box, 'border_left_width'):
	# In collapsed-borders mode: the computed horizontal padding of the
	# cell and, for border values, the used border-width values of the
	# cell (half the winning border-width)
	width += box.border_left_width
	else:
	# In separated-borders mode: the computed horizontal padding and
	# border of the table-cell
	width += box.style['border_left_width']
	if right:
	if collapse and hasattr(box, 'border_right_width'):
	# [...] the used border-width values of the cell
	width += box.border_right_width
	else:
	# [...] the computed border of the table-cell
	width += box.style['border_right_width']

Restoring the collapsed-borders conflict resolution to its original place seems to fix the two bugs.

[kygoh]

Please consider preserving these test cases (originally in test_boxes.py) as unit test on conflict resolution logic for collapsed borders.

[kygoh]

what’s important is to find where this space comes from

Explained in #1884 (comment) and fix has been pushed

• test_running_elements_table_border_collapse_margin shows that the margin doesn’t work when repeated

Appears to be a "repeat" of issue #1278 but for margins and the "ugly fix" to keep the original margins style:

diff --git a/weasyprint/formatting_structure/build.py b/weasyprint/formatting_structure/build.py
index b247eb19..6663214c 100644
--- a/weasyprint/formatting_structure/build.py
+++ b/weasyprint/formatting_structure/build.py
@@ -994,6 +994,13 @@ def wrap_table(box, children):
     # of the wrapper and the table. The other get the initial value.
     # TODO: put this in a method of the table object
     for name in properties.TABLE_WRAPPER_BOX_PROPERTIES:
+        if name in ['margin_left', 'margin_right', 'margin_top', 'margin_bottom']:
+            if f'*-{name}' in table.style:
+                # restore original style
+                table.style[name] = table.style[f'*-{name}']
+            else:
+                # backup original style
+                table.style[f'*-{name}'] = table.style[name]
         wrapper.style[name] = table.style[name]
         table.style[name] = properties.INITIAL_VALUES[name]

[kygoh]

To recap, the root cause of issues (#1278 and #2013 (comment)) boils down to computed values in running tables are being overwritten, box.style at:

WeasyPrint/weasyprint/formatting_structure/build.py

Lines 1094 to 1100 in e018bf3

	# Now that all conflicts are resolved, set transparent borders of
	# the correct widths on each box. The actual border grid will be
	# painted separately.
	def set_transparent_border(box, side, twice_width):
	box.style[f'border_{side}_style'] = 'solid'
	box.style[f'border_{side}_width'] = twice_width / 2
	box.style[f'border_{side}_color'] = TRANSPARENT

and table.style at:

WeasyPrint/weasyprint/formatting_structure/build.py

Lines 988 to 993 in e018bf3

	# Non-inherited properties of the table element apply to one
	# of the wrapper and the table. The other get the initial value.
	# TODO: put this in a method of the table object
	for name in properties.TABLE_WRAPPER_BOX_PROPERTIES:
	wrapper.style[name] = table.style[name]
	table.style[name] = properties.INITIAL_VALUES[name]

[liZe]

@kygoh Thanks a lot 💜