fix(Textbox) Textbox inputs with new lines #9192

asturur · 2023-08-31T14:19:46Z

Motivation

This PR reverts the part of broken code in #9097.
The code doesn't make sense, and we need a test to be sure we don't break it again.

The regression is happening only for splitByGrapheme set to true.

There is still a style issue that i still don't know from which commits originates, also only for splitByGrapheme, and i m looking into it

closes #9190

To add a test the following addition have been done to the CanvasUtil:

add a press method that takes a key
add a ctrlC and ctrlV method.

ctrlC and ctrlV require permissions for clipboard handling and need to treat macos differently.

asturur · 2023-08-31T14:20:50Z

src/shapes/Textbox.ts

-        const graphemeArray = splitByGrapheme
-          ? [word]
-          : this.graphemeSplit(word);
+        const graphemeArray = splitByGrapheme ? word : this.graphemeSplit(word);


this line and line 349 are both necessary to unbreak the textbox.

github-actions · 2023-08-31T14:21:48Z

Build Stats

file / KB (diff)	bundled	minified
fabric	915.100 (+0.058)	305.428 (+0.017)

ShaMan123 · 2023-09-01T02:56:23Z

Please try the fix I did to styles and onInput
#9096

asturur · 2023-09-01T06:20:08Z

i found this bug trying to merge the test from exactly that PR, that is the only thing that will likely get merged.
That pr won't be merged since:

removes methods without explaining why
renames methods together with the fix
does not explain where the fix is.

i would gladly merge the fix if i could individuate it

asturur · 2023-09-01T06:27:52Z

but yes the style bug is the same that is reported in #9028, so i don't need to investigate it.

…s into fix-textbox-regression

asturur · 2023-09-01T07:21:09Z

added a test that covers inputing new lines with both splitByGrapheme true and false

ShaMan123 · 2023-09-01T12:07:00Z

If the description is the problem I can amend that.

asturur · 2023-09-01T12:18:48Z

no @ShaMan123 that pr is a large mix refactor that is not going anywhere for me. if you identified the style bug we can fix it, otherwise we will have to find the bugged code first.

ShaMan123 · 2023-09-01T13:20:44Z

I wrote what the bug is.
A mix between the wrapped and non wrapped use of line/char index
That is why I renamed stuff.
A lot like with getPointer.
And removed the methods that existed to work around all the mix.

asturur · 2023-09-01T14:06:02Z

We talked about this more than once.
What you do is this code is terrible, i ll write a bunch of new code that works
I can't accept PRs like that. Not to fix a bug.
And anyway is unrelated from this PR.

asturur · 2023-09-04T10:01:54Z

@ShaMan123 i would like either a request for change or an approval when you have time, so i can move forward with the other stacked fixes

ShaMan123 · 2023-09-05T04:48:17Z

In the weekend I will get to it

asturur · 2023-09-05T05:59:15Z

ok i ll let text rest this week then. I ll move to look at the layout pr.

ShaMan123

Sorry @asturur but it seems to me you are redoing at least part of #9096 in #9197 (and not fixing all it fixes) instead of merging it.
I really do not understand why you are against that PR. Yes, it removes many methods because they are not needed, all seem to have been written to patch the exisiting situation.
The test in #9096 is more extensive than the tests you wrote here because it also deletes lines.

ShaMan123 · 2023-09-09T02:44:06Z

src/shapes/Textbox.ts

@@ -339,17 +345,16 @@ export class Textbox<
        : this.wordSplit(line);

      if (wordsOrGraphemes.length === 0) {
-        return [];
+        // @ts-expect-error
+        wordsOrGraphemes.push([]);


why?
If it must return something then so be it.

see #9192 (comment)

ShaMan123

Please explain why snapshot 3 is so different when splitByGrapheme is true/false
The style is applied differently to the pasted text, right?

e2e/tests/text/adding-text/index.spec.ts

ShaMan123 · 2023-09-09T03:33:10Z

I have commited 4803a19 - feel free to revert it.
It is a PR review suggestion I wanted to make.
If it works and you agree to it we can change the GraphemeData type back to string[] only

asturur · 2023-09-09T07:45:22Z

@ShaMan123 because i m not doing refactors on text now or later to fix a bug that is a regression and that gets fixed removing the code that introduce the bug. This pr reverts the offending code that broke the functionality. period.

i will check your commit and if is not another refactor we can keep it.

the methods you deleted in your pr are user facing functionalities, are not used by fabricjs ( like cleanStyle )

ShaMan123 · 2023-09-09T08:38:51Z

I was sure cleanStyle was internal.
I do not see a use for it since stylesToArray has come but I might be missing something

ShaMan123 · 2023-09-09T08:39:32Z

@ShaMan123 because i m not doing refactors on text now or later to fix a bug that is a regression and that gets fixed removing the code that introduce the bug. This pr reverts the offending code that broke the functionality. period.

Yes, but there are other bugs with style that PR fixes and these do not and are not regressions

asturur · 2023-09-09T09:40:33Z

I was sure cleanStyle was internal. I do not see a use for it since stylesToArray has come but I might be missing something

cleanStyle allows you to remove styles that are identical to the object main properties. It reduce the quantity of style applied to an object.

asturur · 2023-09-09T09:43:43Z

@ShaMan123 because i m not doing refactors on text now or later to fix a bug that is a regression and that gets fixed removing the code that introduce the bug. This pr reverts the offending code that broke the functionality. period.

Yes, but there are other bugs with style that PR fixes and these do not and are not regressions

I m sure i talked about this with you already.
There is a bug, we find the bug we fix the bug. Rewriting a bunch of code to fix a bug is wrong.
I can't accept it.
I m not going to put in discussion a complicated piece of code like onInput because there is a style shift somewhere else ( that is fixed by another PR ).
When there are bugs we fix them as patches on the current codebase.

In this case we should have remove the breaking PR entirely but is not possible and remove a lot of TS work that is just types. So i just undid the lines that introduced the issue.

asturur · 2023-09-09T09:46:10Z

Please explain why snapshot 3 is so different when splitByGrapheme is true/false The style is applied differently to the pasted text, right?

snapshot 3 is different because this PR doesn't fix the style shift.
That will be fixed by the other PR and the snapshot will have to be updated

Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>

This reverts commit 95115c0.

ShaMan123 · 2023-09-09T10:44:39Z

src/shapes/Textbox.ts

@@ -22,7 +22,7 @@ export const textboxDefaultValues: Partial<TClassProperties<Textbox>> = {

 export type GraphemeData = {
  wordsData: {
-    word: string[];
+    word: string[] | string;


Suggested change

word: string[] | string;

word: string[];

???

ShaMan123 · 2023-09-09T10:45:31Z

src/shapes/Textbox.ts

@@ -327,6 +327,12 @@ export class Textbox<
   *
   */
  getGraphemeDataForRender(lines: string[]): GraphemeData {
+    // this method has issues.
+    // it has been typed after some small refactors and is unclear how is working
+    // graphemeArray is either a string or a string[].


this is also wrong now

github-actions · 2023-09-09T11:43:45Z

Coverage after merging fix-textbox-regression into master will be

82.97%

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%	9
Intersection.ts	100%	100%	100%	100%
Observable.ts	87.76%	85.29%	87.50%	89.58%	134–135, 160–161, 32–33, 41, 50, 78, 89
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, 13, 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.06%	82.35%	100%	93.81%	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, 32, 39, 46, 53, 60, 67, 84–86, 94–96, 98–99
src/canvas
Canvas.ts	79.12%	77.68%	83.05%	79.61%	1000, 1003–1004, 1004, 1004, 1006, 1014, 1014, 1014–1016, 1016, 1016, 1023–1024, 1032–1033, 1033, 1033–1034, 1040, 1042, 1073–1075, 1078–1079, 1083–1084, 1197–1199, 1202–1203, 1276, 1395, 1517, 157, 182, 292–293, 296–300, 305, 328–329, 334–339, 359, 359, 359–360, 360, 360–361, 369, 37, 374–375, 375, 375–376, 378, 387, 393–394, 394, 394, 41, 437, 445, 449, 449, 449–450, 452, 535–536, 536, 536–537, 543, 543, 543–545, 565, 567, 567, 567–568, 568, 568, 571, 571, 571–572, 575, 584–585, 587–588, 590, 590–591, 593–594, 606–607, 607, 607–608, 610–615, 621, 628, 665, 665, 665, 667, 669–674, 680, 686, 686, 686–687, 689, 692, 697, 710, 738, 738, 796–797, 797, 797–798, 800, 803–804, 804, 804–805, 807–808, 811, 811–813, 816–817, 887, 899, 906, 927, 959, 980–981, 997–998, 998, 998–999
CanvasOptions.ts	100%	100%	100%	100%
SelectableCanvas.ts	94.44%	92.28%	94.23%	96.17%	1098, 1100, 1102–1103, 301, 477–478, 480–481, 481, 481, 530–531, 592–593, 646–648, 680, 685–686, 713–714, 898, 898–899, 902, 922, 922, 971, 979
StaticCanvas.ts	96.78%	93.09%	100%	98.53%	1030, 1040, 1092–1093, 1096, 1131–1132, 1208, 1217, 1217, 1221, 1221, 1268–1269, 185–186, 202, 570, 582–583, 913–914, 914, 914–915
StaticCanvasOptions.ts	100%	100%	100%	100%
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	6.06%	0%	0%	11.43%	103, 117, 117, 117, 117, 117, 119–122, 122, 125, 132, 17, 25–29, 29, 29, 29, 29, 29, 29, 29, 50–56, 56, 56, 56, 56, 58, 63–64, 66, 76, 82–84, 84, 86, 89–90, 90, 90, 90, 90, 92, 97
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<

asturur · 2023-09-09T12:34:07Z

the line you changed seems to unbreak the issue anyway and is more consistent with how we planned that function in #9097 so we keep it.

asturur · 2023-09-09T12:39:54Z

The test in #9096 is more extensive than the tests you wrote here because it also deletes lines.

Yes because this PR is fixing a line adding bug, so it is testing that.

Regarding redoing #9096 in the other PR, i don't think it is true.
I m fixing the different bugs you found in the issue, on the current codebase, patching the current code.
There are 3 different bugs, so i ll need 3 smalls prs.
The third i didn't do yet because they are stacked all on top of each other anyway.

fix regression

f01de95

asturur changed the title ~~fix() Textbox inputs with new lines~~ fix(Textbox) Textbox inputs with new lines Aug 31, 2023

asturur commented Aug 31, 2023

View reviewed changes

update CHANGELOG.md

0e195b5

asturur added 4 commits September 1, 2023 09:04

a test

acf8d46

Merge branch 'fix-textbox-regression' of github.com:fabricjs/fabric.j…

af8ee71

…s into fix-textbox-regression

cover both cases

f95c613

no changes to sandbox

6b75654

asturur added 2 commits September 1, 2023 09:32

more tolerance

a0d13b1

more tolerance

dd8278c

asturur mentioned this pull request Sep 1, 2023

[BUG]: text style onInput #9028

Closed

asturur mentioned this pull request Sep 1, 2023

fix(Textbox): implemente a fix for the style shifting issues on new lines #9197

Merged

asturur requested a review from ShaMan123 September 4, 2023 11:00

asturur added bug text labels Sep 4, 2023

ShaMan123 suggested changes Sep 9, 2023

View reviewed changes

suggestion

4803a19

ShaMan123 suggested changes Sep 9, 2023

View reviewed changes

e2e/tests/text/adding-text/index.spec.ts Show resolved Hide resolved

asturur and others added 3 commits September 9, 2023 12:19

Update e2e/tests/text/adding-text/index.spec.ts

95115c0

Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>

Revert "Update e2e/tests/text/adding-text/index.spec.ts"

0dbc933

This reverts commit 95115c0.

remove the unnecessary double type

c416dbe

ShaMan123 reviewed Sep 9, 2023

View reviewed changes

asturur added 2 commits September 9, 2023 13:37

more removals

5f7eef8

changelog

6e351f8

asturur requested a review from ShaMan123 September 9, 2023 12:43

ShaMan123 approved these changes Sep 9, 2023

View reviewed changes

asturur merged commit 5ee3fe2 into master Sep 9, 2023
24 checks passed

asturur deleted the fix-textbox-regression branch September 9, 2023 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(Textbox) Textbox inputs with new lines #9192

fix(Textbox) Textbox inputs with new lines #9192

asturur commented Aug 31, 2023 •

edited

asturur Aug 31, 2023

github-actions bot commented Aug 31, 2023 •

edited

ShaMan123 commented Sep 1, 2023 •

edited

asturur commented Sep 1, 2023 •

edited

asturur commented Sep 1, 2023

asturur commented Sep 1, 2023

ShaMan123 commented Sep 1, 2023

asturur commented Sep 1, 2023

ShaMan123 commented Sep 1, 2023

asturur commented Sep 1, 2023

asturur commented Sep 4, 2023

ShaMan123 commented Sep 5, 2023

asturur commented Sep 5, 2023

ShaMan123 left a comment •

edited

ShaMan123 Sep 9, 2023 •

edited

ShaMan123 Sep 9, 2023

ShaMan123 left a comment •

edited

ShaMan123 commented Sep 9, 2023 •

edited

asturur commented Sep 9, 2023

ShaMan123 commented Sep 9, 2023

ShaMan123 commented Sep 9, 2023 •

edited

asturur commented Sep 9, 2023

asturur commented Sep 9, 2023

asturur commented Sep 9, 2023

ShaMan123 Sep 9, 2023

ShaMan123 Sep 9, 2023

github-actions bot commented Sep 9, 2023

asturur commented Sep 9, 2023

asturur commented Sep 9, 2023 •

edited

fix(Textbox) Textbox inputs with new lines #9192

fix(Textbox) Textbox inputs with new lines #9192

Conversation

asturur commented Aug 31, 2023 • edited

Motivation

asturur Aug 31, 2023

Choose a reason for hiding this comment

github-actions bot commented Aug 31, 2023 • edited

ShaMan123 commented Sep 1, 2023 • edited

asturur commented Sep 1, 2023 • edited

asturur commented Sep 1, 2023

asturur commented Sep 1, 2023

ShaMan123 commented Sep 1, 2023

asturur commented Sep 1, 2023

ShaMan123 commented Sep 1, 2023

asturur commented Sep 1, 2023

asturur commented Sep 4, 2023

ShaMan123 commented Sep 5, 2023

asturur commented Sep 5, 2023

ShaMan123 left a comment • edited

Choose a reason for hiding this comment

ShaMan123 Sep 9, 2023 • edited

Choose a reason for hiding this comment

ShaMan123 Sep 9, 2023

Choose a reason for hiding this comment

ShaMan123 left a comment • edited

Choose a reason for hiding this comment

ShaMan123 commented Sep 9, 2023 • edited

asturur commented Sep 9, 2023

ShaMan123 commented Sep 9, 2023

ShaMan123 commented Sep 9, 2023 • edited

asturur commented Sep 9, 2023

asturur commented Sep 9, 2023

asturur commented Sep 9, 2023

ShaMan123 Sep 9, 2023

Choose a reason for hiding this comment

ShaMan123 Sep 9, 2023

Choose a reason for hiding this comment

github-actions bot commented Sep 9, 2023

asturur commented Sep 9, 2023

asturur commented Sep 9, 2023 • edited

asturur commented Aug 31, 2023 •

edited

github-actions bot commented Aug 31, 2023 •

edited

ShaMan123 commented Sep 1, 2023 •

edited

asturur commented Sep 1, 2023 •

edited

ShaMan123 left a comment •

edited

ShaMan123 Sep 9, 2023 •

edited

ShaMan123 left a comment •

edited

ShaMan123 commented Sep 9, 2023 •

edited

ShaMan123 commented Sep 9, 2023 •

edited

asturur commented Sep 9, 2023 •

edited