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.

Sign up for GitHub

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

fix(Text) Fix style transfer issue on a line that is not empty #9461

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
961c903
fixed style transfer issue
asturur Oct 30, 2023
53c0f4b
update CHANGELOG.md
github-actions[bot] Oct 30, 2023
87ca8fc
added new step to test
asturur Oct 30, 2023
7430586
comment
asturur Oct 30, 2023
babff4c
Merge branch 'text-new-line-style-fix' of github.com:fabricjs/fabric.…
asturur Oct 30, 2023
b4ee60b
Merge branch 'master' into text-new-line-style-fix
asturur Nov 3, 2023
489d58f
Merge branch 'master' into text-new-line-style-fix
ShaMan123 Nov 4, 2023
831fcc2
Merge branch 'master' into text-new-line-style-fix
asturur Dec 7, 2023
71ddf31
add test steps
asturur Dec 7, 2023
214eedc
adding steps correctly
asturur Dec 7, 2023
4fd3548
tests
asturur Dec 7, 2023
0fdb4e5
vscode
asturur Dec 7, 2023
26b6e4d
less pixels
asturur Dec 7, 2023
29dcc2e
approximate
asturur Dec 7, 2023
b84e6f8
correct shots
asturur Dec 7, 2023
d79b01a
fix mispelling
asturur Dec 11, 2023
5efa2df
smaller shots
asturur Dec 11, 2023
13415f6
focused screenshot
asturur Dec 11, 2023
edfccde
more allowancethat still fail locally
asturur Dec 11, 2023
f4a1eb6
Merge branch 'master' into text-new-line-style-fix
asturur Dec 11, 2023
ba0f3fe
Update src/shapes/IText/ITextBehavior.ts
ShaMan123 Dec 11, 2023
e269f09
Update index.spec.ts
asturur Dec 18, 2023
f6960b7
Merge branch 'master' into text-new-line-style-fix
asturur Jan 13, 2024
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
View file
Open in desktop
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)
- test() Backport a test to capture a failing text style situation [#9531](https://github.com/fabricjs/fabric.js/pull/9531)

## [6.0.0-beta16]
Expand Down
2 changes: 1 addition & 1 deletion e2e/tests/text/adding-text/index.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

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

await expect(page).toHaveScreenshot(`${name}.png`, {
          clip: {
            x,
            y,
            width,
            height,
          },
          maxDiffPixels, // insteadof percentage
        });

Copy link
Member Author

Choose a reason for hiding this comment

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

where is this clip in the documentation? i looked for something like this.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh ok is an entirely different way to do visual comparison. the locator can't be clipped.

Copy link
Contributor

Choose a reason for hiding this comment

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

but the image can, right?

});
});
});
Expand Down
Binary file modified ...text/index.spec.ts-snapshots/6-after-adding-a-newline-splitByGrapheme-false.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified ...-text/index.spec.ts-snapshots/6-after-adding-a-newline-splitByGrapheme-true.png
View file
Open in desktop
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
View file
Open in desktop
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
qty--;
}
// for the all the lines or all the other lines
Expand Down