search

svgdotjs / svg.js

The lightweight library for manipulating and animating SVG
https://svgjs.dev
Other
11.04k stars 1.08k forks source link

Firefox Compatibility #1133

Closed CetinOzdil closed 4 years ago

CetinOzdil commented 4 years ago

Hello,

When using SVG.js with Firefox i was getting matrix transformation errors. After checking a bit I have noticed that all values was NaN. Adding a NaN check to section below solved my issue.

https://github.com/svgdotjs/svg.js/blob/dccb11974fe1fc96f68e3e4eca2a1fd7f30543c2/src/types/Matrix.js#L197-L203

Just need to change it like this:

this.a = source.a != null && !isNaN(source.a) ? source.a : base.a
this.b = source.b != null && !isNaN(source.b) ? source.b : base.b
this.c = source.c != null && !isNaN(source.c) ? source.c : base.c
this.d = source.d != null && !isNaN(source.d) ? source.d : base.d
this.e = source.e != null && !isNaN(source.e) ? source.e : base.e
this.f = source.f != null && !isNaN(source.f) ? source.f : base.f
Fuzzyma commented 4 years ago

Please provide some more context. Whats the code which leads to this? Ofc creating a marix will fail when only NaNs are passed. But thats not the Matrixs "fault". Adding a NaN check just masks the error

CetinOzdil commented 4 years ago

The code leads to NaN's was setting a group objects y() property to some value.

And you were right about source of the problem. Traced it again and I think it is related to forEach in dmove, here.

https://github.com/svgdotjs/svg.js/blob/dccb11974fe1fc96f68e3e4eca2a1fd7f30543c2/src/modules/core/containerGeometry.js#L5-L18

If i change it to a old school for-loop like below it works correctly.

export function dmove (dx, dy) {
    var childs = this.children();

    for(var i = 0; i < childs.lenght; i++)
    {
        var child = childs[i];

       // Get the childs bbox
        const bbox = child.bbox()
        // Get childs matrix
        const m = new Matrix(child)
        // Translate childs matrix by amount and
        // transform it back into parents space
        const matrix = m.translate(dx, dy).transform(m.inverse())
        // Calculate new x and y from old box
        const p = new Point(bbox.x, bbox.y).transform(matrix)
        // Move element
        child.move(p.x, p.y)
    }

    return this
}
Fuzzyma commented 4 years ago

I am not sure how a change from forEach to for-loop solve that issue here. They are the basically the same. So this is most likely related to some other code change you did.

CetinOzdil commented 4 years ago

I have isolated the issue and sample code to reproduce problem is here. 

<html>
<head>
    <script src="https://cdnjs.cloudflare.com/ajax/libs/svg.js/3.0.15/svg.min.js"></script>
    <script>
        document.addEventListener("DOMContentLoaded",() => 
        {
            var svg = SVG().addTo("body").size(50, 50);
            var symbol = svg.group();

            var back = symbol.group();
            var mask = symbol.group();

            back.rect(40, 40).fill("blue");
            mask.rect(40, 40).fill("white");

            symbol.maskWith(mask);

            mask.height(20);
            mask.y(0);  
        });
    </script>
</head>
<body>
</body>
</html>

Tried with a Firefox without any addons to be sure and it still give same NaN error at console. Console BlueBox Firefox

But Chrome happily renders it. BlueBoxChrome

Fuzzyma commented 4 years ago

So what happens is the following:

You create a group which you later use as mask (.maskWith(mask)). This function creates a <mask> element in the defs and adds the group to it. However, elements in the defs section are not rendered. Therefore browser apis like getBBox or getSceenCTM will fail or return zero-values (at least in firefox). And that is, why your code is failing.

Either dont use move on a group but instead on the elements in the group or move the group before you convert it to a mask

CetinOzdil commented 4 years ago

Weirdly when i change for-loop it works. Maybe some timing issue? Anyway thanks for feedback and helping.