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

Entering text in a Textbox object expands it when there aren't newlines #2376

Open
onassar opened this issue Jul 30, 2015 · 73 comments
Open
Assignees
Labels

Comments

@onassar
Copy link

onassar commented Jul 30, 2015

I've created a Textbox object with width 400px

When entering text that doesn't have any newlines, it expands the Textbox beyond 400px

Not sure if this is intended behaviour
If it is, would be great to be able to pass in a property to preserve the width and force the text to a newline

@asturur
Copy link
Member

asturur commented Jul 30, 2015

Textbox has a minimum width that is the length of the biggest word.
Textbox split lines with fixed with, using space.

So what are you asking is reverse its behaviour.

@onassar
Copy link
Author

onassar commented Jul 30, 2015

Thanks for the quick response @asturur!
My thinking is that when someone defines the bounding-box, they've decided "I only want text to appear in this area"

So if they drop in text like a link, it's expanding the box, even though they've already decided what width they'd like.

Is it possible to have it limit the width to what's defined, rather than automatically expanding it?

@asturur
Copy link
Member

asturur commented Jul 30, 2015

Maybe adding a limit to the width is possible.
But where would go the extra text?

2015-07-30 17:10 GMT+02:00 Oliver Nassar [email protected]:

Thanks for the quick response @asturur https://github.com/asturur!
My thinking is that when someone defines the bounding-box, they've decided
"I only want text to appear in this area"

So if they drop in text like a link, it's expanding the box, even though
they've already decided what width they'd like.

Is it possible to have it limit the width to what's defined, rather than
automatically expanding it?


Reply to this email directly or view it on GitHub
#2376 (comment).

@onassar
Copy link
Author

onassar commented Jul 30, 2015

The extra text would go to the next line

@asturur
Copy link
Member

asturur commented Jul 30, 2015

we need space to split words. is an acceptable requirement i think. don't you?

@onassar
Copy link
Author

onassar commented Jul 30, 2015

With html/css, you use the word-wrap: break-word property
Here's before: https://i.imgur.com/ElaH8zI.png
Here's after: https://i.imgur.com/PLkbom1.png

Here's the fiddle: https://jsfiddle.net/nkgmr06a/

@kangax
Copy link
Member

kangax commented Jul 30, 2015

There's also an option of reducing text size when it doesn't fit. That's what Apple's Keynote does, for example.

Sent from my iPad

On Jul 30, 2015, at 11:28 AM, Oliver Nassar [email protected] wrote:

With html/css, you use the word-wrap: break-word property
Here's before: https://i.imgur.com/ElaH8zI.png
Here's after: https://i.imgur.com/PLkbom1.png

Here's the fiddle: https://jsfiddle.net/nkgmr06a/


Reply to this email directly or view it on GitHub.

@travisholliday
Copy link

@kangax The reducing text size is probably the best solution. Working on trying to figure that in to an app I am making using Fabric.

@onassar
Copy link
Author

onassar commented Aug 6, 2015

@kangax what do you think? I think it makes sense for the text to wrap to the next line rather than lowering the font size (especially if the size has been explicitly defined)

@kangax
Copy link
Member

kangax commented Aug 9, 2015

@onassar I think so too; especially if the size is explicitly defined (and it always is; whether by user or through default value)

@jafferhaider @inssein what do you think?

@inssein
Copy link
Contributor

inssein commented Aug 10, 2015

I am away on vacation right now, but the behaviour that @onassar described originally is what is supposed to happen. The box has a fixed width and should never change. If that doesn't work, it's a bug I haven't seen in my implementation.

Furthermore, I think we should add the option to lower font size instead of adding lines, that was something I was hoping to implement later down the road.

@inssein
Copy link
Contributor

inssein commented Aug 10, 2015

Ah wait a minute, I just realized that there were no spaces. Currently minimum width is longest word. We could add word breaking, but I think the current implementation is already complex due to the way we store metadata about the text (formatting and style).

@onassar
Copy link
Author

onassar commented Aug 12, 2015

I think word-breaking makes sense, since that would mimic the behaviour of css (see here: https://jsfiddle.net/nkgmr06a/). How tough is something like this?

@onassar
Copy link
Author

onassar commented Aug 17, 2015

Any movement here?

@inssein
Copy link
Contributor

inssein commented Aug 17, 2015

@onassar we could certainly add a different word-wrap strategy, but I don't have the time in the near future to implement it.

@onassar
Copy link
Author

onassar commented Aug 17, 2015

Thanks @inssein
Any chance anyone else has some bandwidth? Would be happy to help test things out

@jafferhaider
Copy link
Contributor

@onassar In my original implementation I had written word wrapping; a long word would get wrapped just like other text. I'd go with that behaviour instead of reducing font size. In general I believe the text box should only wrap lines and not mess with any other property of the text entered by the user. That is what text box controls in HTML and Flash do. The Keynote behaviour that @kangax mentions makes a lot of sense within the context of a presentation making application, but I think our Textbox class should be more general purpose.

I'm on vacation this week. I haven't looked at the formatting and styling code yet which seems to have made things more complicated. I'd definitely want to fix this soon.

@kangax
Copy link
Member

kangax commented Aug 20, 2015

Sure, I'm ok with word wrapping for now

@asturur
Copy link
Member

asturur commented Aug 21, 2015

This adds word wrapping, i cannot push it because i have first to merge the stupid bug i introduced yesterday.

    _wrapLine: function(ctx, text, lineIndex) {
      var lineWidth        = 0,
          lines            = [],
          line             = '',
          words            = text.split(' '),
          word             = '',
          letter           = '',
          offset           = 0,
          infix            = ' ',
          wordWidth        = 0,
          infixWidth       = 0,
+          letterWidth      = 0,
          largestWordWidth = 0;

      for (var i = 0; i < words.length; i++) {
        word = words[i];
+        lineWidth += infixWidth;
+        if (this.breakWords) {
+          word = word.split('');
+            while (word.length) {
+            letterWidth = this._getWidthOfChar(ctx, word[0], lineIndex, offset);
+            if (lineWidth + letterWidth > this.width) {
+              lines.push(line);
+              line = '';
+              lineWidth = 0;
+            }
+            line += word.shift();
+            offset++;
+            lineWidth += letterWidth;
+          }
+        }
+        else {
          wordWidth = this._measureText(ctx, word, lineIndex, offset);
-          lineWidth += infixWidth + wordWidth;
+          lineWidth += wordWidth;
+        }

        if (lineWidth >= this.width && line !== '') {
          lines.push(line);
          line = '';
          lineWidth = wordWidth;
        }

        if (line !== '' || i === 1) {
          line += infix;
        }
        line += word;
        offset += word.length;
        infixWidth = this._getWidthOfChar(ctx, infix, lineIndex, offset);
        offset++;

        // keep track of largest word
        if (wordWidth > largestWordWidth && !this.breakWords) {
          largestWordWidth = wordWidth;
        }
      }

      i && lines.push(line);

      if (largestWordWidth > this.dynamicMinWidth) {
        this.dynamicMinWidth = largestWordWidth;
      }

      return lines;
    },

@onassar
Copy link
Author

onassar commented Aug 21, 2015

Awesome work @asturur
Happy to give it a go and test once it's merged in
Update this thread once the bug is fixed?

@jojobyte
Copy link

Based on @asturur work with a fix to only use the break word function if the word is greater than the textbox width.

fabric.Textbox.prototype._wrapLine = function(ctx, text, lineIndex) {
    var lineWidth        = 0,
        lines            = [],
        line             = '',
        words            = text.split(' '),
        word             = '',
        letter           = '',
        offset           = 0,
        infix            = ' ',
        wordWidth        = 0,
        infixWidth       = 0,
        letterWidth      = 0,
        largestWordWidth = 0;

    for (var i = 0; i < words.length; i++) {
        word = words[i];
        wordWidth = this._measureText(ctx, word, lineIndex, offset);
        lineWidth += infixWidth;

        // Break Words if wordWidth is greater than textbox width
        if (this.breakWords && wordWidth > this.width) {
            line += infix;
            var wordLetters = word.split('');
            while (wordLetters.length) {
                letterWidth = this._getWidthOfChar(ctx, wordLetters[0], lineIndex, offset);
                if (lineWidth + letterWidth > this.width) {
                    lines.push(line);
                    line = '';
                    lineWidth = 0;
                }
                line += wordLetters.shift();
                offset++;
                lineWidth += letterWidth;
            }
            word = '';
        } else {
            lineWidth += wordWidth;
        }

        if (lineWidth >= this.width && line !== '') {
            lines.push(line);
            line = '';
            lineWidth = wordWidth;
        }

        if (line !== '' || i === 1) {
            line += infix;
        }
        line += word;
        offset += word.length;
        infixWidth = this._measureText(ctx, infix, lineIndex, offset);
        offset++;

        // keep track of largest word
        if (wordWidth > largestWordWidth && !this.breakWords) {
            largestWordWidth = wordWidth;
        }
    }

    i && lines.push(line);

    if (largestWordWidth > this.dynamicMinWidth) {
        this.dynamicMinWidth = largestWordWidth;
    }

    return lines;
};

Usage:

var breakingTextbox = new fabric.Textbox(longText, {
        width: 200,
        breakWords: true
});

Demo:
https://jsfiddle.net/jojobyte/3j352toh/1/

@asturur
Copy link
Member

asturur commented Sep 18, 2015

i think that css propery breakwords, break the words always and they wanted a similar behavior.
I do not really like word breaking.

@jojobyte
Copy link

Depends on the use case. Its required for our use-case (fixed width textbox in canvas that the text should remain in, its okay if it goes beyond the height, but not the width) and we were looking at dropping Fabric entirely because it didnt provide this, which would have been a shame considering how many awesome features it provides.

This provides similar functionality to the word-wrap: break-word, the difference between it and your original code is that it prioritizes breaking at space first, then breaking the word only if it has to.

@asturur
Copy link
Member

asturur commented Sep 18, 2015

Did you manage to solve the multiple space issue with textbox?

@jojobyte
Copy link

What multiple space issue?

@asturur
Copy link
Member

asturur commented Sep 18, 2015

using word break having mulitple spaces between words always gave me weird
results with cursor position, even more with with textAlign justify.

2015-09-18 9:26 GMT+02:00 Jordan Hess [email protected]:

What multiple space issue?


Reply to this email directly or view it on GitHub
#2376 (comment).

@jojobyte
Copy link

Ohyeah, cursor is full on broken in the Textbox, but thats even without wordbreak, although we are using Google Webfonts inside it.

Don't think the cursor issues are specifically wordbreak or space related, something else is going on. Haven't had time to look into the cursor problem myself yet, Was hoping someone else had already been looking into it and had code that was close like yours.

@asturur
Copy link
Member

asturur commented Sep 18, 2015

lets talk about normal browser integrates fonts.
it should behave normally but not very good with justify and more than one spaces.

@onassar
Copy link
Author

onassar commented Oct 6, 2015

Any progress here by chance?

@Hellowor1d
Copy link

This would be awesome! Unfortunately the fiddles contains a few bugs.

  1. Overflowing text is pushed to start at the row above ignoring that rows space, ideally it should stay on it's own row and break word from there.
  2. The cursor is not in the correct position, one character to the right, most likely because of the ignored space.
  3. Selection "races" ahead of mouse position increasingly with the amount of text selected.

Here's a screenshot of what I mean:

skarmavbild 2018-06-13 kl 15 06 57

@jjwilly16

@jjwilly16
Copy link

jjwilly16 commented Feb 5, 2019

If anyone is still interested in this feature, I think I have some of the above issues fixed. I've updated my fiddle https://jsfiddle.net/jjwilly16/sd03g4t4/. This will work with version 2.6. Let me know if it still sucks

@Tsourdox
Copy link

@jjwilly16 awesome, found this which seems a bit off:
skarmavbild 2019-02-19 kl 09 26 26

@Tsourdox
Copy link

Might this already been fixed in this update? #5402

@jjwilly16
Copy link

@Tsourdox Yeah, I thought #5479 was a possible fix for this, but I get lots of weird cursor behavior when using the splitByGrapheme option.

@asturur
Copy link
Member

asturur commented Feb 20, 2019

i have an open branch to fix splitByGrapheme

@xiangpaopao
Copy link

The selection issues is still there, I can not select the last 4 letters

image

fabric v 2.7.0

var canvas = new fabric.Canvas(document.getElementById('c'),{
      width:500,
      height:500
    });
    var textbox = new fabric.Textbox('呜呜呜呜呜呜呜呜无无无无无无无无无无无无无无无无无无无无无无无无无', {
      left: 50,
      top: 50,
      width: 150,
      fontSize: 20,
      splitByGrapheme: true,
  });
  canvas.add(textbox).setActiveObject(textbox);

@asturur
Copy link
Member

asturur commented Mar 14, 2019

@xiangpaopao this is a different issue not related to this.
I thought i fixed it but i did not fix it enough.
Is related to splitByGrapheme: true and i need to look at it better.

@treyhoover
Copy link

treyhoover commented Mar 24, 2019

I'm seeing the same thing. splitByGrapheme: true helps control the textbox width but introduces other bugs with the cursor and text rendering.

@Hellowor1d
Copy link

@Tsourdox Yeah, I thought #5479 was a possible fix for this, but I get lots of weird cursor behavior when using the splitByGrapheme option.

Hi ~ , Any movement , here?

@Tsourdox
Copy link

Tsourdox commented May 7, 2019

As said splitByGrapheme: true isn't solving this issue

@asturur
Copy link
Member

asturur commented May 7, 2019

splitByGrapheme does something different, the breakWord scenario won't be supported by me for now. We have too many things to keep an eye on and as everyone of you that is trying to solve the break word issue has probably noticed, this is not plain simple

@xmqywx
Copy link

xmqywx commented May 30, 2019

@asturur Can we do an online voting system( https://feathub.com/ Perhaps we will know the most important features that the user needs)?I just think this feature is very important.You know,it's very unfriendly to two byte word(chinese,japanese,korean),some people waiting for resolve this problem for years(from 1.0 to 2.0 ~3.0).When we copy long word from office word or txt, then paste on https://shutterstock.com, the textbox width will be very very long.
I know it's an open source project. I'm sorry to make such an unreasonable request. If it bothers you, please ignore my suggestion,sorry.

@asturur
Copy link
Member

asturur commented May 31, 2019

It does not bother me, but i can't support this feature.
Adding to the code the knowloedge to split words only when needed, is harder than it looks.

splitByGrapheme now works and solve the problem of asian langauges that were the users with most problems. For english/latin users that want a break-word feature they need to figure out an override somehow.

I can't, i really can't. I have 310 open issues, of which at least 30% are probably solvable. I need to write docs and examples, this problem is very far in my priority list.

Are you an asian language user? Did you try to user splitByGrapheme option?

@xmqywx
Copy link

xmqywx commented Jun 1, 2019

Yes, the splitByGrapheme option, I've used it before.As mentioned above(@treyhoover), It's has many bugs now. I'll try to get around these bugs first.
Thank you for your reply.

@asturur
Copy link
Member

asturur commented Jun 1, 2019

it should not have bugs since fabric 3, if it has them you should report them.

@xmqywx
Copy link

xmqywx commented Jun 3, 2019

Yes, i updated it(to 3) in my projects, it works now . Thanks a lot.

@lcswillems
Copy link

It seems somebody made a bug free implementation of it here: https://jjwilly.com/fabric-js-2-0-breakwords-implementation/

Would it be okay to add it to the library? (I can do it)

As mentioned by others, this seems to be the biggest issue with Textbox, first thing that I discovered.

@asturur
Copy link
Member

asturur commented Jan 5, 2020

You can open a PR so we can check the diffs, i always felt this feature was unnecessary, but somehow people do not think so. We also needs tests to verify we do not break it later.

Unless the amount of code added is not much, i m not against it.

@AdamMadrzejewski
Copy link

@lcswillems are you thinking about creating a PR for it? Otherwise, I will make an attempt.

@AdamMadrzejewski
Copy link

AdamMadrzejewski commented Feb 28, 2020

I also thought about adding a new property called "max-width" that would adjust the size of the container to the text but stop at the max-width. It will also solve the issue raised here: https://stackoverflow.com/questions/60411035/adjust-fabricjs-text-box-size-to-match-the-text-selection

@lcswillems
Copy link

@AdamMadrzejewski I finally don't have the time to do a PR so you can do it :)

@ShaMan123 ShaMan123 added the text label Apr 10, 2022
@kirill-konshin
Copy link

@asturur @ShaMan123 looks like this issue is getting some traction. Great news. We have this issue too and we're looking forward to have it fixed.

@asturur
Copy link
Member

asturur commented Apr 26, 2022

If someone wants to recover https://jjwilly.com/fabric-js-2-0-breakwords-implementation/ this and make a PR we can talk about it.
As i said, we are overloaded with feature. Breaking a word but just if... seems really an extra feature.
I understand some text layouts may enjoy it.

What is your use case @kirill-konshin ? can you make a small example with a gif or a clear explanation?
breaking a word because there is no space i think is also dangerous in some languages and complicated with other because of grapheme clusters and connections between them.

@MuhuiCheng
Copy link

Does anyone know where the issue of wrong cursor position, wrong selection rendering is tracked? I also encountered this problem, in version 5.2.4.

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