Skip to content
Open
Show file tree
Hide file tree
Changes from all 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): skip `initDimensions` in `set` when a `textLayoutProperty` value did not change [#11029](https://github.com/fabricjs/fabric.js/pull/11029)
- chore(deps-dev): bump rolldown from 1.1.0 to 1.1.2 [#11025](https://github.com/fabricjs/fabric.js/pull/11025)
- fix(sandbox): restore vanilla startup with pnpm [#11012](https://github.com/fabricjs/fabric.js/pull/11012)
- fix(): nested duplicated clipPath causes infinite recursion [#10774](https://github.com/fabricjs/fabric.js/pull/10774)
Expand Down
58 changes: 57 additions & 1 deletion src/shapes/Text/Text.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { config } from '../../config';
import { Path } from '../Path';
import { FabricText } from './Text';

import { describe, expect, it, afterEach } from 'vitest';
import { describe, expect, it, afterEach, vi } from 'vitest';
import {
FabricObject,
getFabricDocument,
Expand Down Expand Up @@ -1346,4 +1346,60 @@ describe('FabricText', () => {
expect(cacheKeys?.next().done).toBe(true);
});
});

describe('set & initDimensions', () => {
it('does not call initDimensions when a textLayoutProperty is set to the same value', () => {
const text = new FabricText('hello');
const initSpy = vi.spyOn(text, 'initDimensions');
const setCoordsSpy = vi.spyOn(text, 'setCoords');

// set to the same value it already has
text.set('text', 'hello');
expect(
initSpy,
'initDimensions is not called for a no-op set',
).not.toHaveBeenCalled();
expect(setCoordsSpy).not.toHaveBeenCalled();

// changing the value does trigger it
text.set('text', 'world');
expect(
initSpy,
'initDimensions is called when value changes',
).toHaveBeenCalledTimes(1);
expect(
setCoordsSpy,
'setCoords is called when value changes',
).toHaveBeenCalledTimes(1);

initSpy.mockRestore();
setCoordsSpy.mockRestore();
});

it('does not call initDimensions when textLayoutProperties are set to the same values via object', () => {
const text = new FabricText('hello', { fontSize: 40, charSpacing: 0 });
const initSpy = vi.spyOn(text, 'initDimensions');
const setCoordsSpy = vi.spyOn(text, 'setCoords');

text.set({ text: 'hello', fontSize: 40, charSpacing: 0 });
expect(
initSpy,
'no initDimensions for no-op object set',
).not.toHaveBeenCalled();
expect(setCoordsSpy).not.toHaveBeenCalled();

text.set({ text: 'hello', fontSize: 50, charSpacing: 0 });
expect(
initSpy,
'initDimensions runs when at least one value changes',
).toHaveBeenCalledTimes(1);
expect(
setCoordsSpy,
'setCoords runs when at least one value changes',
).toHaveBeenCalledTimes(1);

initSpy.mockRestore();
setCoordsSpy.mockRestore();
});
});
});
18 changes: 12 additions & 6 deletions src/shapes/Text/Text.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1806,21 +1806,27 @@ export class FabricText<

set(key: string | any, value?: any) {
const { textLayoutProperties } = this.constructor as typeof FabricText;
super.set(key, value);
// detect layout-relevant changes before super.set mutates the instance,
// so initDimensions is only requested when a value actually changed.
let needsDims = false;
let isAddingPath = false;
if (typeof key === 'object') {
for (const _key in key) {
if (_key === 'path') {
this.setPathInfo();
}
needsDims = needsDims || textLayoutProperties.includes(_key);
isAddingPath = isAddingPath || _key === 'path';
needsDims =
needsDims ||
(textLayoutProperties.includes(_key) &&
this[_key as keyof this] !== key[_key]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

}
} else {
needsDims = textLayoutProperties.includes(key);
isAddingPath = key === 'path';
needsDims =
textLayoutProperties.includes(key) && this[key as keyof this] !== value;
}

super.set(key, value);

// setPathInfo must run after super.set so it reads the new path
if (isAddingPath) {
this.setPathInfo();
}
Expand Down
Loading