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

Restore green check on tests #2712

Merged
merged 4 commits into from
Feb 7, 2016
Merged

Restore green check on tests #2712

merged 4 commits into from
Feb 7, 2016

Conversation

asturur
Copy link
Member

@asturur asturur commented Dec 26, 2015

Attempt to fix group transformMatrix test on isTransparent function.

@asturur
Copy link
Member Author

asturur commented Dec 26, 2015

#2624 was ok, #2632 no more. In the middle there is #2625 that i try to rebuild but is not possible to do anymore but it was ok.

@asturur
Copy link
Member Author

asturur commented Dec 26, 2015

I really cannot get what is happening here. It looks like it is not drawing. May it be an async issue? i also tried to disable retina scaling for the canvas test thinking that window.pixelRatio may have some strange value.

@asturur
Copy link
Member Author

asturur commented Dec 31, 2015

https://ide.c9.io/asturur/nodefabric-nodecanvas

@inssein @kangax i put up this example of this problem in nodejs on cloud9.

I'm trying to figure out this failing test. In browser is not failing, it renders correctly.

If i use toDataUrl i get just blank images.

Transparency test is working before apply trasformMatrix ( as expected ), after the pixels i expect to be transparents are not.

getImageData looks ok, toDataUrl not.

Anyone willing to get a look at it?

@asturur
Copy link
Member Author

asturur commented Jan 7, 2016

@inssein you look experienced with nodejs + fabricjs, i'm not.
Any hint on what is happening here?

@asturur
Copy link
Member Author

asturur commented Feb 2, 2016

@jhundley9109 you are welcome to continue here.

@jhundley9109
Copy link

I think I have figured out the problem. There are two issues on the head commit that need fixed. In renderAll for static canvas from commit 8082dae canvasToDrawOn.restore() is extra and I think it can be removed. Additionally in __toDataURL, from commit 520e8e6 var canvasEl = this.upperCanvasEl || this.lowerCanvasEl, was switched to var canvasEl = this.lowerCanvasEl,. I don't know exactly how this works, but switching that line to var canvasEl = this.contextContainer.canvas fixed the problem.

Doing both of these changes fixed the toDataURL on node. Since I don't really know the downstream effects of changing that to use the contextContainer.canvas, you guys should review. Let me know what you think.

@asturur
Copy link
Member Author

asturur commented Feb 2, 2016

I will check personally because i wanna close this story.

@asturur asturur changed the title Transform matrix Restore green check on tests Feb 7, 2016
@asturur
Copy link
Member Author

asturur commented Feb 7, 2016

@kangax would you merge this please?
We have fixed tests and confirmed that toDataUrl has problems.

I'm working on toDataUrl problem but i would like to check again all the PRs with green marks asap.

@kangax
Copy link
Member

kangax commented Feb 7, 2016

Hm, why the failures?

@asturur
Copy link
Member Author

asturur commented Feb 7, 2016

What failures? The one before the last?

kangax added a commit that referenced this pull request Feb 7, 2016
@kangax kangax merged commit c79e872 into fabricjs:master Feb 7, 2016
@asturur asturur deleted the transformMatrix branch February 7, 2016 23:52
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.

3 participants