Implement textLength support #160

Mrfence97 · 2020-11-09T20:13:48Z

This PR adds support for the textLength attribute for <text> nodes, solving issue #69.

The length is set by inserting appropriate spaces of length charSpace between characters. To calculate charSpace, the length ('default length') of the text is calculated using measureTextWidth() as if textLength wasn't applied.

The implementation when the <text> node just contains plain text is straightforward. However, if <tspan>s are present, the implementation is more complicated to account for e.g. different font sizes, as the default length must be calculated for each <tspan> at a time and then summed. Note this means that TextChunks aren't immediately put() to the PDF when a new chunk starts, instead they are stored in an array so these width calculations can occur beforehand.

Note textLength implementations differ between browsers, this PR follows the Firefox implementation:

Firefox
- textLength is applied to a <text> element and all child <tspans> are scaled to fit within the 'global' length.
- setting dx or an absolute x on a <tspan> has no impact on the charSpace.
- if xml:space="default" any space/tab/newline characters present at the very start of the text are treated as a single character when calculating charSpace, but not rendered as such.
Chrome (not implemented, only for reference)
- textLength is applied to individual <tspan>s.
- setting dx or an absolute x impacts charSpace as the 'default length' is increased by the dx amount.
- if xml:space="default" any space/tab/newline characters present are trimmed and not displayed or factored into the charSpace calculation.

- `textLength` implementations differ between browsers, this uses the Firefox convention where it is applied to a `<text>` element (and _not_ to individual `<tspan>`s) - Compile time flag sets whether to use the Firefox convention of inserting a charSpace after the final character, or the Chrome convention of not doing so. Currently uses Chrome convention. - Adds a new unit test also

- If `xml:space="default"` then treats any prefixed 'space-like' characters as being a single character of length 1 when calculating charSpace - Also include the updated `tests.js` file

Mrfence97 · 2020-11-09T20:16:11Z

src/nodes/text.ts

          }
-          if (i === tSpanCount - 1) {
+          if (i === this.element.childNodes.length - 1) {


I think this line was originally a bug as this.element.childElementCount (tSpanCount) doesn't always equal this.element.childNodes.length. Given this loop is over this.element.childNodes, it makes sense to use the latter when checking for the last element in the array!

I think this is what is causing the tests to fail, however.

I agree, this should be this.element.childNodes.length - 1. The tests still fail, though, if I change this back to tSpanCount. Could you investigate why? If the changes are only minor floating point differences, you may override the reference PDF files.

HackbrettXXX

Thanks for this PR! A couple of things:

The textLength attribute is also allowed on tSpan elements. Did you consider this? I think this is not too important for now, but if you have time you may have a look at this.
Could you add test cases with x and y attributes on tSpans?
Maybe we should add a comment to the spec.svg that Chrome doesn't render this file correctly. As far as I can see, Firefox does a better job.

HackbrettXXX · 2020-11-10T11:05:13Z

src/nodes/text.ts

@@ -123,8 +150,11 @@ export class TextNode extends GraphicsNode {
        if (xmlSpace === 'default') {
          if (i === 0) {
            trimmedText = trimLeft(trimmedText)
+            if (originalText.match(/^[(\n\r)|(\t)| ]/)) {


I think this regex and the others should be /^[\n\r\t ]/ or even /^\s/

HackbrettXXX · 2020-11-10T11:57:27Z

src/nodes/text.ts

          }
-          if (i === tSpanCount - 1) {
+          if (i === this.element.childNodes.length - 1) {


I agree, this should be this.element.childNodes.length - 1. The tests still fail, though, if I change this back to tSpanCount. Could you investigate why? If the changes are only minor floating point differences, you may override the reference PDF files.

Mrfence97 · 2020-11-10T18:43:11Z

The textLength attribute is also allowed on tSpan elements. Did you consider this? I think this is not too important for now, but if you have time you may have a look at this.

Although the SVG spec states that it's allowed on <tspan>s, implementations appear to differ. Firefox simply ignores any textLengths that appear in a <tspan> and only ever considers the textLength in the parent <text> element. In Chrome, however, if a <tspan> has a textLength and has at least one of x/y/dx/dy set, then the prior content is stretched to fit the original textLength and the subsequent text is fit to the new textLength.

For example, this is how the following SVG is rendered in both Firefox and Chrome:

<rect y="0" width="200" height="20" fill="transparent" stroke="black"/>
<text textLength="200" y="20" font-family="times">Hello, <tspan y="40" textLength="100" fill="red">wor</tspan><tspan fill="yellow">ld!</tspan></text>

The PR matches Firefox and it I think it would be significant additional work to also implement the Chrome behaviour.

I'll add a test case that includes this example and also ones that set x and y. I'll also comment that it doesn't render correctly in Chrome!

I'll also verify what's causing the checks to fail, there should be no changes to any elements that don't have a textLength set so I'm assuming it'll just be floating point errors.

Mrfence97 · 2020-11-10T21:16:39Z

After resetting back to using tSpanCount, the test errors were all due to small numerical errors in four tests:

complete-diagram2
display-none-and-visibility-inheritance
title-element
xml-space

Switching back to this.element.childNodes.length - 1 a total of 10 tests fail (including those ones above). I haven't included the updated reference PDFs in the commit in case you wanted to review the differences (which will likely be more than just floating point errors). The very latest commit just adds the updated reference PDFs so you could checkout ea316e0 to look at the errors yourself.

I've also changed the regex and included a test case that sets x/y in a <tspan>.

Mrfence97 · 2020-11-10T21:55:50Z

It appears that my machine generates different reference PDFs to the test server...

I've pushed the reference PDFs that make all tests pass, but it seems the server generates different PDFs. Could this be because I'm running Windows? I've deleted and reinstalled all node packages and still all my tests pass. Strange goings on indeed.

HackbrettXXX

Alright, looks good now. I updated the references on my machine, lets see if that helps.

HackbrettXXX · 2020-11-11T10:23:31Z

Seems like it did... thanks for this PR :)

Mrfence97 · 2020-11-11T10:29:40Z

Great, thanks for merging! Is there a road-map for when the next release will be made?

HackbrettXXX · 2020-11-11T10:33:41Z

I would like to coordinate this release with the next jsPDF release, which I will hopefully be able to release within the next 2-3 weeks or so.

yGuy · 2020-11-11T10:54:23Z

@Mrfence97 - thank you very much for the great work and your contribution! 'would love to see more quality PR requests. Let us know if there is a way we can say thank you and provide you with more awesome tasks ;-)

Mrfence97 added 2 commits November 9, 2020 18:28

Fix treatment of prefixed 'space-like' characters

d2ac9a8

- If `xml:space="default"` then treats any prefixed 'space-like' characters as being a single character of length 1 when calculating charSpace - Also include the updated `tests.js` file

Mrfence97 commented Nov 9, 2020

View reviewed changes

HackbrettXXX requested changes Nov 10, 2020

View reviewed changes

Mrfence97 added 2 commits November 10, 2020 21:07

Simplify regex and add x/y attribute test case

ea316e0

Update reference PDFs due to text trimming bug-fix

008663e

Revert some reference PDFs changed by mistake

7dfbdcd

Update reference PDFs due to text trimming bug-fix

0718d6b

Mrfence97 force-pushed the feature/text-length branch from 6c88ddb to 0718d6b Compare November 10, 2020 21:58

update reference PDFs

f0ddb8a

HackbrettXXX approved these changes Nov 11, 2020

View reviewed changes

HackbrettXXX merged commit e11c4d5 into yWorks:master Nov 11, 2020

Mrfence97 deleted the feature/text-length branch November 11, 2020 10:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement textLength support #160

Implement textLength support #160

Mrfence97 commented Nov 9, 2020

Mrfence97 Nov 9, 2020 •

edited

HackbrettXXX Nov 10, 2020

HackbrettXXX left a comment

HackbrettXXX Nov 10, 2020

HackbrettXXX Nov 10, 2020

Mrfence97 commented Nov 10, 2020

Mrfence97 commented Nov 10, 2020

Mrfence97 commented Nov 10, 2020

HackbrettXXX left a comment

HackbrettXXX commented Nov 11, 2020

Mrfence97 commented Nov 11, 2020

HackbrettXXX commented Nov 11, 2020

yGuy commented Nov 11, 2020

Implement textLength support #160

Implement textLength support #160

Conversation

Mrfence97 commented Nov 9, 2020

Mrfence97 Nov 9, 2020 • edited

Choose a reason for hiding this comment

HackbrettXXX Nov 10, 2020

Choose a reason for hiding this comment

HackbrettXXX left a comment

Choose a reason for hiding this comment

HackbrettXXX Nov 10, 2020

Choose a reason for hiding this comment

HackbrettXXX Nov 10, 2020

Choose a reason for hiding this comment

Mrfence97 commented Nov 10, 2020

Mrfence97 commented Nov 10, 2020

Mrfence97 commented Nov 10, 2020

HackbrettXXX left a comment

Choose a reason for hiding this comment

HackbrettXXX commented Nov 11, 2020

Mrfence97 commented Nov 11, 2020

HackbrettXXX commented Nov 11, 2020

yGuy commented Nov 11, 2020

Mrfence97 Nov 9, 2020 •

edited