Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Prevent dom clutter #168

Merged
merged 3 commits into from

3 participants

@qfox

Removes transform for the identity matrix and pointer-events for the default of inherit. Maybe more props can be prevented but I think some are used elsewhere.

@davidaurelio

I have no idea what this attribute is good for. There is no text attribute in svg. Ideas @basecode @padolsey?

As far as I can see it sets an attribute on <tspan>s, but there’s no point in that

Owner

No idea.

@davidaurelio davidaurelio merged commit b3da285 into master
@basecode basecode commented on the diff
src/renderer/svg/svg.js
@@ -20,6 +20,22 @@ define([
// targets webkit based browsers from version 530.0 to 534.4
var isWebkitPatternBug = /AppleWebKit\/53([0-3]|4.([0-4]))/.test(navigator.appVersion);
+ /**
+ * Sets a style property on a style object. Avoids unnecessary creation of
+ * style attributes in the DOM, to ease debugging.
+ *
+ * @param {CSSStyleDeclaration} style The style object to set the property on
+ * @param {string} name The name of the property to set
+ * @param {string} value The value of the property to set.
+ */
+ function setStyle(style, name, value) {
@basecode Owner
basecode added a note

Is closured. How are you going to test that function?

@davidaurelio Owner

I’m not. It uses 3rd-Party API and is part of the implementation, not the public interface

@basecode Owner
basecode added a note

Yep, you're right!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 48 additions and 23 deletions.
  1. +48 −23 src/renderer/svg/svg.js
View
71 src/renderer/svg/svg.js
@@ -20,6 +20,22 @@ define([
// targets webkit based browsers from version 530.0 to 534.4
var isWebkitPatternBug = /AppleWebKit\/53([0-3]|4.([0-4]))/.test(navigator.appVersion);
+ /**
+ * Sets a style property on a style object. Avoids unnecessary creation of
+ * style attributes in the DOM, to ease debugging.
+ *
+ * @param {CSSStyleDeclaration} style The style object to set the property on
+ * @param {string} name The name of the property to set
+ * @param {string} value The value of the property to set.
+ */
+ function setStyle(style, name, value) {
@basecode Owner
basecode added a note

Is closured. How are you going to test that function?

@davidaurelio Owner

I’m not. It uses 3rd-Party API and is part of the implementation, not the public interface

@basecode Owner
basecode added a note

Yep, you're right!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ if (value) {
+ style[name] = value;
+ } else if (style[name]) { // only remove if set to prevent empty style attributes in the DOM
+ style[name] = '';
+ }
+ }
+
// Math
var min = Math.min;
var max = Math.max;
@@ -41,19 +57,20 @@ define([
fontPrefix = AssetController.handlers.Font.prefix;
var basicAttributeMap = {
- cap: 'stroke-linecap',
- join: 'stroke-linejoin',
- miterLimit: 'stroke-miterlimit',
- opacity: 'opacity',
- fillOpacity: 'fill-opacity',
- strokeOpacity: 'stroke-opacity',
- fontSize: 'font-size',
- fontWeight: 'font-weight',
- fontStyle: 'font-style',
- textAnchor: 'text-anchor',
- text: 'text',
- cursor: 'cursor',
- fillRule: 'fill-rule'
+ // bonsai attribute: [svg attribute, default value]
+ cap: ['stroke-linecap', 'butt'],
+ join: ['stroke-linejoin', 'miter'],
+ miterLimit: ['stroke-miterlimit', '4'],
+ opacity: ['opacity', '1'],
+ fillOpacity: ['fill-opacity', '1'],
+ strokeOpacity: ['stroke-opacity', '1'],
+ fontSize: ['font-size'],
+ fontWeight: ['font-weight'],
+ fontStyle: ['font-style'],
+ textAnchor: ['text-anchor', 'start'],
+ text: ['text'],
+ cursor: ['cursor', 'inherit'],
+ fillRule: ['fill-rule', 'inherit']
};
var eventTypes = [
@@ -130,17 +147,21 @@ define([
value = attributes[i];
if (i in basicAttributeMap) {
- if (value != null) {
- el.setAttribute(basicAttributeMap[i], value);
- } else if (value === null) {
- el.removeAttribute(basicAttributeMap[i]);
+ var attributeInfo = basicAttributeMap[i];
+ var attributeName = attributeInfo[0], defaultValue = attributeInfo[1];
+
+ var isInitalValue = '' + value === defaultValue;
+ if (value !== null && !isInitalValue) {
+ el.setAttribute(attributeName, value);
+ } else if (value === null || isInitalValue) {
+ el.removeAttribute(attributeName);
}
continue;
}
switch (i) {
case 'interactive':
- el.style.pointerEvents = value ? 'inherit' : 'none';
+ setStyle(el.style, 'pointerEvents', value ? '' : 'none');
break;
case 'fontFamily':
value = fontIDs[value] || value;
@@ -179,10 +200,14 @@ define([
break;
case 'matrix':
if (value != null) {
- el.setAttribute(
- 'transform',
- matrixToString(value)
- );
+ // clear transform attribute for identity matrix
+ var strMatrix = matrixToString(value);
+ if (strMatrix == 'matrix(1,0,0,1,0,0)') {
+ // this is the default
+ el.removeAttribute('transform');
+ } else {
+ el.setAttribute('transform', strMatrix);
+ }
} else if (value === null) {
el.removeAttribute('transform');
}
@@ -502,7 +527,7 @@ define([
}
if (attr.visible != null) {
- element.style.visibility = attr.visible ? '' : 'hidden';
+ setStyle(element.style, 'visibility', attr.visible ? '' : 'hidden');
}
if (type === 'Path' || type === 'Text' || type === 'TextSpan') {
Something went wrong with that request. Please try again.