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

feat(Vectorizer): add option to support camel case attributes #2339

Merged
merged 4 commits into from
Oct 9, 2023
Merged

feat(Vectorizer): add option to support camel case attributes #2339

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9816da8
feat(Vectorizer): add option to support camel case attributes
kumilingus Sep 16, 2023
17e2823
update
kumilingus Sep 16, 2023
f920e20
update
kumilingus Oct 8, 2023
291b167
update
kumilingus Oct 8, 2023
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
108 changes: 94 additions & 14 deletions src/V/index.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -663,15 +663,17 @@ const V = (function() {
*/
VPrototype.removeAttr = function(name) {

var qualifiedName = V.qualifyAttr(name);
var el = this.node;
const trueName = attributeNames[name];

if (qualifiedName.ns) {
if (el.hasAttributeNS(qualifiedName.ns, qualifiedName.local)) {
el.removeAttributeNS(qualifiedName.ns, qualifiedName.local);
const { ns, local } = V.qualifyAttr(trueName);
const el = this.node;

if (ns) {
if (el.hasAttributeNS(ns, local)) {
el.removeAttributeNS(ns, local);
}
} else if (el.hasAttribute(name)) {
el.removeAttribute(name);
} else if (el.hasAttribute(trueName)) {
el.removeAttribute(trueName);
}
return this;
};
Expand All @@ -692,7 +694,7 @@ const V = (function() {
}

if (V.isString(name) && V.isUndefined(value)) {
return this.node.getAttribute(name);
return this.node.getAttribute(attributeNames[name]);
}

if (typeof name === 'object') {
Expand Down Expand Up @@ -1257,23 +1259,24 @@ const V = (function() {
*/
VPrototype.setAttribute = function(name, value) {

var el = this.node;
const el = this.node;

if (value === null) {
this.removeAttr(name);
return this;
}

var qualifiedName = V.qualifyAttr(name);
const trueName = attributeNames[name];

if (qualifiedName.ns) {
const { ns } = V.qualifyAttr(trueName);
if (ns) {
// Attribute names can be namespaced. E.g. `image` elements
// have a `xlink:href` attribute to set the source of the image.
el.setAttributeNS(qualifiedName.ns, name, value);
} else if (name === 'id') {
el.setAttributeNS(ns, trueName, value);
} else if (trueName === 'id') {
el.id = value;
} else {
el.setAttribute(name, value);
el.setAttribute(trueName, value);
}

return this;
Expand Down Expand Up @@ -1378,6 +1381,83 @@ const V = (function() {
return xml;
};

const _attributeNames = Object.create(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object#null-prototype_objects

Do we need to use a null-prototype object here? It can apparently lead to unexpected behavior, which may be problematic (e.g. if a customer tries to modify the list of attributes for which not to split camel-case).

I think we should be able to use a normal object here, since we are then using a Proxy object anyways to specify the custom getter (line 1444).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What problems specifically?

This:

V('rect', { toString: true }).node

Should produce:

<rect to-string="true"/>

And it would not if we define _attributeNames as {}.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not questioning that part, but my point is that the {} which would be used as a replacement would be a normal Object (i.e. with an Object prototype) - so if using that is okay, then it should be okay to say const _attributes = {} on this line by default, as well.

Examples of potential problems of using null-prototype objects according to the MDN link:

  • An object with a null prototype can behave in unexpected ways, because it doesn't inherit any object methods from Object.prototype. This is especially true when debugging, since common object-property converting/detecting utility functions may generate errors, or lose information (especially if using silent error-traps that ignore errors).
  • The main problem problem is with missing toString, valueOf, hasOwnProperty functions and the constructor property

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment explaining why we use an object without prototype.
Note It's a private variable and can not be accessed from outside.


// List of attributes for which not to split camel case words.
[
'baseFrequency',
'baseProfile',
'clipPathUnits',
'contentScriptType',
'contentStyleType',
'diffuseConstant',
'edgeMode',
'externalResourcesRequired',
'filterRes', // deprecated
'filterUnits',
'gradientTransform',
'gradientUnits',
'kernelMatrix',
'kernelUnitLength',
'keyPoints',
'lengthAdjust',
'limitingConeAngle',
'markerHeight',
'markerUnits',
'markerWidth',
'maskContentUnits',
'maskUnits',
'numOctaves',
'pathLength',
'patternContentUnits',
'patternTransform',
'patternUnits',
'pointsAtX',
'pointsAtY',
'pointsAtZ',
'preserveAlpha',
'preserveAspectRatio',
'primitiveUnits',
'refX',
'refY',
'requiredExtensions',
'requiredFeatures',
'specularConstant',
'specularExponent',
'spreadMethod',
'startOffset',
'stdDeviation',
'stitchTiles',
'surfaceScale',
'systemLanguage',
'tableValues',
'targetX',
'targetY',
'textLength',
'viewBox',
'viewTarget', // deprecated
'xChannelSelector',
'yChannelSelector',
'zoomAndPan' // deprecated
].forEach((name) => _attributeNames[name] = name);

const attributeNames = new Proxy(_attributeNames, {
get(cache, name) {
kumilingus marked this conversation as resolved.
Show resolved Hide resolved
if (!V.supportCamelCaseAttributes) return name;
if (name in cache) {
return cache[name];
}
// Convert camel case to dash-separated words.
return (cache[name] = name.replace(/[A-Z]/g, '-$&').toLowerCase());
}
});

// Dictionary of attribute names
V.attributeNames = attributeNames;

// Should camel case attributes be supported?
V.supportCamelCaseAttributes = false;

/**
* @param {string} name
* @returns {{ns: string|null, local: string}} namespace and attribute name
Expand Down
40 changes: 40 additions & 0 deletions test/vectorizer/vectorizer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -922,6 +922,46 @@ QUnit.module('vectorizer', function(hooks) {
});
});

QUnit.module('camel case support', function(hooks) {

hooks.before(function() {
V.supportCamelCaseAttributes = true;
});

hooks.after(function() {
V.supportCamelCaseAttributes = false;
});

QUnit.test('constructor', function(assert) {
const vel = V('rect', { strokeWidth: 5 });
assert.equal(vel.node.getAttribute('stroke-width'), 5);
});

QUnit.test('attr()', function(assert) {
const vel = V('rect');
vel.attr('strokeWidth', 5);
assert.equal(vel.attr('strokeWidth'), 5);
assert.equal(vel.attr('stroke-width'), 5);
assert.equal(vel.node.getAttribute('stroke-width'), 5);
vel.attr('stroke-width', 10);
assert.equal(vel.attr('strokeWidth'), 10);
assert.equal(vel.attr('stroke-width'), 10);
assert.equal(vel.node.getAttribute('stroke-width'), 10);
vel.attr('strokeWidth', null);
assert.equal(vel.attr('strokeWidth'), null);
assert.equal(vel.attr('stroke-width'), null);
assert.equal(vel.node.getAttribute('stroke-width'), null);
});

QUnit.test('removeAttr()', function(assert) {
const vel = V('rect');
vel.attr('strokeWidth', 5);
assert.equal(vel.node.getAttribute('stroke-width'), 5);
vel.removeAttr('strokeWidth');
assert.equal(vel.node.getAttribute('stroke-width'), null);
});
});

QUnit.test('remove simple', function(assert) {

var a = V('a').attr('href', 'www.seznam.cz');
Expand Down