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

Fix computed values on table element overwritten after first page in running tables #2051

Merged
merged 1 commit into from
Feb 2, 2024

Conversation

kygoh
Copy link
Contributor

@kygoh kygoh commented Jan 30, 2024

This is a naive fix using "ghost" style properties until a proper fix can be implemented.

The comment before the computed values are overwritten is probably a hint for a proper fix but unable to grasp the logic of table wrapper box and table box yet:

# 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

Note: This will fix #2013

@liZe
Copy link
Member

liZe commented Jan 30, 2024

Thanks for this pull request!

The comment before the computed values are overwritten is probably a hint for a proper fix but unable to grasp the logic of table wrapper box and table box yet

For now, just as for #1278, I’d prefer to keep the bug than to add a fake value in the style dictionary. We can keep this PR open as a draft until we find the source of the problem and fix it in a clean and solid way.

@liZe liZe marked this pull request as draft January 30, 2024 14:49
@kygoh
Copy link
Contributor Author

kygoh commented Jan 31, 2024

the source of the problem

The following computed values on table element are overwritten by initial values in the first page:

# https://www.w3.org/TR/CSS21/tables.html#model
# See also https://lists.w3.org/Archives/Public/www-style/2012Jun/0066.html
# Only non-inherited properties need to be included here.
TABLE_WRAPPER_BOX_PROPERTIES = {
'bottom',
'break_after',
'break_before',
'break_inside',
'clear',
'counter_increment',
'counter_reset',
'counter_set',
'float',
'left',
'margin_top',
'margin_bottom',
'margin_left',
'margin_right',
'opacity',
'overflow',
'position',
'right',
'top',
'transform',
'transform_origin',
'vertical_align',
'z_index',
}

From https://lists.w3.org/Archives/Public/www-style/2012Jun/0066.html:

Elements with 'display: table' generate two boxes: a table wrapper box and a table box.

and in https://www.w3.org/TR/CSS21/tables.html#model, these computed values

on the table element are used on the table wrapper box and not the table box; all other values of non-inheritable properties are used on the table box and not the table wrapper box. (Where the table element's values are not used on the table and table wrapper boxes, the initial values are used instead.)

In running tables, the table wrapper box and table box will be generated for every page. However for subsequent pages, the above computed values for the table element have been overwritten by initial values - it appears that table box computed values are copied in subsequent pages.

@kygoh
Copy link
Contributor Author

kygoh commented Feb 1, 2024

Previously box style was copied as well:

table = box.copy_with_children(row_groups, copy_style=True)

@kygoh kygoh marked this pull request as ready for review February 1, 2024 08:58
@kygoh
Copy link
Contributor Author

kygoh commented Feb 1, 2024

The box.copy_with_children method in the following code appears to be the source of the problem:

table = box.copy_with_children(row_groups)

It does not make a copy of box.style for table.style but appears to be referencing box.style. Thus, changing table.style[name] is actually changing box.style[name].

@liZe
Copy link
Member

liZe commented Feb 2, 2024

It does not make a copy of box.style for table.style but appears to be referencing box.style. Thus, changing table.style[name] is actually changing box.style[name].

That’s a recurring problem in WeasyPrint, see for example #950 and #497.

Copying styles was an easy solution, but it was really slow and required a lot of memory (and caused different bugs). That’s why the current solution is to avoid style edition when possible, and to copy it only when it’s necessary. It’s a bit dangerous, but the performance improvement is worth the risk.

The goal is to freeze style dictionaries to forbid all editions. But unfortunately, the few cases left are not easy to fix.

Where is the style modified? We could try to take care of this special case instead of always copying it.

@kygoh
Copy link
Contributor Author

kygoh commented Feb 2, 2024

Where is the style modified?

for name in properties.TABLE_WRAPPER_BOX_PROPERTIES:
wrapper.style[name] = table.style[name]
table.style[name] = properties.INITIAL_VALUES[name]

@liZe
Copy link
Member

liZe commented Feb 2, 2024

Where is the style modified?

Oh, of course! 😁

If it’s OK for you, I’ll keep 3926136 and merge this pull request.

@liZe liZe added this to the 61.0 milestone Feb 2, 2024
@liZe
Copy link
Member

liZe commented Feb 2, 2024

Thanks a lot for your work!

@liZe liZe merged commit e64b69c into Kozea:main Feb 2, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Margin is not applied to running tables
2 participants