Fix: Add support for Css variables by TOKdawn · Pull Request #209 · yWorks/svg2pdf.js

TOKdawn · 2022-04-22T10:37:29Z

Hi,

I add a support for Css variables

I analyzed cssValue then capture with /var\(.*?\)/gi

Processing with function getCssValue().

Best regards

Fixes ##206

Fix: Optimized code structure Optimized code structure Fix: Add support for Css variables

HackbrettXXX

Thanks for the PR. Please add some test cases. I'll have a closer look at the code afterwards.

TOKdawn · 2022-04-24T05:07:56Z

Thanks for the PR. Please add some test cases. I'll have a closer look at the code afterwards.

hi,
I add the css-variables into test-unit;
But i can’t adopt test;
I think it may be because of my reference.pdf version is jsPDF2.00;
I test in /index.html is correct,and can normal download;
I hope you can test it;
Best regards

HackbrettXXX

Thanks for adding the test case. I think the approach is a bit flawed: the lookup for a variable value includes all available CSS rule. However, it should only include the rules that apply to the current element.

HackbrettXXX · 2022-04-25T08:24:22Z

test/specs/css-variables/spec.svg

+  <style>.bluePolyline {stroke: var(--color); } .polyLineColors {--color:  blue;}</style>
+  <style>.redPolyline { --colorRed: red; stroke: var(--colorRed);}</style>
+  <style>.greenPolyline { stroke: var(--colorGreen);}</style>


Just nitpicking, but "textColors" would probably be more understandable than "polylineColors" here.

Yep , I just copy code from issues. not rename class.
I will modify them

You’re right, maybe I should rework my code withelement.style.getPropertyValue(“—my-var”);
and add support for custom property fallback values.
I will try to fix it.
Thank you for your review.

I haven't checked the code too much,
I don't know if I can use the DOM API to get the current element style or analytic CSS-variables key,
if there is no DOM, maybe I can only get the CSS-variables key through Regular Expression.

You can use the DOM API.

HackbrettXXX · 2022-04-25T08:31:05Z

test/specs/css-variables/spec.svg

+  <text class="green greenPolyline" y="20" x="20">Value With Space</text>
+  <text y="40" x="20" class="rgb" >RGB Color</text>
+  <text y="60" x="20" class="space">Var With Space </text>
+


Please add a test case like this

<style>.variable { stroke: var(--color); }</style> <style>.red { --color red; }</style> <style>.green { --color green; }</style> <text class="red variable">Red</text> <text class="green variable">Green</text>

And a couple of tests that test the inheritance of the variables.
Also add a test case where the variable is not defined.
Add test cases for CSS specificity.

Sorry, this is my first PR, not well thought out, I will add and test them

Hi,I fix all bug.and add many test cases.
Looking forward to your review.
QvQ

… defined

HackbrettXXX

Sorry for the delay, I was really busy. Thanks for the changes.

HackbrettXXX · 2022-05-06T10:20:55Z

test/specs/css-variables/spec.svg

+
+  <style>.TestValue { stroke: var(--colorGreen);}</style>
+  <style>.green {--colorGreen:   #009900 }</style>
+  <text class="green greenPolyline" y="20" x="20">Test Value With Space</text>


greenPolyline seems to be a leftover.

HackbrettXXX · 2022-05-06T10:26:23Z

src/context/stylesheets.ts

+  getCssValue(selector: string): string | undefined {
+    const value: string = selector.replace(/^\s+|\s+$/g, '')
+    this.cssVariableList = []
+    if (this.analysisCssVariables(value, 0)) {
+    }
+    return undefined
+  }


Seems to be unused?

HackbrettXXX · 2022-05-06T11:50:41Z

src/context/stylesheets.ts

+            valueIndex = 0
+            valueName = ''
+            //exclude 'var(' and ')' form cssValueString
+            for (let ch = 0; ch < valueString.length; ch++) {
+              if (valueString[ch] == 'v' && valueString.slice(ch, ch + 4) == 'var(') {
+                valueIndex++
+                ch += 3
+                continue
+              }
+              if (valueString[ch] == ')' && valueIndex > 0) {
+                valueIndex--
+                continue
+              }
+              valueName += valueString[ch]
+            }


Why do I need this loop? Couldn't we just adjust the slice call?

The value maybe like this var(--colorOne,var(--colorTwo,rgb(0,0,0))).
I was thinking of removing var( and ),then split with ,
Do you have a better solution?

Hi!
I refactored my code,
reduced the loop level,
reduced unnecessary calculations,
Put the code in a more logical place.
Looking forward to your review.

HackbrettXXX · 2022-05-06T11:51:21Z

src/context/stylesheets.ts

+              originalCss: valueString,
+              valueName: valueName.split(varReg)
+            })
+            return this.analysisCssVariables(selector, untreatedIndex + varEnd)


I don't like the recursive approach. A loop would probably be more readable and better performance-wise.

OK,I'll change

HackbrettXXX · 2022-05-06T11:52:51Z

src/context/stylesheets.ts

  private readonly loadExternalSheets: boolean
  private readonly styleSheets: CSSStyleSheet[]
-
+  private cssVariableList: CssVariable[]


I don't think we need the field. Just return the list from analysisCssVariables (or pass a local list as 'out' variable).

HackbrettXXX · 2022-05-06T11:55:48Z

src/context/stylesheets.ts

+    const cssValue: string = mostSpecificRule.style.getPropertyValue(propertyCss)
+    const varReg = /var\(.*?\)/gi
+    if (cssValue && varReg.test(cssValue)) {
+      const originalValue = cssValue.replace(/^\s+|\s+$/g, '')


HackbrettXXX · 2022-05-06T11:56:19Z

src/context/stylesheets.ts

+      let res = originalValue
+      this.cssVariableList = []
+      if (this.analysisCssVariables(originalValue, 0)) {
+        this.cssVariableList.map(CssVariable => {


lower case var

HackbrettXXX · 2022-05-06T11:58:24Z

src/context/stylesheets.ts

+          for (let i = 0; i < CssVariable.valueName.length; i++) {
+            const name = CssVariable.valueName[i].replace(/^\s+|\s+$/g, '')
+            if (name.slice(0, 2) == '--') {
+              const css = getComputedStyle(node).getPropertyValue(name)


getComputedStyle won't work, because the SVG is not part of document.

It's been a long time,Looking forward to your review.

Sadly, @HackbrettXXX has not been available in recent months. He will get back to you, I am very sure, once he is back to work. Please allow him a bit more time.

OK,I'll be waiting.

HackbrettXXX

I'm back and finally have time to review this again. Sorry for the very long delay.

Please add still more test cases (like mentioned in my previous comment). Especially for CSS specificity and my example test case.

Please also try to make the code more readable, extract some functions, make the loop more readable, etc.

Fix: Add support for Css variables

d2998f7

Fix: Optimized code structure Optimized code structure Fix: Add support for Css variables

TOKdawn force-pushed the fix-supportCssVariables branch from 5e88066 to d2998f7 Compare April 22, 2022 11:27

HackbrettXXX requested changes Apr 22, 2022

View reviewed changes

Fix: Add test-unit for css-variables

d3d9f20

HackbrettXXX requested changes Apr 25, 2022

View reviewed changes

fix: add inheritance of the css variables and fix css variable is not…

40aa0aa

… defined

HackbrettXXX requested changes May 6, 2022

View reviewed changes

TOKdawn added 2 commits May 10, 2022 17:30

Fix: Add support for Css variables

5df932c

Fix: Add support for Css variables

fbdfebc

HackbrettXXX requested changes Aug 10, 2023

View reviewed changes

Merge branch 'yWorks:master' into fix-supportCssVariables

bbbc90f

HackbrettXXX marked this pull request as draft December 11, 2023 09:14

yWorks deleted a comment from enersis-pst Feb 7, 2024

Conversation

TOKdawn commented Apr 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HackbrettXXX left a comment

Choose a reason for hiding this comment

Uh oh!

TOKdawn commented Apr 24, 2022

Uh oh!

HackbrettXXX left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TOKdawn Apr 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HackbrettXXX left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HackbrettXXX left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

TOKdawn commented Apr 22, 2022 •

edited

Loading

TOKdawn Apr 25, 2022 •

edited

Loading