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(Controls): allow for smooth scaling fo text at any zoom level #10186

Merged
merged 4 commits into from
Oct 3, 2024
Merged

fix(Controls): allow for smooth scaling fo text at any zoom level #10186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3a37c46
removed Math.ceil call when calculating new width in resize handler
Soldier-B Sep 27, 2024
049d329
prevent newWidth from being less than 1
Soldier-B Oct 3, 2024
ff95419
added a test for decimal
asturur Oct 3, 2024
12b8e1a
changelog
asturur Oct 3, 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(Controls): changeWidth can change width with decimals [#10186](https://github.com/fabricjs/fabric.js/pull/10186)
- ci(): Add some prebuilt fabric in the dist folder [#10178](https://github.com/fabricjs/fabric.js/pull/10178)

## [6.4.2]
Expand Down
9 changes: 9 additions & 0 deletions src/controls/changeWidth.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,15 @@ describe('changeWidth', () => {
expect(target.top).toBe(0);
});

test('changeWidth changes the width with decimals', () => {
expect(target.width).toBe(100);
const changed = changeWidth(eventData, transform, 200.2, 300);
expect(changed).toBe(true);
expect(target.width).toBe(199.2);
expect(target.left).toBe(0);
expect(target.top).toBe(0);
});
Copy link
Member

Choose a reason for hiding this comment

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

@Soldier-B there were plenty of tests and adding a new one was trivial


test('changeWidth does not change the width', () => {
const target = new Rect({ width: 100, height: 100, canvas });
jest.spyOn(target, '_set').mockImplementation(function _set(this: Rect) {
Expand Down
7 changes: 3 additions & 4 deletions src/controls/changeWidth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,9 @@ export const changeObjectWidth: TransformActionHandler = (
target.strokeWidth / (target.strokeUniform ? target.scaleX : 1),
multiplier = isTransformCentered(transform) ? 2 : 1,
oldWidth = target.width,
newWidth = Math.ceil(
Math.abs((localPoint.x * multiplier) / target.scaleX) - strokePadding,
);
target.set('width', Math.max(newWidth, 0));
newWidth =
Math.abs((localPoint.x * multiplier) / target.scaleX) - strokePadding;
Copy link
Member

Choose a reason for hiding this comment

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

Since this was avoiding newWidth to go under 1, could you change the line below to
target.set('width', Math.max(newWidth, 1)); ?

Copy link
Member

Choose a reason for hiding this comment

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

Also please check if there is a test for changeWidth and extend it to cover this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@asturur Sure I can make that change but I just want to make sure it's necessary as versions before v6 didn't have that restriction.
Also, I'm not familiar with how the tests are set up but there doesn't appear to be any existing test that run against the changeWidth function. I did try making changes locally to the controls_handlers test script but it's unclear to me how I'm supposed to reference the changeWidth function there.

Copy link
Member

Choose a reason for hiding this comment

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

if the previous code was using Math.ceil, it means that any float that was smaller than 1 was rounded to 1.
As a consequence newWidth was never smaller than 1.
Now your changes remove that to obtain smoothness at higher zoom levels but that 1 barrier should stay, and the line below is a perfect place to reimplement it.

A test i can add when the code changes are done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just committed that change.

target.set('width', Math.max(newWidth, 1));
// check against actual target width in case `newWidth` was rejected
return oldWidth !== target.width;
}
Expand Down
Loading