Another take on implementing CSS variables (custom properties) #3314

Neograph734 · 2023-11-02T22:39:32Z

I have always thought this would be difficult because of the time #1872 is already open, but thanks tmolitor-stud-tu for showing how easy it actually is in #3255.

The approach there was a great start, but it was missing the possibility to use fallback values as a second parameter of var(). Furthermore I have shuffled some assignments around and wrote tests. Please share your thoughts.

Neograph734 · 2023-11-03T17:34:23Z

Funny how the tests do pass on my local machine :/

Mellthas · 2023-11-05T15:46:29Z

Funny how the tests do pass on my local machine :/

CI failed with undefined index notices on PHP 7.3, here’s the output:

There were 6 errors

1) Dompdf\Tests\Css\StyleTest::testVar with data set "invalid_value" ('Helvetica', 'var(--undefined)', 'Helvetica')
Undefined index: --undefined

/home/runner/work/dompdf/dompdf/src/Css/Style.php:1235
/home/runner/work/dompdf/dompdf/src/Css/Style.php:1244
/home/runner/work/dompdf/dompdf/src/Css/Style.php:1694
/home/runner/work/dompdf/dompdf/src/Css/Style.php:1723
/home/runner/work/dompdf/dompdf/src/Css/Style.php:1661
/home/runner/work/dompdf/dompdf/tests/Css/StyleTest.php:1083
phpvfscomposer:///home/runner/work/dompdf/dompdf/vendor/phpunit/phpunit/phpunit:106

2) Dompdf\Tests\Css\StyleTest::testInheritedVar with data set "green" (0, '#00ff00FF')
Undefined index: --fallback-propert

/home/runner/work/dompdf/dompdf/src/Css/Style.php:1235
/home/runner/work/dompdf/dompdf/src/Css/Style.php:1250
/home/runner/work/dompdf/dompdf/src/Css/Style.php:1694
/home/runner/work/dompdf/dompdf/src/Css/Style.php:1723
/home/runner/work/dompdf/dompdf/src/Css/Style.php:1283
/home/runner/work/dompdf/dompdf/src/Css/Stylesheet.php:1069
/home/runner/work/dompdf/dompdf/src/Dompdf.php:731
/home/runner/work/dompdf/dompdf/tests/Css/StyleTest.php:1164
phpvfscomposer:///home/runner/work/dompdf/dompdf/vendor/phpunit/phpunit/phpunit:106

3) Dompdf\Tests\Css\StyleTest::testInheritedVar with data set "red" (1, '#ff0000FF')
Undefined index: --fallback-propert

/home/runner/work/dompdf/dompdf/src/Css/Style.php:1235
/home/runner/work/dompdf/dompdf/src/Css/Style.php:1250
/home/runner/work/dompdf/dompdf/src/Css/Style.php:1694
/home/runner/work/dompdf/dompdf/src/Css/Style.php:1723
/home/runner/work/dompdf/dompdf/src/Css/Style.php:1283
/home/runner/work/dompdf/dompdf/src/Css/Stylesheet.php:1069
/home/runner/work/dompdf/dompdf/src/Dompdf.php:731
/home/runner/work/dompdf/dompdf/tests/Css/StyleTest.php:1164
phpvfscomposer:///home/runner/work/dompdf/dompdf/vendor/phpunit/phpunit/phpunit:106

4) Dompdf\Tests\Css\StyleTest::testInheritedVar with data set "yellow" (2, '#ffff00FF')
Undefined index: --fallback-propert

/home/runner/work/dompdf/dompdf/src/Css/Style.php:1235
/home/runner/work/dompdf/dompdf/src/Css/Style.php:1250
/home/runner/work/dompdf/dompdf/src/Css/Style.php:1694
/home/runner/work/dompdf/dompdf/src/Css/Style.php:1723
/home/runner/work/dompdf/dompdf/src/Css/Style.php:1283
/home/runner/work/dompdf/dompdf/src/Css/Stylesheet.php:1069
/home/runner/work/dompdf/dompdf/src/Dompdf.php:731
/home/runner/work/dompdf/dompdf/tests/Css/StyleTest.php:1164
phpvfscomposer:///home/runner/work/dompdf/dompdf/vendor/phpunit/phpunit/phpunit:106

5) Dompdf\Tests\Css\StyleTest::testInheritedVar with data set "blue" (3, '#0000ffFF')
Undefined index: --fallback-propert

/home/runner/work/dompdf/dompdf/src/Css/Style.php:1235
/home/runner/work/dompdf/dompdf/src/Css/Style.php:1250
/home/runner/work/dompdf/dompdf/src/Css/Style.php:1694
/home/runner/work/dompdf/dompdf/src/Css/Style.php:1723
/home/runner/work/dompdf/dompdf/src/Css/Style.php:1283
/home/runner/work/dompdf/dompdf/src/Css/Stylesheet.php:1069
/home/runner/work/dompdf/dompdf/src/Dompdf.php:731
/home/runner/work/dompdf/dompdf/tests/Css/StyleTest.php:1164
phpvfscomposer:///home/runner/work/dompdf/dompdf/vendor/phpunit/phpunit/phpunit:106

6) Dompdf\Tests\Css\StyleTest::testInheritedVar with data set "purple" (4, '#ff00ffFF')
Undefined index: --fallback-propert

/home/runner/work/dompdf/dompdf/src/Css/Style.php:1235
/home/runner/work/dompdf/dompdf/src/Css/Style.php:1250
/home/runner/work/dompdf/dompdf/src/Css/Style.php:1694
/home/runner/work/dompdf/dompdf/src/Css/Style.php:1723
/home/runner/work/dompdf/dompdf/src/Css/Style.php:1283
/home/runner/work/dompdf/dompdf/src/Css/Stylesheet.php:1069
/home/runner/work/dompdf/dompdf/src/Dompdf.php:731
/home/runner/work/dompdf/dompdf/tests/Css/StyleTest.php:1164
phpvfscomposer:///home/runner/work/dompdf/dompdf/vendor/phpunit/phpunit/phpunit:106

When running locally on PHP 8.2, I get several deprecation notices (need to pass --display-deprecations when using PHPUnit 10).

bsweeney · 2023-11-05T16:43:20Z

Approved running the tests so you can see any failures as you make updates.

bsweeney

Some initial thoughts for discussion. I haven't thoroughly evaluated this in relation to the spec nor in relation to how we generally parse CSS properties.

src/Css/Style.php

src/Css/Stylesheet.php

src/Css/Style.php

Neograph734 · 2023-11-06T19:20:37Z

Awsome, those 3 tests fail locally as well because the border shorthand property is never running compute_prop(). That's enough for today.

src/Css/Style.php

Neograph734 · 2023-11-08T21:12:11Z

Thanks for all the reviews, but I am getting the feeling that the different conversations are getting mixed (and duplicated). So I am struggling to keep up...

I'm happy to push the adjustments I tried locally that appear to resolve current issues.

Feel free to commit what you have, I believe you have the required permissions. I don't want to break anything of your work in an attempt to close some of the above threads.

bsweeney · 2023-11-08T22:02:43Z

Thanks for all the reviews, but I am getting the feeling that the different conversations are getting mixed (and duplicated). So I am struggling to keep up...

Yes, I noticed that. My apologies. I dunno what happened I think I somehow started a new review when I was commenting on another thread. And then I made a similar comment twice. 😞

I'm happy to push the adjustments I tried locally that appear to resolve current issues.

Feel free to commit what you have, I believe you have the required permissions. I don't want to break anything of your work in an attempt to close some of the above threads.

I'll make those changes and resolve a few conversations.

bsweeney · 2023-11-17T02:45:43Z

OK I've added changes that handle recursion plus improve variable resolution dependency. @Neograph734 you'll want to re-clone or hard reset your local repo since I rewrote the history to avoid potential complications with merging (after I royally flubbed it earlier).

The dependency logic is kinda kludgey, but I was having trouble finding a better way to handle it. I have a vague idea about how to improve the dependency handling in set_prop but I don't think I'm able to sufficiently articulate it right now. Plus the direction I'm thinking would require a change to the internal structure of the Style class related to specified properties.

I added some test cases to cover some scenarios that weren't working previously. The document I was using to test things follows.

sample document

<style>
    :root {
        --color: orange;
    }
</style>


<style>
    .border {
        font-size: var(--color);
        border: 2px solid var(--color);
        --color: green;
    }
    .inherit {
        border: inherit;
    }
</style>
<p class="border"><span class="inherit">inherit (green)</span></p>

<style>
    .merge {
        border: 2px solid var(--color);
        --color: green;
    }
    p.merge {
        --color: blue;
    }
</style>
<p class="merge">merge (blue)</p>

<style>
    .specific {
        border: 2px solid var(--color);
        border-color: red;
        --color: blue;
    }
</style>
<p class="specific">specific (red)</p>

<style>
    .specific2 {
        border: 2px solid var(--color);
        border-top-color: red;
        --color: blue;
    }
</style>
<p class="specific2">specific2 (blue, red top)</p>

<style>
    .specific3 {
        border-top-color: red;
        border: 2px solid var(--color);
        --color: blue;
    }
</style>
<p class="specific3">specific3 (blue)</p>

<style>
    .twice {
        border: 2px solid var(--color);
        --color: red;
        --color: purple;
    }
</style>
<p class="twice">twice (purple)</p>

<style>
    .multiple {
        border: 2px var(--style) var(--color);
        --style: dashed;
    }
</style>
<p class="multiple">multiple (orange dashed)</p>

<style>
    .nested {
        border: var(--border-specification);
        --border-specification: 2px solid var(--bg);
        --bg: #ff0000FF;
    }
</style>
<p class="nested">nested (red)</p>

<style>
    .important-prop {
        --color: red;
        border: 2px solid var(--color) !important;
    }
    p.important-prop {
        border-color: green;
    }
</style>
<p class="important-prop">important prop (red)</p>

<style>
    .important-var {
        --color: blue !important;
    }
    p.important-var {
        border: 2px solid var(--color);
        --color: green;
    }
</style>
<p class="important-var">important var (blue)</p>

<style>
    .undef {
        font-size: var(--undef);
    }
</style>
<p class="bad">bad (none)</p>

<style>
    .unset {
        --color: unset;
        border: 2px solid var(--color);
    }
</style>
<p class="unset">unset (orange)</p>

<style>
    .initial {
        --color: initial;
        border: 2px solid var(--color);
    }
</style>
<p class="initial">initial (none)</p>

<style>
    .recurse {
        --one: var(--two);
        --two: var(--one);
    }
</style>

<style>
    .recurse-with-fallback {
        --one: var(--two);
        --two: var(--one, black);
    }
</style>

<style>
    .recurse-with-recursive-fallback {
        --one: var(--two, var(--two));
        --two: var(--one, var(--one));
    }
</style>

<style>
    .recurse-self {
        --one: var(--one, black);
    }
</style>

<style>
    .recurse-self-with-fallback {
        --one: var(--one, black);
    }
</style>

<style>
    .recurse-self-with-recursive-self-fallback {
        --one: var(--one, var(--one));
    }
</style>

<style>
    .recurse-self-with-recursive-self-fallback-with-fallback {
        --one: var(--one, var(--one, black));
    }
</style>

src/Css/Stylesheet.php

…been updated. Border tests still fail because the shorthand values are never computed and parse_var() is never called.

Neograph734 · 2023-11-22T19:56:12Z

I have just implemented this in my development environment. This obviously contains much more CSS than the test cases, but I must say that it appears to work quite good. My color variables all neatly resolve. Thanks for giving this the last push Brian!

By the way, I cannot find that last requested change that github is complaining about...

bsweeney · 2023-11-22T22:27:20Z

Glad it's working well for you as well.

I think that change request is just because I haven't updated my review so it's still reporting that I have requested changes. I added an approval review to make that go away. I have some other things I need to work on so I'll let this incubate a little longer in case anybody else has suggestions.

Mellthas

Don’t have the time for a full review currently, so just some minor review comments for now.

src/Css/Style.php

- fixes important property check - improves handling of important properties with regard to variable resolution - improves unset and initial keyword handling for variables

Neograph734 · 2023-12-18T19:53:43Z

I was wondering, we have introduced a lot of different variants of:

substr($prop, 0, 2) === "--"
substr($prop, 0, 2) !== "--")
!substr($prop, 0, 2) == "--"

I am tempted to create a regex pattern for it, to bring it more in line with the rest of the code. At the same time I always learned that regex is slow. I am not sure if performance or code readbility is more important here. A small is_custom_property() function to wrap substr might also work?

Similarly, on line 1559 of Style.php we might want to use the self::CSS_VAR pattern instead of strpos($val, 'var(') !== false)?

bsweeney · 2024-01-08T13:13:06Z

I am tempted to create a regex pattern for it, to bring it more in line with the rest of the code. At the same time I always learned that regex is slow. I am not sure if performance or code readbility is more important here. A small is_custom_property() function to wrap substr might also work?

Performance is always top of mind, but I think we're safe adding a wrapper function to improve maintainability. I would expect that would be a compiler optimizable call.

Similarly, on line 1559 of Style.php we might want to use the self::CSS_VAR pattern instead of strpos($val, 'var(') !== false)?

Yes, that makes sense. We should use consistent logic to ensure that the code is more maintainable.

src/Css/Style.php

bsweeney · 2024-02-02T13:56:55Z

I think we're brought this far enough, though I'm sure we could continue to optimize. I'll merge next week if no further updates or feedback are forthcoming.

Neograph734 · 2024-02-02T14:46:59Z

Great news, much appreciated!
I have nothing to add anymore.

bsweeney added this to the 2.1.1 milestone Nov 3, 2023

bsweeney mentioned this pull request Nov 5, 2023

Implement css vars #3255

Closed

bsweeney requested changes Nov 5, 2023

View reviewed changes

Mellthas reviewed Nov 6, 2023

View reviewed changes

src/Css/Style.php Outdated Show resolved Hide resolved

bsweeney requested changes Nov 6, 2023

View reviewed changes

src/Css/Style.php Show resolved Hide resolved

src/Css/Style.php Outdated Show resolved Hide resolved

src/Css/Style.php Outdated Show resolved Hide resolved

src/Css/Style.php Show resolved Hide resolved

Mellthas reviewed Nov 6, 2023

View reviewed changes

src/Css/Style.php Outdated Show resolved Hide resolved

Mellthas mentioned this pull request Nov 6, 2023

Style: Optimize logic for properties that inherit by default #3322

Merged

bsweeney requested changes Nov 7, 2023

View reviewed changes

src/Css/Style.php Outdated Show resolved Hide resolved

Mellthas mentioned this pull request Nov 8, 2023

Simplify regex for parsing property sections #3323

Merged

bsweeney force-pushed the master branch 4 times, most recently from dbff59f to 9a52a8b Compare November 17, 2023 02:39

Mellthas reviewed Nov 17, 2023

View reviewed changes

src/Css/Stylesheet.php Outdated Show resolved Hide resolved

Neograph734 and others added 9 commits November 18, 2023 10:00

Support CSS custom properties.

c260f65

Change tabs to spaces.

b00c42a

Comment out failing tests with commas.

5e58cb0

Add a todo to the var inheritance test.

ecc48fe

Fix coding standards.

aec63aa

Use null coalescing operator

ee962c5

Pushing progress. Utilizing inheritance and dependencies. Tests have …

2a476d1

…been updated. Border tests still fail because the shorthand values are never computed and parse_var() is never called.

Regression fix.

c1ceb32

Pushing some minor changes from code review.

9143689

Improve variable dependency and recursion handling

c7fbd49

bsweeney force-pushed the master branch from 9a52a8b to c7fbd49 Compare November 18, 2023 15:02

bsweeney approved these changes Nov 22, 2023

View reviewed changes

bsweeney modified the milestones: 2.1.1, 2.1.0 Nov 27, 2023

Mellthas requested changes Dec 5, 2023

View reviewed changes

src/Css/Style.php Outdated Show resolved Hide resolved

src/Css/Style.php Show resolved Hide resolved

src/Css/Style.php Outdated Show resolved Hide resolved

src/Css/Style.php Outdated Show resolved Hide resolved

CSS variable parsing updates

cb8981b

- fixes important property check - improves handling of important properties with regard to variable resolution - improves unset and initial keyword handling for variables

Neograph734 and others added 2 commits January 15, 2024 22:54

Merge branch 'dompdf:master' into master

862f536

More consistent logic.

9454735

bsweeney reviewed Jan 16, 2024

View reviewed changes

src/Css/Style.php Outdated Show resolved Hide resolved

Neograph734 added 2 commits January 16, 2024 21:37

More consistent logic (missed call).

db33aee

Fixing code sniffer errors.

962c7bc

bsweeney linked an issue Feb 17, 2024 that may be closed by this pull request

CSS variables #1872

Closed

bsweeney merged commit e1d262b into dompdf:master Feb 17, 2024
11 checks passed

bsweeney mentioned this pull request Mar 5, 2024

Style::compute_prop throwing exception if prop is a custom variable with a value of inherit #3410

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Another take on implementing CSS variables (custom properties) #3314

Another take on implementing CSS variables (custom properties) #3314

Neograph734 commented Nov 2, 2023

Neograph734 commented Nov 3, 2023

Mellthas commented Nov 5, 2023

bsweeney commented Nov 5, 2023

bsweeney left a comment

Neograph734 commented Nov 6, 2023

Neograph734 commented Nov 8, 2023 •

edited

bsweeney commented Nov 8, 2023

bsweeney commented Nov 17, 2023 •

edited

Neograph734 commented Nov 22, 2023

bsweeney commented Nov 22, 2023

Mellthas left a comment

Neograph734 commented Dec 18, 2023 •

edited

bsweeney commented Jan 8, 2024

bsweeney commented Feb 2, 2024

Neograph734 commented Feb 2, 2024

Another take on implementing CSS variables (custom properties) #3314

Another take on implementing CSS variables (custom properties) #3314

Conversation

Neograph734 commented Nov 2, 2023

Neograph734 commented Nov 3, 2023

Mellthas commented Nov 5, 2023

bsweeney commented Nov 5, 2023

bsweeney left a comment

Choose a reason for hiding this comment

Neograph734 commented Nov 6, 2023

Neograph734 commented Nov 8, 2023 • edited

bsweeney commented Nov 8, 2023

bsweeney commented Nov 17, 2023 • edited

Neograph734 commented Nov 22, 2023

bsweeney commented Nov 22, 2023

Mellthas left a comment

Choose a reason for hiding this comment

Neograph734 commented Dec 18, 2023 • edited

bsweeney commented Jan 8, 2024

bsweeney commented Feb 2, 2024

Neograph734 commented Feb 2, 2024

Neograph734 commented Nov 8, 2023 •

edited

bsweeney commented Nov 17, 2023 •

edited

Neograph734 commented Dec 18, 2023 •

edited