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

Fix gradient parsing #5836

Merged
merged 19 commits into from
Aug 12, 2019
Merged

Fix gradient parsing #5836

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
d391c63
a test file
asturur Jul 22, 2019
f23bf1a
improvements
asturur Aug 10, 2019
001218d
improvements
asturur Aug 10, 2019
d029fc8
improvements
asturur Aug 10, 2019
fe3d5a8
fixed tests
asturur Aug 10, 2019
2cdc443
fix lint
asturur Aug 11, 2019
78f594b
fix lint
asturur Aug 11, 2019
46f60e4
changes to jsdoc
asturur Aug 11, 2019
69f9dff
more jsdocs and deprecations
asturur Aug 11, 2019
05aec06
some small extra test
asturur Aug 11, 2019
01fe62a
fixed typo
asturur Aug 11, 2019
5e2e845
more tests
asturur Aug 11, 2019
54f20de
fix some gradients export
asturur Aug 11, 2019
695c13a
added more code
asturur Aug 12, 2019
c2a697a
lint in tests
asturur Aug 12, 2019
7931ee6
added a visual test
asturur Aug 12, 2019
8843f53
added a visual test for svg export
asturur Aug 12, 2019
14db4d7
but i killed a test
asturur Aug 12, 2019
b8a8c32
added a simple UT
asturur Aug 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/elements_parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ fabric.ElementsParser = function(elements, callback, options, reviver, parsingOp
var gradientDef = this.extractPropertyDefinition(obj, property, 'gradientDefs');
if (gradientDef) {
var opacityAttr = el.getAttribute(property + '-opacity');
var gradient = fabric.Gradient.fromElement(gradientDef, obj, opacityAttr);
var gradient = fabric.Gradient.fromElement(gradientDef, obj, opacityAttr, this.options);
obj.set(property, gradient);
}
};
Expand Down
155 changes: 90 additions & 65 deletions src/gradient.class.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,18 +95,59 @@
*/
offsetY: 0,

/**
* A transform matrix to apply to the gradient before paiting.
* It shares the same name with the svg property but behaves differently.
* @type Array[Number]
* @default null
*/
gradientTransform: null,
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. painting
  2. behaves differently how? Describe the layout and the members of this matrix, please, enough to describe them for people coming along in the future.
/**
 * | x1 x2 0 0 
 * | ... |
 * ...
 * x1: a description of x1
 * ...
 */

Yes, I know, JavaDoc is probably insufficient for this (I've never seen a 2-D array defined in JavaDoc in detail), but some documentation of the members is better than none.

Even better would be to make this an instance of an explicit unit-testable matrix class. I realize that would be a significant refactor, but I'd be very surprised if you didn't have a need for it by now elsewhere in the code. I do see there's a fabric.ColorMatrix class, but that appears to be something else.


/**
* coordinates units for coords.
* If `pixels`, the number of cords are in the same unit of width/ height.
Copy link
Contributor

Choose a reason for hiding this comment

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

coords

* If set as `percentage` the coords are still a number, but 1 means 100% of width
* for the X and 100% of the height for the y. It can be bigger than 1 and negative.
* @type String pixels || percentage
* @default 'pixels'
*/
gradientUnits: 'pixels',

/**
* Gradient type
* @type String linear || radial
* @default 'pixels'
*/
type: 'linear',

/**
* Constructor
* @param {Object} [options] Options object with type, coords, gradientUnits and colorStops
* @param {Object} options Options object with type, coords, gradientUnits and colorStops
* @param {Object} [options.type] gradient type
* @param {Object} [options.gradientUnits] gradient units
* @param {Object} [options.offsetX] SVG import compatibility
* @param {Object} [options.offsetY] SVG import compatibility
* @param {Object} options.coords contains the coords of the gradient
* @param {Array[Object]} options.colorStops contains the colorstops.
* @param {Number} [options.coords.x1]
* @param {Number} [options.coords.y1]
* @param {Number} [options.coords.x2]
* @param {Number} [options.coords.y2]
* @param {Number} [options.coords.r1]
* @param {Number} [options.coords.r2]
Copy link
Contributor

Choose a reason for hiding this comment

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

options.coords should probably be next to options.coords.x1. Move colorstops up one line?

* @return {fabric.Gradient} thisArg
*/
initialize: function(options) {
options || (options = { });

var coords = { };
var coords = { }, _this = this;

this.id = fabric.Object.__uid++;
this.type = options.type || 'linear';
// sets everything, then coords and colorstops get sets again
Object.keys(options).forEach(function(option) {
_this[option] = options[option];
});

!this.id && (this.id = fabric.Object.__uid++);

coords = {
x1: options.coords.x1 || 0,
Expand All @@ -119,13 +160,9 @@
coords.r1 = options.coords.r1 || 0;
coords.r2 = options.coords.r2 || 0;
}

this.coords = coords;
this.colorStops = options.colorStops.slice();
if (options.gradientTransform) {
this.gradientTransform = options.gradientTransform;
}
this.offsetX = options.offsetX || this.offsetX;
this.offsetY = options.offsetY || this.offsetY;
},

/**
Expand Down Expand Up @@ -307,7 +344,7 @@
* @see http://www.w3.org/TR/SVG/pservers.html#LinearGradientElement
* @see http://www.w3.org/TR/SVG/pservers.html#RadialGradientElement
*/
fromElement: function(el, instance, opacityAttr) {
fromElement: function(el, instance, opacityAttr, svgOptions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You might as well add a javadoc line for svgOptions.

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, i m not done yet, svg export is now broken and needs to be adequate to the new logic.

/**
* @example:
*
Expand Down Expand Up @@ -349,43 +386,45 @@

var colorStopEls = el.getElementsByTagName('stop'),
type,
gradientUnits = el.getAttribute('gradientUnits') || 'objectBoundingBox',
gradientTransform = el.getAttribute('gradientTransform'),
gradientUnits = el.getAttribute('gradientUnits') === 'userSpaceOnUse' ?
'pixels' : 'percentage',
gradientTransform = el.getAttribute('gradientTransform') || '',
colorStops = [],
coords, ellipseMatrix, i;

coords, i, offsetX = 0, offsetY = 0,
transformMatrix;
if (el.nodeName === 'linearGradient' || el.nodeName === 'LINEARGRADIENT') {
type = 'linear';
coords = getLinearCoords(el);
}
else {
type = 'radial';
}

if (type === 'linear') {
coords = getLinearCoords(el);
}
else if (type === 'radial') {
coords = getRadialCoords(el);
}

for (i = colorStopEls.length; i--; ) {
colorStops.push(getColorStop(colorStopEls[i], multiplier));
}

ellipseMatrix = _convertPercentUnitsToValues(instance, coords, gradientUnits);
transformMatrix = fabric.parseTransformAttribute(gradientTransform);

__convertPercentUnitsToValues(instance, coords, svgOptions, gradientUnits);

if (gradientUnits === 'pixels') {
offsetX = -instance.left;
offsetY = -instance.top;
}

var gradient = new fabric.Gradient({
id: el.getAttribute('id'),
type: type,
coords: coords,
colorStops: colorStops,
offsetX: -instance.left,
offsetY: -instance.top
gradientUnits: gradientUnits,
gradientTransform: transformMatrix,
offsetX: offsetX,
offsetY: offsetY,
});

if (gradientTransform || ellipseMatrix !== '') {
gradient.gradientTransform = fabric.parseTransformAttribute((gradientTransform || '') + ellipseMatrix);
}

return gradient;
},
/* _FROM_SVG_END_ */
Expand All @@ -399,54 +438,40 @@
*/
forObject: function(obj, options) {
options || (options = { });
_convertPercentUnitsToValues(obj, options.coords, 'userSpaceOnUse');
__convertPercentUnitsToValues(obj, options.coords);
return new fabric.Gradient(options);
}
});

/**
* @private
*/
function _convertPercentUnitsToValues(object, options, gradientUnits) {
var propValue, addFactor = 0, multFactor = 1, ellipseMatrix = '';
for (var prop in options) {
if (options[prop] === 'Infinity') {
options[prop] = 1;
}
else if (options[prop] === '-Infinity') {
options[prop] = 0;
function __convertPercentUnitsToValues(instance, options, svgOptions, gradientUnits) {
var propValue, finalValue;
Object.keys(options).forEach(function(prop) {
propValue = options[prop];
if (propValue === 'Infinity') {
finalValue = 1;
}
propValue = parseFloat(options[prop], 10);
if (typeof options[prop] === 'string' && /^(\d+\.\d+)%|(\d+)%$/.test(options[prop])) {
multFactor = 0.01;
else if (propValue === '-Infinity') {
finalValue = 0;
}
else {
multFactor = 1;
}
if (prop === 'x1' || prop === 'x2' || prop === 'r2') {
multFactor *= gradientUnits === 'objectBoundingBox' ? object.width : 1;
addFactor = gradientUnits === 'objectBoundingBox' ? object.left || 0 : 0;
}
else if (prop === 'y1' || prop === 'y2') {
multFactor *= gradientUnits === 'objectBoundingBox' ? object.height : 1;
addFactor = gradientUnits === 'objectBoundingBox' ? object.top || 0 : 0;
}
options[prop] = propValue * multFactor + addFactor;
}
if (object.type === 'ellipse' &&
options.r2 !== null &&
gradientUnits === 'objectBoundingBox' &&
object.rx !== object.ry) {

var scaleFactor = object.ry / object.rx;
ellipseMatrix = ' scale(1, ' + scaleFactor + ')';
if (options.y1) {
options.y1 /= scaleFactor;
}
if (options.y2) {
options.y2 /= scaleFactor;
finalValue = parseFloat(options[prop], 10);
if (typeof propValue === 'string' && /^(\d+\.\d+)%|(\d+)%$/.test(propValue)) {
finalValue *= 0.01;
if (gradientUnits === 'pixels') {
// then we need to fix those percentages here in svg parsing
if (prop === 'x1' || prop === 'x2' || prop === 'r2') {
finalValue *= svgOptions.viewBoxWidth || svgOptions.width;
}
if (prop === 'y1' || prop === 'y2') {
finalValue *= svgOptions.viewBoxHeight || svgOptions.height;
}
}
}
}
}
return ellipseMatrix;
options[prop] = finalValue;
});
}
})();
33 changes: 9 additions & 24 deletions src/mixins/object_geometry.mixin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,11 +478,7 @@
* @return {Array} rotation matrix for the object
*/
_calcRotateMatrix: function() {
if (this.angle) {
var theta = degreesToRadians(this.angle), cos = fabric.util.cos(theta), sin = fabric.util.sin(theta);
return [cos, sin, -sin, cos, 0, 0];
}
return fabric.iMatrix.concat();
return fabric.util.calcRotateMatrix(this);
},

/**
Expand Down Expand Up @@ -546,22 +542,14 @@
},

_calcDimensionsTransformMatrix: function(skewX, skewY, flipping) {
var skewMatrix,
scaleX = this.scaleX * (flipping && this.flipX ? -1 : 1),
scaleY = this.scaleY * (flipping && this.flipY ? -1 : 1),
scaleMatrix = [scaleX, 0, 0, scaleY, 0, 0];
if (skewX) {
skewMatrix = [1, 0, Math.tan(degreesToRadians(skewX)), 1];
scaleMatrix = multiplyMatrices(scaleMatrix, skewMatrix, true);
}
if (skewY) {
skewMatrix = [1, Math.tan(degreesToRadians(skewY)), 0, 1];
scaleMatrix = multiplyMatrices(scaleMatrix, skewMatrix, true);
}
return scaleMatrix;
return fabric.util.calcDimensionsMatrix({
skewX: skewX,
skewY: skewY,
scaleX: this.scaleX * (flipping && this.flipX ? -1 : 1),
scaleY: this.scaleY * (flipping && this.flipY ? -1 : 1)
});
},


/*
* Calculate object dimensions from its properties
* @private
Expand Down Expand Up @@ -625,12 +613,9 @@
x: dimX,
y: dimY
}],
i, transformMatrix = this._calcDimensionsTransformMatrix(skewX, skewY, false),
transformMatrix = this._calcDimensionsTransformMatrix(skewX, skewY, false),
bbox;
for (i = 0; i < points.length; i++) {
points[i] = fabric.util.transformPoint(points[i], transformMatrix);
}
bbox = fabric.util.makeBoundingBoxFromPoints(points);
bbox = fabric.util.makeBoundingBoxFromPoints(points, transformMatrix);
return this._finalizeDimensions(bbox.width, bbox.height);
},

Expand Down
8 changes: 5 additions & 3 deletions src/parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,12 +559,14 @@
parsedDim.height = parseUnit(heightAttr);
return parsedDim;
}

minX = -parseFloat(viewBoxAttr[1]);
minY = -parseFloat(viewBoxAttr[2]);
viewBoxWidth = parseFloat(viewBoxAttr[3]);
viewBoxHeight = parseFloat(viewBoxAttr[4]);

parsedDim.minX = minX;
parsedDim.minY = minY;
parsedDim.viewBoxWidth = viewBoxWidth;
parsedDim.viewBoxHeight = viewBoxHeight;
if (!missingDimAttr) {
parsedDim.width = parseUnit(widthAttr);
parsedDim.height = parseUnit(heightAttr);
Expand Down Expand Up @@ -723,7 +725,7 @@
recursivelyParseGradientsXlink(doc, referencedGradient);
}
gradientsAttrs.forEach(function(attr) {
if (!gradient.hasAttribute(attr)) {
if (referencedGradient && !gradient.hasAttribute(attr) && referencedGradient.hasAttribute(attr)) {
gradient.setAttribute(attr, referencedGradient.getAttribute(attr));
}
});
Expand Down
9 changes: 8 additions & 1 deletion src/shapes/object.class.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1474,10 +1474,17 @@
var t = filler.gradientTransform || filler.patternTransform;
var offsetX = -this.width / 2 + filler.offsetX || 0,
offsetY = -this.height / 2 + filler.offsetY || 0;
ctx.translate(offsetX, offsetY);
// ctx.translate(offsetX, offsetY);
if (filler.gradientUnits === 'percentage') {
ctx.transform(this.width, 0, 0, this.height, offsetX, offsetY);
}
else {
ctx.transform(1, 0, 0, 1, offsetX, offsetY);
}
if (t) {
ctx.transform(t[0], t[1], t[2], t[3], t[4], t[5]);
}
console.log(filler)
return { offsetX: offsetX, offsetY: offsetY };
},

Expand Down
Loading