-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
fix(Textbox) Textbox inputs with new lines #9192
Conversation
src/shapes/Textbox.ts
Outdated
const graphemeArray = splitByGrapheme | ||
? [word] | ||
: this.graphemeSplit(word); | ||
const graphemeArray = splitByGrapheme ? word : this.graphemeSplit(word); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line and line 349 are both necessary to unbreak the textbox.
Build Stats
|
Please try the fix I did to styles and onInput |
i found this bug trying to merge the test from exactly that PR, that is the only thing that will likely get merged.
i would gladly merge the fix if i could individuate it |
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
added a test that covers inputing new lines with both splitByGrapheme true and false |
If the description is the problem I can amend that. |
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. |
I wrote what the bug is. |
We talked about this more than once. |
@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 |
In the weekend I will get to it |
ok i ll let text rest this week then. I ll move to look at the layout pr. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/shapes/Textbox.ts
Outdated
@@ -339,17 +345,16 @@ export class Textbox< | |||
: this.wordSplit(line); | |||
|
|||
if (wordsOrGraphemes.length === 0) { | |||
return []; | |||
// @ts-expect-error | |||
wordsOrGraphemes.push([]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
If it must return something then so be it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see #9192 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please explain why snapshot 3 is so different when splitByGrapheme is true/false
The style is applied differently to the pasted text, right?
I have commited 4803a19 - feel free to revert it. |
@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 ) |
I was sure cleanStyle was internal. |
Yes, but there are other bugs with style that PR fixes and these do not and are not regressions |
cleanStyle allows you to remove styles that are identical to the object main properties. It reduce the quantity of style applied to an object. |
I m sure i talked about this with you already. 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. |
snapshot 3 is different because this PR doesn't fix the style shift. |
Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>
This reverts commit 95115c0.
src/shapes/Textbox.ts
Outdated
@@ -22,7 +22,7 @@ export const textboxDefaultValues: Partial<TClassProperties<Textbox>> = { | |||
|
|||
export type GraphemeData = { | |||
wordsData: { | |||
word: string[]; | |||
word: string[] | string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
word: string[] | string; | |
word: string[]; |
???
src/shapes/Textbox.ts
Outdated
@@ -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[]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is also wrong now
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. |
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. |
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:
ctrlC and ctrlV require permissions for clipboard handling and need to treat macos differently.