Fix minimum word width calculation by asturur · Pull Request #9004 · fabricjs/fabric.js

asturur · 2023-06-09T15:42:49Z

Motivation

While working on text measurement reorganization, @ShaMan123 found some possible improvemente in the handlin of the minimum text width/ largest word width.

This PR is the extraction of that improvement from his PR that has too many changes.
reference:
#8994

Changes

words are measured all together before layout.
adds 2 loops around lines rather than 1 as the recent PR that added looping over the whole line before layouting the single line, before fabric was trying to do measurements while progressing on the line, but this of course came with compromises.

Gist

In Action

github-actions · 2023-06-09T15:44:38Z

Build Stats

file / KB (diff)	bundled	minified
fabric	922.593 (+1.026)	303.266 (+0.210)

…s into fix-minimum-word-width

ShaMan123

This looks good
I would like to rename the use of words to parts or something else because in case of splitByGrapheme it is not words and very confusing.
I also think we should change the signature of _wrapLine to be more orderly
_wrapLine(lie, lineIndex, context)

Pull in the test from #8994

Regardless I think we should hold the entire measurement data in wordsData and reuse it in case of resizing, it optimizes perf significantly.
If we do that overriding, measureWords allows easy customization of wrapping (this is what I am doing) because it will contain of the data needed.

asturur · 2023-06-10T00:22:03Z

This looks good I would like to rename the use of words to parts or something else because in case of splitByGrapheme it is not words and very confusing. I also think we should change the signature of _wrapLine to be more orderly _wrapLine(lie, lineIndex, context)

Pull in the test from #8994

Regardless I think we should hold the entire measurement data in wordsData and reuse it in case of resizing, it optimizes perf significantly. If we do that overriding, measureWords allows easy customization of wrapping (this is what I am doing) because it will contain of the data needed.

Let me verify if this fix what i think it fixes ( i think is just a case in initialization ), let's see then what is missing for your overrides need, then we can also rename and change signature of private methods in another PR.

asturur · 2023-06-10T23:45:00Z

+   * measure each word width and extract the largest word from all.
+   * @param {string[]} lines the lines we need to measure
+   */
+  measureWords(lines: string[]): WordsWidthData {


@ShaMan123 this code now works as your for what regards min word width.
You said that this would be enough to also let you override the code, but in that case we need to be specific of the requirements of this functions.
Seems to me that this on top of measuring is also deciding the words that are going to be rendered?
(ex: swapping last word for ellipsis)
like are you returning different words from this function from the one that are in input?

in that case we need to rename it accordingly and also specify this in the returned type and description.

this function so far would be something like:

getGraphemeDataForRender

measureAndSplitWords

getSplittedWords

getSplitWordsInfo

So I have a few things.

(ex: swapping last word for ellipsis)

Yes. I am doing that exactly. And I can PR it if you wish.
I am also breaking words, this is why it is crucial to manage the infix here as well because then I can add and measure the infix, or not if I am simply breaking a word. It makes sense to have all measuring in one place, another reason to move infix measuring to here.
We should move this method to Text, it belongs there IMO.
Then the measureChars function should use it instead of remeasuring.
Positioning data (left, top => getGraphemeDataForRender) should be applied separately because these props are invalidated more frequently (always actually) where as the words data (width, height) could be cached to improve perf.
I have something POCing this and the perf diff is extremely significant when I apply caching.

And actually it means that _wrapLine becomes a very standard method

Don't poc anything i m not accepting new features, i m trying to let you get the method you need without flipping textbox now.
How can we get to what you need without revolutionin the code now?

I am POCing in my app.

github-actions · 2023-06-10T23:46:25Z

Coverage after merging fix-minimum-word-width into master will be

83.74%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
index.node.ts	7.69%	100%	0%	14.29%	17, 20, 23, 35, 38, 41
src
ClassRegistry.ts	100%	100%	100%	100%
Collection.ts	94.71%	94.64%	86.67%	97.09%	101, 104, 207–208, 233–234
CommonMethods.ts	96.55%	87.50%	100%	100%	10
Intersection.ts	100%	100%	100%	100%
Observable.ts	87.23%	85.29%	84.62%	89.36%	144–145, 170–171, 39–40, 48, 57, 91, 99
Point.ts	100%	100%	100%	100%
Shadow.ts	98.36%	95.65%	100%	100%	178
cache.ts	97.06%	90%	100%	100%	57
config.ts	75%	66.67%	66.67%	82.76%	130, 138, 138, 138, 138, 138–140, 151–153
constants.ts	100%	100%	100%	100%
src/Pattern
Pattern.ts	92.31%	91.89%	90%	93.55%	118, 129, 138, 31, 94
src/brushes
BaseBrush.ts	100%	100%	100%	100%
CircleBrush.ts	0%	0%	0%	0%	108, 108, 108, 110, 112, 114–116, 118–121, 128–129, 136, 138, 23–24, 32–36, 40–44, 51–54, 62–66, 68, 76, 76, 76, 76, 76–77, 79, 79, 79–82, 84, 92–93, 95, 97–99
PatternBrush.ts	97.06%	87.50%	100%	100%	21
PencilBrush.ts	91.01%	82.35%	100%	93.75%	122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
SprayBrush.ts	0%	0%	0%	0%	107, 107, 107, 107, 107–108, 110–111, 118–119, 121, 123–127, 136, 140–141, 141, 149, 149, 149–152, 154–157, 161–162, 164, 166–169, 17, 172, 179, 18, 180, 182, 184–185, 187, 194–195, 197–198, 20, 201, 201, 208, 208, 21, 212, 22, 22, 22–24, 28, 37, 44, 51, 58, 65, 84–86, 94–96, 98–99
src/canvas
Canvas.ts	79.05%	77.54%	83.05%	79.57%	1000–1001, 1001, 1001–1003, 1005–1006, 1006, 1006, 1008, 1016, 1016, 1016–1018, 1018, 1018, 1024–1025, 1033–1034, 1034, 1034–1035, 1040, 1042, 1073–1075, 1078–1079, 1083–1084, 1197–1199, 1202–1203, 1276, 1395, 1515, 161, 186, 296–297, 300–304, 309, 332–333, 338–343, 36, 363, 363, 363–364, 364, 364–365, 373, 378–379, 379, 379–380, 382, 391, 397–398, 398, 398, 40, 441, 449, 453, 453, 453–454, 456, 538–539, 539, 539–540, 546, 546, 546–548, 568, 570, 570, 570–571, 571, 571, 574, 574, 574–575, 578, 587–588, 590–591, 593, 593–594, 596–597, 609–610, 610, 610–611, 613–618, 624, 631, 668, 668, 668, 670, 672–677, 683, 689, 689, 689–690, 692, 695, 700, 713, 741, 741, 799–800, 800, 800–801, 803, 806–807, 807, 807–808, 810–811, 814, 814–816, 819–820, 890, 902, 909, 930, 962, 983–984
SelectableCanvas.ts	94.42%	92.24%	94.23%	96.15%	1115, 1115–1116, 1119, 1139, 1139, 1188, 1196, 1315, 1317, 1319–1320, 519, 694–695, 697–698, 698, 698, 747–748, 809–810, 863–865, 897, 902–903, 930–931
StaticCanvas.ts	96.73%	92.88%	100%	98.52%	1037–1038, 1038, 1038–1039, 1159, 1169, 1223–1224, 1227, 1262–1263, 1339, 1348, 1348, 1352, 1352, 1399–1400, 309–310, 326, 694, 706–707
TextEditingManager.ts	84.31%	71.43%	91.67%	88%	17–18, 18, 18–19, 19, 19
src/canvas/DOMManagers
CanvasDOMManager.ts	95.52%	70%	100%	100%	21–22, 29
StaticCanvasDOMManager.ts	100%	100%	100%	100%
util.ts	86.67%	80.56%	83.33%	93.94%	14, 26, 63–64, 67, 67, 74, 93–94
src/color
Color.ts	94.96%	91.67%	96.30%	96.05%	233, 258–259, 267–268, 48
color_map.ts	100%	100%	100%	100%
constants.ts	100%	100%	100%	100%
util.ts	85.71%	76.92%	100%	89.74%	55–56, 56, 58, 58, 58–59, 61–62, 89
src/controls
Control.ts	93.33%	87.88%	91.67%	97.78%	175, 240, 327, 327, 362
changeWidth.ts	100%	100%	100%	100%
commonControls.ts	100%	100%	100%	100%
controlRendering.ts	81.63%	78%	100%	84.78%	106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
drag.ts	100%	100%	100%	100%
fireEvent.ts	88.89%	75%	100%	100%	13
polyControl.ts	5.97%	0%	0%	11.11%	100, 105, 119, 119, 119, 119, 119, 121–124, 124, 127, 134, 17, 25–29, 29, 29, 29, 29, 29, 29, 29, 50–56, 56, 56, 56, 56, 58, 63–64, 66, 76, 82–83, 83, 83–84, 88–90, 90, 90, 90, 90, 92
rotate.ts	19.57%	12.50%	50%	21.43%	41, 45, 51, 51, 51–52, 55–57, 59, 59, 59, 59, 59–61, 61, 61–63, 65, 65, 65–67, 67, 67–68, 73, 73, 73–74, 76, 78, 80–81
scale.ts	93.57%	92.94%	100%	93.67%	129–130, 132–134, 148–149, 181–183, 42
scaleSkew.ts	78.79%	64.29%	100%	85.71%	27, 29, 29, 29, 31, 33, 35
skew.ts	91.03%	79.31%	100%	97.67%	131–132, 163–164, 171, 177, 179
util.ts	100%	100%	100%	100%
wrapWithFireEvent.ts	100%	100%	100%	100%
wrapWithFixedAnchor.ts	100%	100%	100%	100%
src/env
browser.ts	84.21%	77.78%	50%	100%	14, 17
index.ts	100%	100%	100%	100%
node.ts	74.07%	33.33%	66.67%	88.89%

asturur · 2023-06-11T01:03:10Z

    offset = 0;
    let i;
-    for (i = 0; i < words.length; i++) {
+    for (i = 0; i < data.length; i++) {


there is probably something else that can be done down here to bring infix width calculation in measure words.
but i m intentionally skipping it for now.
Seems like line 419 could be pused in the else condition after 415.

i m also not convinced at all from the offset++ at line 426, but eventually it works

Yes, all this is correct.
I did that in the other PR.

Now it will make sense to you.

ShaMan123 · 2023-06-11T13:12:38Z

+      }
+
+      // measure words
+      data[i] = words.map((word) => {


why not use lines.map?

welll the previus for loop didn't exist, so i can use lines.map yes

ShaMan123 · 2023-06-11T13:22:03Z

+   * measure each word width and extract the largest word from all.
+   * @param {string[]} lines the lines we need to measure
+   */
+  measureWords(lines: string[]): WordsWidthData {


So I have a few things.

(ex: swapping last word for ellipsis)

Yes. I am doing that exactly. And I can PR it if you wish.
I am also breaking words, this is why it is crucial to manage the infix here as well because then I can add and measure the infix, or not if I am simply breaking a word. It makes sense to have all measuring in one place, another reason to move infix measuring to here.
We should move this method to Text, it belongs there IMO.
Then the measureChars function should use it instead of remeasuring.
Positioning data (left, top => getGraphemeDataForRender) should be applied separately because these props are invalidated more frequently (always actually) where as the words data (width, height) could be cached to improve perf.
I have something POCing this and the perf diff is extremely significant when I apply caching.

asturur · 2023-06-11T17:05:40Z

@ShaMan123 let me know if this is helpful and if you want to merge it.

ShaMan123

This is almost there.
I want the infix as part of this change
Also please rename:

All the use of words to something else because when splitting by grapheme words are not words and it is awfully confusing.
getGraphemeDataForRender => this is not for rendering, it is for layout. The rendering measurements are in renderChar

ShaMan123 · 2023-06-11T23:17:11Z

without handling the infix I can't control word breaking because _wrapLine will push the infix it decides instead of the one I want so this PR will not be good enough.
However it is a step forward.
Moving the infix logic will make both this and #8994 identical.

ShaMan123 · 2023-06-11T23:18:42Z

this part doesn't make sense in _wrapLine
https://github.com/fabricjs/fabric.js/pull/9004/files#diff-25ec98d446b9f1934b6a36a8784e77fed6b0bf5304a0f95fc7155ad93dfd6d33R427-R429

ShaMan123 · 2023-06-12T20:44:15Z

@asturur please merge this before you publish.
This is good enough because the lifecycle is fixed. I can subclass now.
We can get back to infix later and then I adjust my logic in favor of what we decide

asturur · 2023-06-17T13:40:14Z

this part doesn't make sense in _wrapLine https://github.com/fabricjs/fabric.js/pull/9004/files#diff-25ec98d446b9f1934b6a36a8784e77fed6b0bf5304a0f95fc7155ad93dfd6d33R427-R429

It Doesn't make sense anymore because we moved the calculation up, and this code is now sort of unnecessary.
But i really wanted to do the minimum possible here.

ShaMan123 · 2023-06-17T14:21:06Z

+    });
+
+    return {
+      wordsData: data,


I am not happy about naming

ShaMan123 · 2023-06-17T15:45:44Z

this part doesn't make sense in _wrapLine https://github.com/fabricjs/fabric.js/pull/9004/files#diff-25ec98d446b9f1934b6a36a8784e77fed6b0bf5304a0f95fc7155ad93dfd6d33R427-R429

It Doesn't make sense anymore because we moved the calculation up, and this code is now sort of unnecessary. But i really wanted to do the minimum possible here.

#8994!! That is why I changed so much, because of all this ugliness

ShaMan123 · 2023-06-17T15:48:23Z

I am not 100% happy of naming and overall state of this PR

asturur and others added 3 commits June 9, 2023 17:40

fix with possibly less amount of changes

ab7bd0d

fix with possibly less amount of changes

5ae167b

update CHANGELOG.md

e591773

asturur mentioned this pull request Jun 9, 2023

fix(Textbox): minWidth of text, measure lines before wrapping #8994

Closed

asturur added 4 commits June 9, 2023 17:51

more comments

689ab8c

Merge branch 'fix-minimum-word-width' of github.com:fabricjs/fabric.j…

23b66e2

…s into fix-minimum-word-width

better changelog

a06bf0f

better changelog

bbf81b9

ShaMan123 reviewed Jun 9, 2023

View reviewed changes

add tests

2a1664a

asturur added the text label Jun 10, 2023

asturur added 2 commits June 11, 2023 01:41

added original test

d7bab6c

added original test

25ab331

asturur commented Jun 10, 2023

View reviewed changes

not understand this test

9b82e89

asturur commented Jun 11, 2023

View reviewed changes

Comment thread test/unit/textbox.js

asturur added 3 commits June 11, 2023 02:36

add styles test

cf4c0fe

add styles test

ab06ad2

add styles test

6b836f4

asturur commented Jun 11, 2023

View reviewed changes

ShaMan123 reviewed Jun 11, 2023

View reviewed changes

asturur added 2 commits June 11, 2023 17:20

removed unused param

80cb6b9

renamed method

61d9d02

ShaMan123 reviewed Jun 11, 2023

View reviewed changes

asturur added 2 commits June 17, 2023 22:40

Merge branch 'master' into fix-minimum-word-width

48d219f

Update Textbox.ts

6a2a3e2

asturur merged commit 5abd37b into master Jun 17, 2023

asturur deleted the fix-minimum-word-width branch June 17, 2023 14:14

ShaMan123 reviewed Jun 17, 2023

View reviewed changes

Comment thread src/shapes/Textbox.ts

});

return {

wordsData: data,

ShaMan123 Jun 17, 2023

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

I am not happy about naming

Uh oh!

Uh oh!

Conversation

asturur commented Jun 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

Gist

In Action

Uh oh!

github-actions Bot commented Jun 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ShaMan123 left a comment

Choose a reason for hiding this comment

Uh oh!

asturur commented Jun 10, 2023

Uh oh!

asturur Jun 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asturur Jun 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asturur commented Jun 11, 2023

Uh oh!

ShaMan123 left a comment

Choose a reason for hiding this comment

Uh oh!

ShaMan123 commented Jun 11, 2023

Uh oh!

ShaMan123 commented Jun 11, 2023

Uh oh!

ShaMan123 commented Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asturur commented Jun 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShaMan123 commented Jun 17, 2023

Uh oh!

ShaMan123 commented Jun 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

asturur commented Jun 9, 2023 •

edited

Loading

github-actions Bot commented Jun 9, 2023 •

edited

Loading

asturur Jun 10, 2023 •

edited

Loading

asturur Jun 11, 2023 •

edited

Loading

github-actions Bot commented Jun 10, 2023 •

edited

Loading

ShaMan123 commented Jun 12, 2023 •

edited

Loading