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 for IE11 IndexSizeError for negative source crops #5428

Merged
merged 8 commits into from
Jan 13, 2019

Conversation

CaptEmulation
Copy link
Contributor

This codepen will not render on IE11 https://codepen.io/captemulation/pen/PXPORb (copy to your own codepen and replace pen with debug to view on IE11)

In IE11, if a negative number is passed in for sx or sy when using the 9 argument version of drawImage an IndexSizeError exception will be thrown. This can occur with rounding errors making a number just past 0, e.g. -2.4e-12.

Negative numbers do not really make sense here.

@asturur
Copy link
Member

asturur commented Dec 30, 2018

@CaptEmulation eventually also the source width/height must be at max the naturalWidth/naturalHeight ( width/height ) of the element we are about to draw.
That is source of another indexSizeError.

Can you fix that too?

@CaptEmulation
Copy link
Contributor Author

Can you fix that too?

Taking a looksee

@CaptEmulation
Copy link
Contributor Author

Add the additional boundary check and added a test. Could not follow test style because I could not run anything in image test suite (timeout). IDE also picked up on some reformatting.

dist/fabric.js Outdated
x, y, w, h);
var elementToDraw = this._element,
w = this.width, h = this.height,
sW = Math.min(elementToDraw.naturalWidth || elementToDraw.width, w * this._filterScalingX),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sW = Math.min(elementToDraw.naturalWidth || elementToDraw.width, w * this._filterScalingX),
sW = Math.min(elementToDraw.naturalWidth || elementToDraw.width, w) * this._filterScalingX,

we want to limit the source area, this._filterScaling apply anyway

dist/fabric.js Outdated
var elementToDraw = this._element,
w = this.width, h = this.height,
sW = Math.min(elementToDraw.naturalWidth || elementToDraw.width, w * this._filterScalingX),
sH = Math.min(elementToDraw.naturalHeight || elementToDraw.height, h * this._filterScalingY),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sH = Math.min(elementToDraw.naturalHeight || elementToDraw.height, h * this._filterScalingY),
sH = Math.min(elementToDraw.naturalHeight || elementToDraw.height, h) * this._filterScalingY,

@asturur
Copy link
Member

asturur commented Jan 11, 2019

Thanks for the test too.
Can you checkout away the changes to dist folder?

i usually do git checkout master dist and i rest them.

@CaptEmulation
Copy link
Contributor Author

Understood. Thanks and will do!

@asturur
Copy link
Member

asturur commented Jan 12, 2019

image
ok we broke something.
i probably suggested something wrong?

@asturur
Copy link
Member

asturur commented Jan 12, 2019

oh well if there is a filter scaling, the elementToDraw is already resized to the right dimension of width * filterScalingX. So as you did in the first version, the code was right, with filterScaling inside the parenthesis.

@asturur asturur merged commit 1cf38e1 into fabricjs:master Jan 13, 2019
@asturur asturur mentioned this pull request Jan 13, 2019
thiagocunha pushed a commit to thiagocunha/fabric.js that referenced this pull request Nov 18, 2019
* Fix for IE11 IndexSizeError for negative source crops
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants