Skip to content
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(Text) Fix style transfer issue on a line that is not empty #9461

Merged
merged 23 commits into from
Jan 13, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

## [next]

- fix(Text) Fix style transfer issue on a line that is not empty [#9461](https://github.com/fabricjs/fabric.js/pull/9461)
- feat(util): expose `calcPlaneRotation` [#9419](https://github.com/fabricjs/fabric.js/pull/9419)
- refactor(Canvas): BREAKING remove button from mouse events, delegate to event.button property [#9449](https://github.com/fabricjs/fabric.js/pull/9449)
- patch(Canvas): move event mouse:up:before earlier in the logic for more control [#9434](https://github.com/fabricjs/fabric.js/pull/9434)
Expand Down
33 changes: 29 additions & 4 deletions e2e/tests/text/adding-text/index.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ setup();

test.describe.configure({ mode: 'serial' });

for (const splitByGrapheme of [true, false]) {
for (const splitByGrapheme of [false, true]) {
test(`adding new lines and copy paste - splitByGrapheme: ${splitByGrapheme}`, async ({
page,
context,
Expand Down Expand Up @@ -57,6 +57,8 @@ for (const splitByGrapheme of [true, false]) {
await expect(await canvasUtil.screenshot()).toMatchSnapshot({
name: `2-before-deleting-${splitByGrapheme}.png`,
});
// continuos line deletion are a part of the test,
// an old bug was shifting style and then deleting it all at once
await canvasUtil.press('Backspace');
await canvasUtil.press('Backspace');
await expect(await canvasUtil.screenshot()).toMatchSnapshot({
Expand All @@ -66,11 +68,10 @@ for (const splitByGrapheme of [true, false]) {
await canvasUtil.press('b');
await canvasUtil.press('c');
await canvasUtil.press('Enter');
await canvasUtil.press('Enter');
await expect(await canvasUtil.screenshot()).toMatchSnapshot({
name: `4-before-pasting-splitByGrapheme-${splitByGrapheme}.png`,
name: `4-after-typing-${splitByGrapheme}.png`,
});
const pastePoint = await textBoxutil.getCanvasCursorPositionAt(36);
const pastePoint = await textBoxutil.getCanvasCursorPositionAt(35);
await canvasUtil.click({
position: pastePoint,
delay: 200,
Expand All @@ -80,5 +81,29 @@ for (const splitByGrapheme of [true, false]) {
name: `5-after-pasting-splitByGrapheme-${splitByGrapheme}.png`,
maxDiffPixelRatio: 0.03,
});
// go back where we have 2 empty lines, after abc.
const clickPointEnd = await textBoxutil.getCanvasCursorPositionAt(20);
await canvasUtil.click({
position: clickPointEnd,
delay: 200,
});
await page.mouse.down();
await page.mouse.move(1, clickPointEnd.y + 150, { steps: 15 });
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@ShaMan123 ShaMan123 Nov 4, 2023

Choose a reason for hiding this comment

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

screenshot

85800a532aaae57a102d53213c2b8114a0cadb8e.webm

Copy link
Contributor

@ShaMan123 ShaMan123 Nov 4, 2023

Choose a reason for hiding this comment

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

Checkout src files to master
image

const clickPointYellow = await textBoxutil.getCanvasCursorPositionAt(45);
await canvasUtil.click({
position: clickPointYellow,
delay: 200,
});
await canvasUtil.press('Enter');
// this part of test is valid if the new line is after a styled char,
// 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.03,
});
});
}
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file not shown.
Binary file not shown.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

this is splitByGrapheme true so this will not take care of words, will just wraps whenever it feels it has to wrap.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you look at v5 that is not the same I think

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
15 changes: 9 additions & 6 deletions src/shapes/IText/ITextBehavior.ts
Original file line number Diff line number Diff line change
Expand Up @@ -805,9 +805,10 @@ export abstract class ITextBehavior<
copiedStyle?: { [index: number]: TextStyleDeclaration }
) {
const newLineStyles: { [index: number]: TextStyleDeclaration } = {};
const isEndOfLine =
this._unwrappedTextLines[lineIndex].length === charIndex;
let somethingAdded = false;
const originalLineLength = this._unwrappedTextLines[lineIndex].length;
const isEndOfLine = originalLineLength === charIndex;

let someStyleIsCarringOver = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean someStyleIsCarryingOver?

Copy link
Contributor

Choose a reason for hiding this comment

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

@gloriousjob you can use the suggestion markdown
image

Suggested change
let someStyleIsCarringOver = false;
let someStyleIsCarryingOver = false;

qty || (qty = 1);
this.shiftLineStyles(lineIndex, qty);
const currentCharStyle = this.styles[lineIndex]
Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
someStyleIsCarringOver = true;
someStyleIsCarryingOver = true;

newLineStyles[numIndex - charIndex] = this.styles[lineIndex][index];
// remove lines from the previous line since they're on a new line now
if (!(isEndOfLine && charIndex === 0)) {
Expand All @@ -828,14 +829,16 @@ export abstract class ITextBehavior<
}
}
let styleCarriedOver = false;
if (somethingAdded && !isEndOfLine) {
if (someStyleIsCarringOver && !isEndOfLine) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (someStyleIsCarringOver && !isEndOfLine) {
if (someStyleIsCarryingOver && !isEndOfLine) {

// if is end of line, the extra style we copied
// is probably not something we want
this.styles[lineIndex + qty] = newLineStyles;
styleCarriedOver = true;
}
if (styleCarriedOver) {
if (styleCarriedOver || originalLineLength > charIndex) {
// skip the last line of since we already prepared it.
// or contains text without style that we don't want to style
// just becuase it changed line
ShaMan123 marked this conversation as resolved.
Show resolved Hide resolved
qty--;
}
// for the all the lines or all the other lines
Expand Down
Loading