Skip to content

Commit

Permalink
Fix(clip): delete clipping region with restore (#73)
Browse files Browse the repository at this point in the history
@hustcc please release a patch version after this pull request to version 2.2.1. 

Thanks!

* Fix(clip): delete clipping region with restore

Previously, the restore() method would update its pointer to the previous
clipping region (stackIndex = n-1) in the stack without deleting the current
clipping region (stackIndex = n). This caused problems for future save() calls,
which would push a new clipping region onto the stack at location n+1 while
only incrementing stackIndex to n, thus referencing the wrong region.

This commit fixes the issue by deleting the current clipping region when
restore() is called. By doing this, we lose the ability to inspect old
clipping regions, but this is easy to change in the future if it is desired.

* Address PR comments

Tests have been combined and make better use of the afterEach snapshots.

* Remove excess checks in test
  • Loading branch information
lekha committed Dec 4, 2020
1 parent 340de22 commit c4d1fb2
Show file tree
Hide file tree
Showing 5 changed files with 55 additions and 0 deletions.
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ canvas.toDataURL.mockReturnValueOnce(
- [@hustcc](https://github.com/hustcc)
- [@jtenner](https://github.com/jtenner)
- [@evanoc0](https://github.com/evanoc0)
- [@lekha](https://github.com/lekha)

## License

Expand Down
11 changes: 11 additions & 0 deletions __tests__/classes/CanvasRenderingContext2D.__getClippingPath.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,4 +51,15 @@ describe('__getClippingRegion', () => {
ctx.restore();
expect(region).toStrictEqual(ctx.__getClippingRegion());
});

it('should delete current clipping region when restored', () => {
ctx.rect(1, 2, 3, 4);
ctx.clip();
ctx.save();
ctx.rect(1, 2, 3, 4);
ctx.arc(1, 2, 3, 4, 5);
ctx.clip();
ctx.restore();
ctx.save();
});
});
20 changes: 20 additions & 0 deletions __tests__/classes/CanvasRenderingContext2D.restore.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
let canvas;
let ctx;

beforeEach(() => {
canvas = document.createElement('canvas');
ctx = canvas.getContext('2d');
canvas.width = 400;
canvas.height = 300;
});

describe('save', () => {
it('should be a function', () => {
expect(typeof ctx.restore).toBe('function');
});

it('should be callable', () => {
ctx.restore();
expect(ctx.restore).toBeCalled();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,28 @@

exports[`__getClippingRegion should be empty when there are no path elements 1`] = `Array []`;

exports[`__getClippingRegion should delete current clipping region when restored 1`] = `
Array [
Object {
"props": Object {
"height": 4,
"width": 3,
"x": 1,
"y": 2,
},
"transform": Array [
1,
0,
0,
1,
0,
0,
],
"type": "rect",
},
]
`;

exports[`__getClippingRegion should save the clipping region correctly when saved 1`] = `
Array [
Object {
Expand Down
1 change: 1 addition & 0 deletions src/classes/CanvasRenderingContext2D.js
Original file line number Diff line number Diff line change
Expand Up @@ -1477,6 +1477,7 @@ export default class CanvasRenderingContext2D {
if (this._stackIndex <= 0) return;

this._transformStack.pop();
this._clipStack.pop();
this._directionStack.pop();
this._fillStyleStack.pop();
this._filterStack.pop();
Expand Down

0 comments on commit c4d1fb2

Please sign in to comment.