svgdotjs / svg.js

The lightweight library for manipulating and animating SVG
https://svgjs.dev
Other
11.04k stars 1.07k forks source link

Text.tspan("xy").newLine(): undocumented and unexpected behavior #1088

Closed d4h0 closed 1 year ago

d4h0 commented 4 years ago

Today I experimented with text in svg.js and it seems newLine() doesn't have enough documentation (and the behavior was unexpected for me).

Here is the example from the documentation for Text:

var text = draw.text(function(add) {
  add.tspan('Lorem ipsum dolor sit amet ').newLine()
  add.tspan('consectetur').fill('#f06')
  add.tspan('.')
  add.tspan('Cras sodales imperdiet auctor.').newLine().dx(20)
  add.tspan('Nunc ultrices lectus at erat').newLine()
  add.tspan('dictum pharetra elementum ante').newLine()
})

I'd expect the following regarding the first three spans:

Lorem ipsum dolor sit amet
consectetur.

But the result is:

Lorem ipsum dolor sit amet consectetur.

To me (and probably many other people) 'newLine' means "insert new line character" or "break line". But it seems 'newLine' here means "create new line with this tspan".

I think another name than 'newLine' would have been better here to reduce confusion (for example 'toNewLine' or 'asNewLine').

Even more strange is that a tspan without 'newLine' at the end doesn't show up in the visible area of the SVG. Why does this happen?

I think the documentation needs to be updated to explain what 'newLine' actually does, and how tspans after a 'newLine' behave.

Besides, that, wouldn't there be other APIs that are less confusing?

For example, if 'Text' had a 'newLine' function, we could do the following:

var text = draw.text(function(add) {
  add.newLine(
    add.tspan('Lorem ipsum dolor sit amet ')
    add.tspan('consectetur').fill('#f06')
    add.tspan('.')
  )

  // Or:

  add.newLine().dx(20).tspan('Cras sodales imperdiet auctor.')
  add
    .newLine()
    .tspan('Nunc ultrices lectus at erat')

  // Or

  add.newLine('dictum pharetra elementum ante')
})

I don't know if this would be easy to archive, but to me, this seems like a much more userfriendly and intuitive API.

What do you think?

Fuzzyma commented 4 years ago

Yes maybe the naming is a bit off. The json representation of this node marks a line as "newLined" which would be already a better naming. The way newlines are realized is best done by having this method on a tspan that starts a new line. This might be a bit odd but everything else might get out of hand. Beside that, your last syntax proposal looks like it is easily implementable. And since it does not breaks any old behavior we could even ship it as a feature. In that case newLine() would be a fancy way of creating a new tspan with newLine already called on it. You could then somehow combine it with the first approach (which btw is full of syntax errors :D):

add.newLine(function(line) {
  line.tspan(...)
})

This would then lead to nested tspans which is allowed by the specs. However, calling newLine on those tspans wouldnt have any effect.

d4h0 commented 4 years ago

Beside that, your last syntax proposal looks like it is easily implementable. And since it does not breaks any old behavior we could even ship it as a feature.

Sounds good.

So basically, the new 'newLine' would be just a wrapper around 'tspan' which calls 'newLine' and returns the tspan, correct?

Somehow I think 'newLine' as the name for this new method would be confusing (because now there would be two methods (on 'Text' and 'tspan') with the same name but different behauviour, if I understand everything correctly).

Maybe just 'line' would be okay?

In this context, I'd prefer this name anyway.

This would then lead to nested tspans which is allowed by the specs. However, calling newLine on those tspans wouldn't have any effect.

Would 'newLine' on inner tspans automatically have no effect, or would there need to be a check in 'newLine'?

Fuzzyma commented 4 years ago

So basically, the new 'newLine' would be just a wrapper around 'tspan' which calls 'newLine' and returns the tspan, correct?

correct!

Somehow I think 'newLine' as the name for this new method would be confusing (because now there would be two methods (on 'Text' and 'tspan') with the same name but different behauviour, if I understand everything correctly).

I agree!

Would 'newLine' on inner tspans automatically have no effect, or would there need to be a check in 'newLine'?

When building our lines with svg.js we only check the first level of tspans. So calling newLine on a deeper nested tspan would just not recognized by the function building the lines. It is silently ignored. Therefore nothing "bad" happens. A check is unneccesary.

d4h0 commented 4 years ago

Okay, sound pretty simple.

Will you implement this?

Or should I give it a try? :)

Fuzzyma commented 4 years ago

Well you can if you want. But only if you see any benefit for you with that. I am in the process of creating a new release anyway atm. So it might be faster when I do it instead of reviewing a PR forth and back :D

d4h0 commented 4 years ago

So it might be faster when I do it instead of reviewing a PR forth and back :D

I agree, and there wouldn't be a significant benefit for me personally (now that I know how 'newline()' works).

Thanks for your work :)

markocrni commented 2 years ago

I comment in this section, but this is probably a problem for itself, and again it refers to tspans, and their dx value. I've been working with svg that isn't attached to the DOM, and I've come to the conclusion that this part is causing the problem (tspan): const fontSize = globals.window.getComputedStyle(this.node) Since SVG is not part of DOM, getComputedStyle cannot read font size and value always will be 0. Setting value by .dx(20) also doesn't work.