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 render preserve #2120

Merged
merged 3 commits into from
Apr 17, 2015
Merged

Fix render preserve #2120

merged 3 commits into from
Apr 17, 2015

Conversation

asturur
Copy link
Member

@asturur asturur commented Apr 16, 2015

This fix make active group render ok with preserveObjectStacking.
Otherwise we should revert #2083.

It sound more reasonable to keep the render as it is and add this fix, expecially for big pathgroup of complex SVGs.

@@ -748,6 +748,9 @@
* @param {Boolean} fromLeft When true, context is transformed to object's top/left corner. This is used when rendering text on Node
*/
transform: function(ctx, fromLeft) {
if (this.group && this.canvas.preserveObjectStacking && this.group === this.canvas._activeGroup) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is breaking for me right now because this.cavas doesn't seem to be defined, which is odd because all groups should have a canvas.

Edit: Wait, shouldn't this be this.group.canvas, and not this.canvas?

@inssein
Copy link
Contributor

inssein commented Apr 16, 2015

@asturur thanks for the quick fix, there is just a little mistake that I have mentioned as a line note. Also, I'm sure you see this already, but there is a code style failure (trailing whitespace).

@asturur
Copy link
Member Author

asturur commented Apr 16, 2015

can you show me how you get this.canvas undefined?

not getting it.

kangax added a commit that referenced this pull request Apr 17, 2015
@kangax kangax merged commit 0f502a5 into fabricjs:master Apr 17, 2015
@inssein
Copy link
Contributor

inssein commented Apr 17, 2015

@asturur just emailed you back, but if anyone else is wondering, its because objects inside a group don't have access to this.canvas, but they can get it through this.group.canvas. Maybe this is a separate issue.

@asturur
Copy link
Member Author

asturur commented Apr 17, 2015

No is just demostration that this fix is not 100% safe.
Reverting this AND #2083 till i have proper fix is better, but the problem happens just on .preserveObjectStacking = true, so maybe the fix will come before the issues.

@asturur
Copy link
Member Author

asturur commented Apr 21, 2015

as found out from @sapics, i post the problem of this fix.
Or i found proper fix or better revert this and #2083.
i m on it.

ws000012

@vizo
Copy link

vizo commented Jul 16, 2015

Hey @asturur ,
i have the same problem like @inssein. preserveObjectStacking = true show (image) objects on selection in upper left corner. Is this fixed yet?

@vizo
Copy link

vizo commented Jul 16, 2015

Ok i see in master is already working... thanks anyway ...;)

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.

4 participants