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

Multiple values for "text-decoration" are not applied correctly #1621

Closed
Morikko opened this issue Apr 12, 2022 · 9 comments
Closed

Multiple values for "text-decoration" are not applied correctly #1621

Morikko opened this issue Apr 12, 2022 · 9 comments
Labels
bug Existing features not working as expected
Milestone

Comments

@Morikko
Copy link

Morikko commented Apr 12, 2022

We have found a few cases where text-decoration values are not applied correctly: only a single value is possible at a time.

The HTML output is based on Firefox 98 and the PDF is generated with weasyprint==54.3.

import weasyprint
from IPython.display import display_pdf

css_size = weasyprint.CSS(string="""
@page {
  size: 100mm;
}
""")

def generate_pdf(data_str):
    html_obj = weasyprint.HTML(string=data_str)
    pdf_data = html_obj.write_pdf(stylesheets=[css_size])
    display_pdf(pdf_data, raw=True)

Single Tag with multiple options

<span style="text-decoration: underline line-through;">
        demo text
</span>

HTML

2022-04-12_11-16 html_single_span_multi_values

PDF

2022-04-12_11-17 pdf_single_span_multi_values_regression

It used to work with weasyprint==52.5.

2022-04-12_11-17 pdf_single_span_multi_values

Two tags with different options

<span style="text-decoration: underline;">
    <span style="text-decoration: line-through;">
        demo text
    </span>toto
</span>

HTML

2022-04-12_11-22 html_multi_spans

PDF

2022-04-12_11-23 pdf_multi_spans

Historical HTML tags

<p><u><s>demo text</s>toto</u></p>

<p><del><u>demo text</u>toto</del></p>

HTML

2022-04-12_11-23 html_html_tags

PDF

2022-04-12_11-23 pdf_html_tags

With other CSS properties

Other CSS properties like color or font-weight are applied aside only a single value text-decoration:

<del style="color: blue;"><u>d<span style="font-weight: bold;">em</span>o te<b>xt</b></u></del>

HTML

2022-04-12_11-26 html_cross_css

PDF

2022-04-12_11-26 pdf_cross_css

@liZe liZe closed this as completed in 20c91dc Apr 12, 2022
@liZe liZe added the bug Existing features not working as expected label Apr 12, 2022
@liZe liZe added this to the 55.0 milestone Apr 12, 2022
@liZe
Copy link
Member

liZe commented Apr 12, 2022

Thanks a lot for the bug report and the examples! It should be fixed (and tested) in master, real-life tests are welcome.

@Morikko
Copy link
Author

Morikko commented Apr 12, 2022

Thank you for the reactivity. I can confirm that the case Single Tag with multiple options (regression) is fixed.

However all the other cases are still broken. Only the most nested value of text-decoration is kept.

@liZe liZe reopened this Apr 12, 2022
@liZe
Copy link
Member

liZe commented Apr 12, 2022

However all the other cases are still broken. Only the most nested value of text-decoration is kept.

Sorry, I shouldn’t have closed this issue. Only the first example is fixed.

@liZe
Copy link
Member

liZe commented Apr 12, 2022

The CSS specification is full of surprises about inheritance.

@liZe liZe closed this as completed in af9a495 Apr 13, 2022
@liZe
Copy link
Member

liZe commented Apr 13, 2022

This bug is now closed, because all the examples given here are now rendered "correctly".

But, even if the new behavior is better than the previous one, it’s far from perfect. Here are some examples of what don’t work yet:

For all other box types, the decorations are propagated to all in-flow children.

Decorations are currently propagated to all children.

[…] for underlines and overlines the UA must use a single thickness and position on each line for the decorations deriving from a single decorating box.

That’s not true for thickness (because of font-weight or font-size), that’s not true for color.

If anyone needs this problem to be fixed, please open a new issue! Unfortunately, it will require more complicated code (and more time).

@liZe
Copy link
Member

liZe commented Apr 13, 2022

(There’s one more bug that needs to be fixed, sorry…)

liZe added a commit that referenced this issue Apr 13, 2022
Even if there are many unsupported cases, this one was a real bug in the test.

Related to #1621.
@liZe
Copy link
Member

liZe commented Apr 13, 2022

(Tests now pass.)

@Morikko
Copy link
Author

Morikko commented Apr 13, 2022

Great, I confirm that all the cases in this issue are fixed 🎉. Thank you very much. That CSS specification is definitively not simple 😅

Do you know when 55 is planned to be released ?


For the non technical CSS people like me, here the visual impact of the non cover cases

<div style="color: red;text-decoration: underline blue;">
a
<span style="text-decoration: line-through lime;">b</span>
c</div>

2022-04-13_11-07 color diff weasyprint

<div style="color: red;text-decoration: underline blue;">a<span style="float: right;">b</span></div>

2022-04-13_11-11 weasyprint float case

@liZe
Copy link
Member

liZe commented Apr 13, 2022

Do you know when 55 is planned to be released ?

You can expect a beta to be released in the next days / weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Existing features not working as expected
Projects
None yet
Development

No branches or pull requests

2 participants