Skip to content

Conversation

wxul
Copy link

@wxul wxul commented Dec 20, 2022

Motivation

Textbox option splitByGrapheme does not meet my needs

Description

The Textbox adds an overflowBreakWord option to achieve a result similar to the css property overflow-wrap: break-word. Unlike the default, the width can be adjusted, and unlike splitByGrapheme: true, the word will be kept as complete as possible.

MDN: overflow-wrap

Changes

Gist

In Action

Textbox default

fabric-textbox-default.mp4

Textbox with splitByGrapheme: true

fabric-textbox-slipByGrapheme.mp4

Textbox with overflowBreakWord: true

fabric-textbox-overflowBreakWord.mp4

@ShaMan123 ShaMan123 requested a review from melchiar December 20, 2022 12:06
@ShaMan123 ShaMan123 added the text label Dec 20, 2022
@ShaMan123
Copy link
Contributor

Hi @wxul
This PR looks really good.
We will review it in the near future.
It is a requested feature, nice!

Copy link
Contributor

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will test it and comment in case I have more findings.
Good job!
Could you add a line to the CHANGELOG?

* Will block splitByGrapheme option!
* @type Boolean
*/
overflowBreakWord: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we should introduce a breakLineStrategy prop or something similar instead of a flag and perhaps make splitByGrapheme an option

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but how to deal with the previous versions of splitByGrapheme ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can mark it as deprecated and introduce its predecessor and have logic supporting falling back to splitByGrapheme.
Or we break.
@asturur ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

introduced resolveTextOverflowStrategy

Copy link
Contributor

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have deprecated splitByGrapheme in favor of textOverflow. Is the naming good enough?

I have tested live and have a few concerns:

  • should initial size be equal to the largest word if width is not set? This question might be a non issue due to #8470 . Current behavior is like splitByGrapheme, width is set to the largest char.
  • lines with whitespace only should be dropped unless last line I believe

Text value: fabric.js sandbox sfdg sdfg sdfg sdfgsssa dsfgert ergertq wtrh srth

fabric.js.sandbox.-.Google.Chrome.2022-12-20.17-58-50_Trim.mp4
  • After this is merged we need to visit #8470 and see how it works and add a test perhaps.

@ShaMan123
Copy link
Contributor

After looking into cursor position I see there is an unrelated bug/wrong behavior of the cursor with line breaking.
It happens with textOverflow: 'anywhere' as well and predates this PR.
Looking at HTMLTextarea I see that when a line breaks all preceding white space is appended to that line without affecting the bounding box, while the next line starts with a non white space char. The cursor runs over the white space length - 1, then is drops to the next line start while remaining bound to the bounding box (while hitting the arrow the cursor remains at the edge of the textarea until white space has finished).
Cursor location/position correlates to text length in fabric. I think in order to align with HTMLTextarea we should address that. And/or replace white space with '' for cursor logic.
However, that should/could be in a separate PR.
I will try this out and see if it is too complicated.
Thoughts?

@wxul
Copy link
Author

wxul commented Dec 21, 2022

I have no opinion, I simply solved my problem and made a pr by the way, your opinion will be more comprehensive than mine.

@ShaMan123
Copy link
Contributor

fair enough

Copy link
Contributor

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asturur @melchiar I need your help here to determine 2 issues:

  1. CharSpacing + line breaking not on the first word still keeps some spacing
    See how it breaks with spacing on sdfg and precisely on fabric.js
fabric.js.sandbox.-.Google.Chrome.2022-12-21.15-04-37_Trim.mp4
  1. Style map generation, see comment

I am done here.
@wxul Thanks!


TODO

  • cursor is off with text overflow. It should break before the last space of a line (if exists) to the beginning of the next. This doesn't happen. Should be addressed in a follow.
  • ws at EOL in default breaking strategy

realLineCount++;
} else if (
!this.splitByGrapheme &&
!this.resolveTextOverflowStrategy() &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guessing this...
no tests and not enough context to understand this.
Why doesn't it use the split lines?

@ShaMan123 ShaMan123 changed the title feat(): Add Textbox overflowBreakWord option feat(): Add Textbox textOverflow option Dec 21, 2022
@asturur
Copy link
Member

asturur commented Dec 26, 2022

Hi @wxul this PR looks good to me, i m focused those days in finishing the TS migration and i would like to look at this shortly after.
Please don't feel ignored, we will get here.

@asturur
Copy link
Member

asturur commented Dec 26, 2022

(this feature was requested in the past too, we had an implementation but then we decided to do no exagerate with options)

Copy link
Contributor

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated from master

@melchiar
Copy link
Contributor

Code looks good to me, though I'm wondering if maybe the css property hyphens: auto would be a better behavior to replicate than overflow-wrap: break-word, since inserting a hyphen between a word break is more common typographically.

https://developer.mozilla.org/en-US/docs/Web/CSS/hyphens

@umtdemr
Copy link

umtdemr commented Feb 21, 2023

I noticed an issue with this PR. If we keep pressing space, the lines are getting broken.

@ShaMan123
Copy link
Contributor

@umtdemr can you provide a video?

@umtdemr
Copy link

umtdemr commented Feb 27, 2023

Screen.Recording.2023-02-23.at.2.36.13.PM.mov

@ShaMan123
Copy link
Contributor

nice!
we should adapt the code to break after a space and keep space length -1 in the new line

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants