Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
alexandernst opened this issue May 29, 2024 · 13 comments
Closed

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

alexandernst opened this issue May 29, 2024 · 13 comments
Labels

Comments

@alexandernst
Copy link
Contributor

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):

Grabacion.de.pantalla.2024-05-29.a.las.14.44.31.mov

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

@alexandernst alexandernst changed the title [Bug]: JJ4 textWrap behaves differently from JJ3 [Bug]: JJ4 textWrap behaves differently from JJ3 (possible race condition) May 29, 2024
@kumilingus
Copy link
Contributor

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
Copy link
Contributor Author

🤔 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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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:

  • there is a text (preloader) that is using the font-related styles
  • the "desired action" (the diagram being rendered) is delayed 5 seconds, which gives plenty amount of time for the browser to finish loading all styles.

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
Copy link
Contributor

kumilingus commented May 30, 2024

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
Copy link
Contributor Author

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

@kumilingus
Copy link
Contributor

Done.
And here's a fix draft.

@alexandernst
Copy link
Contributor Author

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 kumilingus changed the title [Bug]: JJ4 textWrap behaves differently from JJ3 (possible race condition) [Bug]: texWrap does not take SVG attributes into account May 31, 2024
@kumilingus
Copy link
Contributor

Fixed in v4.0.4.

@alexandernst
Copy link
Contributor Author

Fixed in v4.0.4.

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

@kumilingus
Copy link
Contributor

Well, thank you for the bug report 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants