fix(Text) Fix style transfer issue on a line that is not empty #9461
Conversation
|
Build Stats
|
…js into text-new-line-style-fix
|
This in theory removes the last known style text bug with typing. |
There was a problem hiding this comment.
there is another major bug here
fabric.js should be in the first line and not break, this is a regression I am seeing at work as well
There was a problem hiding this comment.
this is splitByGrapheme true so this will not take care of words, will just wraps whenever it feels it has to wrap.
There was a problem hiding this comment.
If you look at v5 that is not the same I think
There was a problem hiding this comment.
Setting split should split the word up (as in the other split true tests). I wondered about the word grapheme since this is really a Unicode character split but apparently there's different takes on what's considered a character vs. grapheme. I think the question here is what was broken in v5 that was considered "working" and what is actually a regression.
There was a problem hiding this comment.
splitByGrapheme is an option that was introduced for languages that do not use spaces to break text.
It doesn't split by unicode character but by grapheme clusters, if the appropriate splitter is provided.
Otherwise it splits with the rudimental grapheme splitter of fabricJS.
a grapheme can be made of more unicode codepoints/characters glued together. In our example it doesn't happen.
There was a problem hiding this comment.
regarding ' I think the question here is what was broken in v5 that was considered "working" and what is actually a regression.' i think nothing broken in v5 was considered working for styles.
This bug wasn't listed anywhere and i didn't know or remember of it.
As i said, we don't refactor to fix bugs, and moreover we are not refactoring text anytime soon. |
|
Anyway there is no rush here to merge this, this particular bug is here since before 5. |
src/shapes/IText/ITextBehavior.ts
Outdated
| const originalLineLength = this._unwrappedTextLines[lineIndex].length; | ||
| const isEndOfLine = originalLineLength === charIndex; | ||
|
|
||
| let someStyleIsCarringOver = false; |
There was a problem hiding this comment.
Do you mean someStyleIsCarryingOver?
There was a problem hiding this comment.
@gloriousjob you can use the suggestion markdown
| let someStyleIsCarringOver = false; | |
| let someStyleIsCarryingOver = false; |
|
Just checking but the example shows splitting a styled, then unstyled into two lines. Is the reverse handled as well (i.e., foo is unstyled, bar is red, and splitting doesn't make the b unstyled)? |
| await page.mouse.up(); | ||
| // we remove them because space is finishing | ||
| await canvasUtil.press('Backspace'); | ||
| // lets click where style end to show that we can add new line without carrying over |
There was a problem hiding this comment.
I suggest usign test.step to organize the test instead of leaving comments
It is more clear when reading and when looking at test results
There was a problem hiding this comment.
85800a532aaae57a102d53213c2b8114a0cadb8e.webm
There was a problem hiding this comment.
Checkout src files to master
src/shapes/IText/ITextBehavior.ts
Outdated
| @@ -819,7 +820,7 @@ export abstract class ITextBehavior< | |||
| for (const index in this.styles[lineIndex]) { | |||
| const numIndex = parseInt(index, 10); | |||
| if (numIndex >= charIndex) { | |||
| somethingAdded = true; | |||
| someStyleIsCarringOver = true; | |||
There was a problem hiding this comment.
| someStyleIsCarringOver = true; | |
| someStyleIsCarryingOver = true; |
src/shapes/IText/ITextBehavior.ts
Outdated
| @@ -828,14 +829,16 @@ export abstract class ITextBehavior< | |||
| } | |||
| } | |||
| let styleCarriedOver = false; | |||
| if (somethingAdded && !isEndOfLine) { | |||
| if (someStyleIsCarringOver && !isEndOfLine) { | |||
There was a problem hiding this comment.
| if (someStyleIsCarringOver && !isEndOfLine) { | |
| if (someStyleIsCarryingOver && !isEndOfLine) { |
|
i will add the test in master first, with steps as commented. We are adding an extra test step to cover the bug, and we are adding a step construct between the blocks of the test. |
|
hopefully test fail, but i wouldn't count on it. |
|
so at commit 29dcc2e we have failure |
|
and at commit b84e6f8 we have good test. Locally it passes anyway, but i can't do miracles, i should redo a different test that show this particular but bigger or find a way to only have a zone of the screenshot, but i can't find an option for it. But also this is good enough |
There was a problem hiding this comment.
There a typos that are commented by @gloriousjob
And I cpmmented a way tp clip the snapshot and perform a more reliable diff using number of pixels instead of percentage
| @@ -112,7 +112,7 @@ for (const splitByGrapheme of [true, false]) { | |||
| // and there is no style on the part of text that follows, but there is visible text. | |||
| await expect(await canvasUtil.screenshot()).toMatchSnapshot({ | |||
| name: `6-after-adding-a-newline-splitByGrapheme-${splitByGrapheme}.png`, | |||
| maxDiffPixelRatio: 0.01, | |||
| maxDiffPixelRatio: 0.009, | |||
There was a problem hiding this comment.
await expect(page).toHaveScreenshot(`${name}.png`, {
clip: {
x,
y,
width,
height,
},
maxDiffPixels, // insteadof percentage
});
There was a problem hiding this comment.
where is this clip in the documentation? i looked for something like this.
There was a problem hiding this comment.
oh ok is an entirely different way to do visual comparison. the locator can't be clipped.
There was a problem hiding this comment.
but the image can, right?
Co-authored-by: gloriousjob <gloriousjob@users.noreply.github.com>
| width: 120, | ||
| height: 120, | ||
| }, | ||
| maxDiffPixels: 0.029, |
There was a problem hiding this comment.
I thought this was an interger
Motivation
Fix las bug mentioned in #9028 that adds unwanted style on previous text when we insert a new line.
This bug is also present in 5.x
bug after inserting a new line
close #9028
Description
Changes