clientIO / joint

A proven SVG-based JavaScript diagramming library powering exceptional UIs
https://jointjs.com
Mozilla Public License 2.0
4.73k stars 853 forks source link

[Bug]: texWrap does not take SVG attributes into account #2675

Closed alexandernst closed 6 months ago

alexandernst commented 6 months ago

Current versus expected behaviour

Hi Roman!

After upgrading to JJ4 I have been investigating multiple issues with text being wrapped "incorrectly" or "differently" from how they were wrapper in JJ3. I made a small demo that can be used to repro the bug: https://codesandbox.io/p/sandbox/jj4-custom-widget-bug-fymtlv

image

I did saw the earlyCalcEvaluation and legacyAttributes sections of the JJ4 migration guide, but I haven't found the reason why that would matter in my case / code.

Further debugging of the bug has led me to believe there is actually some sort of race condition bug in JJ4, because reloading the POC multiple times will sometimes render the text correctly (without wrapping it):

https://github.com/clientIO/joint/assets/89727/486d987c-c68b-4a67-ab0f-f7df1dccf09c

I have consistently repro the bug in FF, Safari and Chrome.

Steps to reproduce

  1. Open https://codesandbox.io/p/sandbox/jj4-custom-widget-bug-fymtlv
  2. Text in JJ4 is wrapped (it shouldn't be wrapper; compare with JJ3)

Version

4.0.3

What browsers are you seeing the problem on?

Firefox, Chrome, Safari

What operating system are you seeing the problem on?

Mac

kumilingus commented 6 months ago

I don't think this is a bug, nor regression (note that v4.0.2 reads the font from the external CSS which could cause the different behavior).

This is a race condition - which comes first - loading the font or rendering the diagram. You need to make sure to start rendering your diagram after the font is loaded.

See StackOverflow. Our flowchart example, where we force the font to be loaded.

alexandernst commented 6 months ago

🤔 I don't think this is the case. I updated the POC in order to pre-load the font and then render the diagram(s). I wait 5s to give plenty of time the browser to finish loading all the assets (styles and fonts). The behaviour is still the same. Furthermore, even without the "wait", JJ3.7 is consistent at rendering the text correctly, while JJ4 inconsistently wraps the text.

I understand your suggestion that it might be because the diagram is being rendered before the font is completely loaded, but that doesn't explain why JJ3.7 is consistently behaving correctly. Shouldn't it be hit by the case problem as JJ4?

kumilingus commented 6 months ago

If you want to test JointJS v3 you need to set the font-family as SVG attribute (not using external CSS).

The support for external CSS was introduced in 4.0.2.

If you install @joint/core": "4.0.1" you should see no difference.

alexandernst commented 6 months ago

Hmmm 🤔

I think I'm missing something (as in "I'm not sure I fully understand this"). I tested with 4.0.1 and indeed the "bug" doesn't occur. So... in my POC, the JJ4 code is doing what actually is supposed to happen, and the JJ3.7 is not wrapping the text just because it happens to fit in the width of that element (without taking into account the size of the font that is yet-to-be-applied). Is that correct?

If so, then I guess my textWrap attributes are incorrect (or maybe I misunderstood the docs).

The width of the entire box is 100px. The title element is defined as so:

title: {
    textAnchor: "middle",
    x: 50,
    width: 92,
    textWrap: {
        width: 92,
        ellipsis: true,
    }
}

There should be 4px of unused space on each side of the text, which means that the text element should have 92px width, and the text wrap should take 100% of these 92px, right?

image

If "yes", then the text "Abcdefg hijkl" should fit inside the label, but it doesn't. And I'm not really sure why, since I'm pre-loading the text (the same way you're on the flowchart demo), and I'm also delaying the execution of the JJ code. Why isn't the textWrap able to properly detect the size of my font?

kumilingus commented 6 months ago

You understand correctly.

This is what usually causes this issue:

The textWrap detects the font-family correctly. But when it measure the text in order to break it, the font is not ready yet. The breaking is done based on the default font.

When the browser finishes loading the font, it updates the text again. Since the new font is narrower, an extra space around the text is created.

This is what I think is happening:

Since there is no external font being loaded in your example, I assume the problem is that you load your CSS from <body/>.

See https://stackoverflow.com/a/1642259/2921495.

alexandernst commented 6 months ago

Hi Roman!

Ok, so the textWrap is correctly setup, nice to know!

I read the link, but it basically states that styles might not have been loaded by the time "the desired action is performed", which I think is not my case, because:

Nevertheless, I created a new demo that specifically loads the styles in the <head> and then renders the diagram, and I'm still getting the same result: https://codesandbox.io/p/sandbox/jj4-bug-tetxwrap-q3kn5p

There must be something else affecting the way JJ is calculating the wrap, but I can't figure it out.

kumilingus commented 6 months ago

It is a bug 😃

The font-size SVG attribute is set on the node after the text break calculation is done.

It computes the breaks with font-size 16px but later the font size is changed to 12.


Here's an updated codesandbox that shows the issue more clearly.

alexandernst commented 6 months ago

I think maybe the codesandbox link is private? I can't see it 👀

kumilingus commented 6 months ago

Done. And here's a fix draft.

alexandernst commented 6 months ago

phew! Nice to see you saw the bug! I was that close 🤏 to assuming that it is a bug somewhere in my code and that I'd have to keep digging 😂

So... JJ 4.0.4 release in the next ~10 minutes? 🤪

kumilingus commented 6 months ago

Fixed in v4.0.4.

alexandernst commented 6 months ago

Fixed in v4.0.4.

Did I say "~10min"? It was a typo, I meant to say "~10hours" 😏

kumilingus commented 6 months ago

Well, thank you for the bug report 😉