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

Update svgcanvas.js #57

Merged
merged 1 commit into from
Feb 15, 2016
Merged

Update svgcanvas.js #57

merged 1 commit into from
Feb 15, 2016

Conversation

StalderT
Copy link
Contributor

correct #54

@codedread
Copy link
Member

So we don't know why this doesn't work in IE but we can just skip doing the offset?

@StalderT
Copy link
Contributor Author

For info : this patch master...StalderT:patch-4 doesn't work :

ie

@StalderT
Copy link
Contributor Author

With patch master...StalderT:patch-4 :

ie2

codedread added a commit that referenced this pull request Feb 15, 2016
@codedread codedread merged commit e26c7cd into SVG-Edit:master Feb 15, 2016
if (typeof(svgroot.getIntersectionList) == 'function') {
// Offset the bbox of the rubber box by the offset of the svgcontent element.
rubberBBox.x += parseInt(svgcontent.getAttribute('x'), 10);
rubberBBox.y += parseInt(svgcontent.getAttribute('y'), 10);

Choose a reason for hiding this comment

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

I wonder whether the root of the problem is modifying x/y in place. At least that's what the error seems to imply.

Perhaps instead svgcontent.createSVGRect should be invoked here similarly to line 553 above?

@magnebra
Copy link
Contributor

This fixes the "interface" problem StalderT describes: [https://github.com/magnebra/svgedit/commit/88ef7b02cc1325451e86a4298f9cc3d917f437de]

@mihailik
Copy link

It looks more like a workaround specifically for IE.

I wonder whether the code instead of creating empty rect and modifying that instance should calculate the rect coords, and then create the rect with those coords?

Look at the error: NoModificationAllowedError. To me it looks like that SVGRect may be an immutable instance. They don't want you to change properties on an existing instance, they want you to create a new instance and pass values in constructor.

Note also from MDN/SVGRect:

An SVGRect object can be designated as read only, which means that attempts to modify the object will result in an exception being thrown.
[ . . . ]
Exceptions on setting: a DOMException with code NO_MODIFICATION_ALLOWED_ERR is Raised on an attempt to change the value of a read only attribute.

My suspicion is that the error is not really related to IE. You can see there are multiple branches in this getInteresectionList function — some of them go through mutation of rubberBBox, others create a new instance. I suspect in IE you are more likely to fall into the mutation branch and thus get errors. But perhaps on non-IE browsers that may happen less often, and we might still have the bug appearing although rarely.

So the suggestion is to avoid modifying existing SVGRect and in all cases create a new instance. Does it make sense?

jfhenon added a commit that referenced this pull request Mar 14, 2021
Fixes #53: 4th option for the background display fixed
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