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]: textWrap is not calculating properly text size when using custom fonts / size (Bootstrap repro) #2295

Closed
alexandernst opened this issue Aug 11, 2023 · 17 comments · Fixed by #2542

Comments

@alexandernst
Copy link
Contributor

What happened?

It seems that textWrap isn't working as expected when using css styling. A simple repro: https://codesandbox.io/s/textwrap-bug-76f2md?file=/src/index.js

Note that commenting the bootstrap import fixes the issue.

image

Version

3.7.5

What browsers are you seeing the problem on?

Firefox, Chrome, Safari

What operating system are you seeing the problem on?

Mac

@kumilingus
Copy link
Contributor

Now you must explicitly set all font-related attributes in the <text> element for the textWrap function to work correctly. Back then, this decision was made consciously for performance reasons.

Now it is probably a good time to use the getComputedStyle() function to retrieve the actual font attributes needed to calculate the text wrap.

@alexandernst
Copy link
Contributor Author

@kumilingus is this going to make it in the next patch release (3.7.6?) or will it land in 4.0.0? If the later, is there any workaround that I can apply?

@kumilingus
Copy link
Contributor

Sorry, haven't made the decision yet.

A workaround is to re-define the textWrap attribute. It can be done per element, as shown below.

class MyRect extends joint.dia.Element {

    static attributes = {
        textWrap: {
            qualify: joint.util.isPlainObject,
            set: function(value, refBBox, node, attrs) {
                const computedFontSize = parseFloat(getComputedStyle(node).fontSize);
                const computedAttrs = { ...attrs };
                computedAttrs.fontSize = computedFontSize;
                joint.dia.attributes.textWrap.set.call(this, value, refBBox, node, computedAttrs);
            }
        }
    };
}

Here's updated version of your codesandbox:
https://codesandbox.io/s/textwrap-bug-forked-svgjq9?file=/src/index.js

@alexandernst
Copy link
Contributor Author

Hi! I was finally able to get to this. I tried it, but it doesn't seem to be working as expected. Resizing the cell makes the text overflow the width.

image

@kumilingus
Copy link
Contributor

The break text depends not only on the font-size, but also on font-family, font-weight, letter-spacing.

  static attributes = {
    textWrap: {
      qualify: joint.util.isPlainObject,
      set: function (value, refBBox, node, attrs) {
        const computedStyles = getComputedStyle(node);
        const fontSize = parseFloat(computedStyles.fontSize);
        const fontFamily = computedStyles.fontFamily;
        const computedAttrs = { ...attrs, fontSize, fontFamily };
        joint.dia.attributes.textWrap.set.call(
          this,
          value,
          refBBox,
          node,
          computedAttrs
        );
      }
    }
  };

Updated example: https://codesandbox.io/s/textwrap-bug-forked-svgjq9?file=/src/index.js

@alexandernst
Copy link
Contributor Author

Oh, I see 🤔. I was about to ask why / how the different attributes are handled, then I remembered I can see the code 😂. I had a look at the textWrap:set function. I see that the following attributes are taken into account when computing the width / height of the text:

  • font-size
  • font-family
  • font-weight
  • line-height
  • letter-spacing

I think the following extra attributes should be taken into account:

  • word-spacing
  • text-transform (text might be capitalized, which would change the width and height of non monospaced fonts)
  • text-indent (first line of text might be indented)
  • white-space (the handling of white spaces might affect the width)
  • text-shadow (I agree that this one might be an edge case, but shadows can affect the width)

@kumilingus
Copy link
Contributor

I think that only text-transform applies to SVG.

IMHO text-shadow does not affect the text. It only adds a shadow next to the text. But it is a matter of interpretation.

Note that there is no line-height in SVG. It's a property that we introduced in the Vectorizer.text() method.

@kumilingus
Copy link
Contributor

Fix waits for #2335. For the time being please use the workaround.

It's currently not guaranteed that the font-size is already set on the SVGNode (because it is a special attribute - because of calc()). i.e. the computed style might return incorrect font size (whatever the font size is before the font-size attribute is set).

@coyoteecd
Copy link
Contributor

Another workaround that I've used in our project for a similar problem (body style, including font family/size/weight affected the SVG text but did not get taken into account when calculating wrapping) is to set the text attributes explicitly with references to CSS variables. For me this solved the text wrapping without being forced to duplicate values between the CSS file and the TypeScript file containing the shape definition.

E.g.

{
  text: {
    fontFamily: 'var(--my-body-font-family)',
    fontWeight: 'var(--my-body-font-weight)',
    fontSize: 'var(--my-body-font-size)',
    textWrap: {
      width: '97%' // padding
    }
  }
}

Recent versions of Bootstrap already define CSS vars, so you might be able to use them as-is.

@kumilingus
Copy link
Contributor

Nice, I didn't know this is even possible: https://jsfiddle.net/kumilingus/qc9geah6/

:root {
  --rect-color: red;  
}
<svg>
  <rect width="100" height="100" fill="var(--rect-color)" />
</svg>

@alexandernst
Copy link
Contributor Author

I thought this was going to land in 4.0 since it's a breaking change 🤔
maybe in a future 4.x release?

@kumilingus
Copy link
Contributor

The necessary breaking change was made and the issue can be fixed with a patch now (4.0.x).

@zbynekstara
Copy link
Contributor

Hi @alexandernst, the fix was released as part of JointJS v4.0.2 (JointJS+ v4.0.1).

@alexandernst
Copy link
Contributor Author

Thank you @zbynekstara !!

@frnora
Copy link

frnora commented Aug 21, 2024

After this change: For our custom shapes the breakText is called with wrong params (from updateViewsAsync -> ... -> updateRelativeAttributes -> set). It is not respecting the properties set in the markup.

export default class CustomElement extends dia.Element {

    preinitialize() {
        super.preinitialize(...arguments);
        // etc.
    }

    // etc.

    defaults() {
        return {
            type: 'xyz',
            size: { width: 200, height 60 },
            attrs: {
                // etc.
                label: {
                    textAnchor: 'start',
                    text: '',
                    textWrap: {
                        width: 160,
                        ellipsis: true,
                        maxLineCount: 3
                    },
                    x: 20,
                    fontSize: 'var(--test-font-size, 14px)',
                    fontFamily: 'var(--test-font-family, sans-serif)',
                    fill: 'var(--test-fill-color, #333)'
                }
            }
        };
    }

   // etc.
}

fontSize, fontFamily from the markup are ignored, I guess they are set later than the view update is executed? That means the breakText will try to break a (e.g.) 12px font-sized default, while the --test-font-size could be set to e.g. 20px. So it won't break it and the text will overflow the element (body, or whatever).

This was working flawlessly before this change. I will write an override to take those values from the attrs as it was before unless you have a better idea. But I am not quite sure whether this is the intended behavior.

Thanks.

EDIT: forgot to mention, you can test this in your example as well, simply change the font-size to e.g. 20.
New wrapping:
Screenshot 2024-08-22 at 11 50 35
Old wrapping:
Screenshot 2024-08-22 at 11 50 43

No bueno.

@kumilingus
Copy link
Contributor

Hi @frnora , that was already fixed in https://github.com/clientIO/joint/pull/2679/files I believe.

@frnora
Copy link

frnora commented Aug 27, 2024

Hi @kumilingus, thank you, that's great to hear!

Sadly, it's not part of the latest joint+ dist, thus I can't use it. (must rely on that file) I will keep in mind to remove the override once a new version of joint+ hits.

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

Successfully merging a pull request may close this issue.

5 participants