Add support for concatenating var() functions in 'content' declarations #1165

mikevoets · 2020-07-16T20:58:30Z

Concatenation of var() functions in content declarations does not work. Currently, a variable in content declaration gets resolved correctly, however it disregards the remaining value of a content declaration. E.g.:

body { --var: 'Paragraph'; counter-reset: p }
p { counter-increment: p }
p:before { content: var(--var) ': ' counter(p); }

resolves p:before to Paragraph, and not to as you would expect; Paragraph: 1.

This bug is fixed in the changes in this PR.

Linked to my comment in #898.

mikevoets · 2020-07-16T22:12:48Z

Checks fail in Python 3.5 due to some unexpected syntax in .egg files. Any idea how these can be fixed?

liZe

Thanks a lot for this pull request.

I think that there’s something wrong in the way var() is handled in properties with multiple values, not just content. Having workarounds for compute and the validator make your test pass (and I really appreciate tests 😄), but I feel that there’s something wrong.

I need some time to dive deep into this bug and find what’s wrong. If you can find a clean way to solve it before me, I’d be happy too 😉.

weasyprint/css/validation/properties.py

weasyprint/css/computed_values.py

liZe · 2020-07-18T11:43:09Z

Checks fail in Python 3.5 due to some unexpected syntax in .egg files. Any idea how these can be fixed?

Isort doesn’t support Python 3.5 anymore, and WeasyPrint could follow it for the next release. We can safely ignore this error for now.

mikevoets · 2020-07-20T00:30:35Z

weasyprint/css/validation/properties.py

-        var_function = check_var_function(token)
-        if var_function:
-            return ((name, var_function),)
+    if name not in MULTIVAL_PROPERTIES:


This is necessary because a content declaration is a special case since its property handler returns a list, as opposed to every other property. Currently if one of the rules in content is a variable function, it would only parse that variable function and return it from the function here, without processing the other rules. This is why I have added a special case condition here.

I cleaned it up by declaring the content property as a multi values property.

mikevoets · 2020-07-20T00:31:33Z

weasyprint/css/utils.py

@@ -699,6 +699,11 @@ def get_content_list_token(token, base_url):
    if string is not None:
        return string

+    # <var>


Here a variable function in a content declaration is acknowledged and returned accordingly.

mikevoets · 2020-07-20T00:35:34Z

weasyprint/css/computed_values.py

+
+            if not isinstance(value, list):
+                converted_to_list = True
+                value = [value]


Multi values properties (for now only content property classifies as such) return a list that possibly includes variable functions among their declaration values. Therefore, isinstance(value, tuple) and value[0] == 'var()' would never be true for that situation.

I fixed this by looping over all the values and compute the variables when there are variable functions. I temporarily convert non-list declaration values to a list to do this elegantly.

mikevoets · 2020-07-20T00:37:36Z

weasyprint/css/computed_values.py

+                already_computed_value = name not in INITIAL_NOT_COMPUTED
+                value = INITIAL_VALUES[name]
+        elif isinstance(new_value, list):
+            value, = new_value


The content property handler always returns a list regardless of input. In this case, the handler will only ever get a computed variable declaration, and thus I unpack the computed value again after getting it back from the handler.

mikevoets · 2020-07-20T00:38:26Z

@liZe I've replied to your comments in my newest commit. I have also cleaned up a whole bit. Thanks for your review and feedback!

mikevoets

See my newest comments.

mikevoets · 2020-07-27T21:06:42Z

@liZe You're probably busy, but did you have time to look at this?

liZe · 2020-07-29T12:14:17Z

@liZe You're probably busy, but did you have time to look at this?

I’m busy, as I’m… on holidays 😄. Thanks a lot for your changes, I’ll have the time to review them this week or next week.

mikevoets · 2020-08-09T19:58:08Z

Hope you had a good holiday @liZe. If there's anything I can do to improve the PR, let me know :-)

liZe · 2020-08-10T20:15:12Z

Hope you had a good holiday @liZe. If there's anything I can do to improve the PR, let me know :-)

I’m late, I’m really sorry… It’s currently complicated for me to find the time for a good review. I’ll do my best…

mikevoets · 2020-08-11T21:36:58Z

Hope you had a good holiday @liZe. If there's anything I can do to improve the PR, let me know :-)

I’m late, I’m really sorry… It’s currently complicated for me to find the time for a good review. I’ll do my best…

All good! Hope you'll be able to look at it soon. Weasyprint has been extremely useful in my projects and I am happy to continuously contribute to improve it.

mikevoets · 2020-08-31T21:38:43Z

Hi @liZe, just checking in to see if you had any chance to review this PR any time soon? I'm depending on this fix currently, and I need to use a fork of this repository until the PR has been merged. Hope I can revert to upstream soon!

liZe · 2020-09-01T21:36:15Z

Hi @liZe, just checking in to see if you had any chance to review this PR any time soon? I'm depending on this fix currently, and I need to use a fork of this repository until the PR has been merged. Hope I can revert to upstream soon!

Thanks again for your patience.

I’ve found the time to read your pull request, and I have mixed feelings about it. Your code definitely goes in the right direction, I will merge it, and I’ll add a couple of minor changes (for example, to add all the properties returning iterables).

The main problem with this PR is not your code 😄. The main problem is that this PR shows how much the current implementation is wrong. I already knew that a lot of CSS values are stored in random Python structures, and passed from function to function where they’re randomly detected and randomly computed. It’s been bad for a long time, but it now reaches dangerous limits, and I don’t want to go beyond.

Let’s say this PR is the last one before a large rewrite. If we want to have var, attr and calc (hello #357!) working correctly, we have to clean this quickly before it turns into a monster (I’m looking at you, split_first_line).

mikevoets · 2020-09-01T22:02:10Z

Thanks so much. If there's anything I can do to help you cleaning up, let me know.

liZe · 2020-09-04T06:30:11Z

Thanks again, it’s finally merged.

I will merge it, and I’ll add a couple of minor changes (for example, to add all the properties returning iterables).

Most of the properties returning iterables use the comma_separated_list decorator and return a tuple. Having a single way to handle var for content and these properties would require a lot of work.

We’ll rewrite this sooner or later, to get a clean support of var, calc and attr.

mikevoets · 2020-09-06T21:21:08Z

@liZe thanks!

Mike Voets added 3 commits July 17, 2020 08:50

Add support for concatenating var() functions in 'content' declarations

3f3b564

Fix variables tests

b234d1b

Fix lint issues

d08c5b4

liZe requested changes Jul 18, 2020

View reviewed changes

weasyprint/css/validation/properties.py Outdated Show resolved Hide resolved

weasyprint/css/computed_values.py Outdated Show resolved Hide resolved

weasyprint/css/computed_values.py Outdated Show resolved Hide resolved

Mike Voets added 2 commits July 20, 2020 12:25

Make changes more generic

db3944c

Clean up comment

e444c95

mikevoets commented Jul 20, 2020

View reviewed changes

Fix blank line warning

736e62f

mikevoets commented Jul 21, 2020

View reviewed changes

liZe merged commit 89ab158 into Kozea:master Sep 4, 2020

mikevoets deleted the fix/variables-concatenation-in-content branch September 6, 2020 21:20

liZe mentioned this pull request Sep 28, 2020

Handle var function in properties with multiple values #1219

Closed

grewn0uille added this to the 52 milestone Oct 29, 2020

liZe mentioned this pull request Jan 20, 2021

ValueError: too many values to unpack (expected 1) - on version 52.2 #1287

Closed

Mo0rBy mentioned this pull request Feb 17, 2023

Getting the following error: "ValueError: too many values to unpack (expected 4)" #1811

Closed

cavasinf mentioned this pull request Feb 24, 2023

52.5 - concatenating CSS vars() #1819

Closed

FozzieHi mentioned this pull request Nov 25, 2023

Rendering issue on MacOS preview #2010

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for concatenating var() functions in 'content' declarations #1165

Add support for concatenating var() functions in 'content' declarations #1165

mikevoets commented Jul 16, 2020 •

edited

Loading

mikevoets commented Jul 16, 2020

liZe left a comment

liZe commented Jul 18, 2020

mikevoets Jul 20, 2020 •

edited

Loading

mikevoets Jul 20, 2020

mikevoets Jul 20, 2020 •

edited

Loading

mikevoets Jul 20, 2020

mikevoets commented Jul 20, 2020

mikevoets left a comment

mikevoets commented Jul 27, 2020

liZe commented Jul 29, 2020

mikevoets commented Aug 9, 2020

liZe commented Aug 10, 2020

mikevoets commented Aug 11, 2020

mikevoets commented Aug 31, 2020 •

edited

Loading

liZe commented Sep 1, 2020

mikevoets commented Sep 1, 2020

liZe commented Sep 4, 2020

mikevoets commented Sep 6, 2020

Add support for concatenating var() functions in 'content' declarations #1165

Add support for concatenating var() functions in 'content' declarations #1165

Conversation

mikevoets commented Jul 16, 2020 • edited Loading

mikevoets commented Jul 16, 2020

liZe left a comment

Choose a reason for hiding this comment

liZe commented Jul 18, 2020

mikevoets Jul 20, 2020 • edited Loading

Choose a reason for hiding this comment

mikevoets Jul 20, 2020

Choose a reason for hiding this comment

mikevoets Jul 20, 2020 • edited Loading

Choose a reason for hiding this comment

mikevoets Jul 20, 2020

Choose a reason for hiding this comment

mikevoets commented Jul 20, 2020

mikevoets left a comment

Choose a reason for hiding this comment

mikevoets commented Jul 27, 2020

liZe commented Jul 29, 2020

mikevoets commented Aug 9, 2020

liZe commented Aug 10, 2020

mikevoets commented Aug 11, 2020

mikevoets commented Aug 31, 2020 • edited Loading

liZe commented Sep 1, 2020

mikevoets commented Sep 1, 2020

liZe commented Sep 4, 2020

mikevoets commented Sep 6, 2020

mikevoets commented Jul 16, 2020 •

edited

Loading

mikevoets Jul 20, 2020 •

edited

Loading

mikevoets Jul 20, 2020 •

edited

Loading

mikevoets commented Aug 31, 2020 •

edited

Loading