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

Jump to bottom

fix a crash in getImageDate if the rectangle is outside the canvas #2145

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

fix a crash in getImageDate if the rectangle is outside the canvas #2145

wants to merge 4 commits into from

Conversation

phipla
Copy link
Contributor

@phipla phipla commented Oct 26, 2022 •
edited

Thanks for contributing!

  • Have you updated CHANGELOG.md?

Fixes #2024

@phipla
Copy link
Contributor Author

phipla commented Oct 27, 2022

The tests failing seem related to the issue addressed by #2133 . Once this PR is rebased on #2133 the tests pass.

@phipla phipla force-pushed the pr/fix-getimagedata-crash-if-rectangle-outside-canvas branch from 66ab835 to 149e13b Compare November 2, 2022 07:29
@phipla
Copy link
Contributor Author

phipla commented Nov 2, 2022

Rebased

@chearon
Copy link
Collaborator

chearon commented Jan 3, 2023

Looks good, thanks. Can you fix the conflicts? "Allow edits by maintainers" is off.

@chearon
Copy link
Collaborator

chearon commented Jan 3, 2023

(while you're at it, typo in the commit summary: s/getImageDate/getImageData)

@LinusU
Copy link
Collaborator

LinusU commented Jan 4, 2023

Sorry for being a bit late to this.

I think that this change is adding behaviour that is not in line with how canvas should work?

Screenshot 2023-01-04 at 09 29 56

While it is of course great that we are fixing a crash, I think it would be better to throw an "not implemented" error rather than returning an incorrectly sized ImageData? Or am I missing something? 🤔

@zbjornson
Copy link
Collaborator

@LinusU is correct, the spec is sort of surprising here:

Pixels outside the canvas must be returned as transparent black.

See #1849 for more info and link to the spec.

@phipla are you up for trying to implement the standard behavior?

@phipla
Copy link
Contributor Author

phipla commented Jan 5, 2023

You are right (it was the existing behavior of the lib when the rectangle was partly outside, and I wanted to just fix the crash, not alter the existing behavior).

However I can alter it to comply with the standard. I will update the PR in a few days.

@phipla phipla force-pushed the pr/fix-getimagedata-crash-if-rectangle-outside-canvas branch from 149e13b to 7281e95 Compare January 7, 2023 18:26
@phipla phipla force-pushed the pr/fix-getimagedata-crash-if-rectangle-outside-canvas branch from 7281e95 to ad01a10 Compare January 7, 2023 19:25
Philippe Plantier added 2 commits January 8, 2023 10:41
remove dead code, add comments
3c6577b
Merge commit 'fdf709a7b08abae33a93c510b96f71df6c13c7b0' into pr/fix-g…
07e167d 
…etimagedata-crash-if-rectangle-outside-canvas

* commit 'fdf709a7b08abae33a93c510b96f71df6c13c7b0':
  move ctx.font string to the state struct

# Conflicts:
#	CHANGELOG.md
@phipla
Copy link
Contributor Author

phipla commented Mar 29, 2023

@zbjornson @LinusU Any input on the requested changes? This PR should now fix issues #2024 (crash) and #1849 (non-compliant behavior when retrieving data partly or completely outside the canvas)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonaskistler jonaskistler jonaskistler approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Node crash in getImageData
5 participants
@phipla @chearon @LinusU @zbjornson @jonaskistler